[6/?] StaticAddr: Quoting #750

hieblmi · 2024-05-22T15:57:30Z

~~the last 4 commits are relevant~~

~~Depends on #721~~

This PR introduces quoting for static address loop-ins while reusing the existing loop-in quoting infrastructure.

lightninglabs-deploy · 2024-06-14T08:36:31Z

@bhandras: review reminder
@starius: review reminder
@sputn1ck: review reminder

starius

The first part of the review :-)
Reviewed until commit staticaddr: static address manager.

starius · 2024-06-19T17:38:09Z

loopdb/sqlc/migrations/000009_static_address.down.sql

@@ -0,0 +1 @@
+DROP TABLE IF EXISTS static_addresses;


Line end missing.

starius · 2024-06-19T17:38:16Z

loopdb/sqlc/migrations/000009_static_address.up.sql

+    -- downgrades their protocol version for static address outputs already in
+    -- use.
+    protocol_version INTEGER NOT NULL
+);


Line end missing.

starius · 2024-06-19T17:42:20Z

loopdb/sqlc/migrations/000009_static_address.up.sql

+
+    -- expiry denotes the CSV delay at which funds at a specific static address
+    -- can be swept back to the client.
+    expiry INT NOT NULL,


In other places INTEGER is used, not INT. Can INTEGER be used here as well. (Also in next two fields.)

starius · 2024-06-19T17:42:58Z

loopdb/sqlc/queries/static_addresses.sql

+             $5,
+             $6,
+             $7
+         );


Line end missing.

starius · 2024-06-19T17:50:13Z

swapserverrpc/server.proto

+    bytes server_key = 1;
+
+    // The CSV expiry for the MuSig2 static address output.
+    uint32 expiry = 2;


Line end missing.

starius · 2024-06-19T18:27:48Z

staticaddr/sql_store.go

+		return nil, err
+	}
+
+	var result []*AddressParameters


result := make([]*AddressParameters, 0, len(staticAddresses))

starius · 2024-06-19T18:29:44Z

staticaddr/sql_store.go

+func (s *SqlStore) ExecTx(ctx context.Context, txOptions loopdb.TxOptions,
+	txBody func(queries *sqlc.Queries) error) error {


Can you call baseDB's ExecTx here?

starius · 2024-06-19T18:34:20Z

staticaddr/manager.go

+	m.Lock()
+	addresses, err := m.cfg.Store.GetAllStaticAddresses(ctx)
+	if err != nil {
+		m.Unlock()


Can m.Unlock() call be done right after GetAllStaticAddresses call? In all 3 branches the first action is m.Unlock().

starius · 2024-06-19T18:35:20Z

staticaddr/manager.go

+		clientPubKey := addresses[0].ClientPubkey
+		serverPubKey := addresses[0].ServerPubkey
+		expiry := int64(addresses[0].Expiry)
+


Is the order of addresses deterministic? If there are more than one, can addresses[0] be different in several calls (would be confusing for the user).

starius · 2024-06-19T18:40:57Z

staticaddr/manager.go

+		ClientPubkey: clientPubKey.PubKey,
+		ServerPubkey: serverPubKey,
+		PkScript:     pkScript,
+		Expiry:       serverParams.Expiry,


Should serverParams.Expiry be validated? E.g. that it is within some range (positive, not too high, fits into int32).

starius

LGTM for last 4 commits 🥇

starius · 2024-06-19T20:26:54Z

loopd/swapclient_server.go

+	// The requested amount is 0 here if the request contained deposit
+	// outpoints. In case we quote for deposits we send the server both the


Can this be enforced with a check?

if amount != 0 && len(summary.FilteredDeposits) > 0 { return nil, error

starius · 2024-06-19T20:29:05Z

cmd/loop/quote.go

@@ -33,20 +35,36 @@ var quoteInCommand = cli.Command{
 		verboseFlag,
 		privateFlag,
 		routeHintsFlag,
+		cli.StringSliceFlag{
+			Name: "deposit_outpoint",
+			Usage: "one or more static address deposit outpoints " +


one or more

Can you specify how to pass multiple outpoints, please?

starius · 2024-06-20T02:16:28Z

cmd/loop/quote.go

+	var (
+		depositAmt btcutil.Amount
+	)


var depositAmt btcutil.Amount
also it can be moved before the loop, since it is used there.

hieblmi · 2024-06-20T08:25:44Z

Thank you for your review @starius, there seems to have been outdated commits present in your last review. I've now rebased with the latest master changes.

It would be great if you could give the prevous PR #721 a look.

hieblmi · 2024-06-20T19:31:38Z

loopd/swapclient_server.go

@@ -1395,6 +1426,201 @@ func (s *swapClientServer) WithdrawDeposits(ctx context.Context,
 	return &clientrpc.WithdrawDepositsResponse{}, err
 }

+// GetStaticAddressSummary returns a summary static address related information.


nit: grammar

bhandras

LGTM, very clean PR 🎉

bhandras · 2024-06-21T17:46:25Z

cmd/loop/staticaddr.go

+var summaryCommand = cli.Command{
+	Name:      "summary",
+	ShortName: "s",
+	Usage:     "Display a summary of static address related data.",


nit: instead of "data" i'd word it as "information".

bhandras · 2024-06-21T17:53:45Z

loopd/swapclient_server.go

+func filter(deposits []*deposit.Deposit, f filterFunc) []*clientrpc.Deposit {
+	var clientDeposits []*clientrpc.Deposit
+	for _, d := range deposits {
+		if f(d) {


style+nit: if !f(d) { continue }?

bhandras · 2024-06-21T17:54:49Z

looprpc/client.proto

@@ -760,6 +760,13 @@ message QuoteRequest {
    probing and payment.
    */
    bool private = 7;
+
+    /*
+    Static address deposit outpoints that will be quoted for. This option only


nit: commit message could be reformatted for readability.

bhandras · 2024-06-21T17:58:02Z

cmd/loop/quote.go

@@ -33,20 +35,36 @@ var quoteInCommand = cli.Command{
 		verboseFlag,
 		privateFlag,
 		routeHintsFlag,
+		cli.StringSliceFlag{
+			Name: "deposit_outpoint",
+			Usage: "one or more static address deposit outpoints " +


This commit adds static address deposit outpoints to the QuoteRequest message. It allows to quote for loop in swaps with the total value of specified deposits.

This commit adds the number of deposits to the ServerLoopInQuoteRequest that the client wants to quote for.

hieblmi self-assigned this May 22, 2024

hieblmi added the static-loop-in label May 22, 2024

hieblmi marked this pull request as draft May 22, 2024 15:57

hieblmi force-pushed the static-addr-6 branch 3 times, most recently from be24617 to f6d49f9 Compare May 24, 2024 09:21

hieblmi force-pushed the static-addr-staging branch 2 times, most recently from f614090 to 70a74bd Compare May 27, 2024 11:20

hieblmi force-pushed the static-addr-staging branch 7 times, most recently from dbc351a to c4012d0 Compare June 5, 2024 09:37

hieblmi force-pushed the static-addr-6 branch 2 times, most recently from ac2a2ae to e1d833d Compare June 6, 2024 11:48

hieblmi force-pushed the static-addr-staging branch from 385f3ea to 17618f2 Compare June 6, 2024 11:48

hieblmi force-pushed the static-addr-6 branch 2 times, most recently from 18593dc to 470ce4e Compare June 6, 2024 13:31

hieblmi marked this pull request as ready for review June 6, 2024 13:32

hieblmi requested review from bhandras, starius and sputn1ck June 6, 2024 13:32

hieblmi force-pushed the static-addr-6 branch 2 times, most recently from ec46c45 to 08bb91d Compare June 7, 2024 07:34

hieblmi force-pushed the static-addr-6 branch from 08bb91d to bcd6dd6 Compare June 19, 2024 14:09

starius reviewed Jun 19, 2024

View reviewed changes

hieblmi force-pushed the static-addr-staging branch from 17618f2 to 6f9539b Compare June 19, 2024 21:03

starius approved these changes Jun 20, 2024

View reviewed changes

hieblmi force-pushed the static-addr-staging branch from 6f9539b to add5263 Compare June 20, 2024 08:18

hieblmi force-pushed the static-addr-6 branch from bcd6dd6 to f009427 Compare June 20, 2024 08:21

hieblmi force-pushed the static-addr-6 branch 2 times, most recently from d94cabe to 11e9b12 Compare June 20, 2024 09:01

hieblmi commented Jun 20, 2024

View reviewed changes

bhandras approved these changes Jun 21, 2024

View reviewed changes

hieblmi force-pushed the static-addr-staging branch from add5263 to 48cc041 Compare June 24, 2024 17:00

hieblmi force-pushed the static-addr-6 branch 2 times, most recently from bef49c2 to 5184bbd Compare June 24, 2024 17:12

hieblmi added 3 commits June 25, 2024 13:55

looprpc: deposit outpoints for QuoteRequest

bfdfd92

This commit adds static address deposit outpoints to the QuoteRequest message. It allows to quote for loop in swaps with the total value of specified deposits.

swapserverrpc: number of deposits for ServerLoopInQuoteRequest

ee26e8f

This commit adds the number of deposits to the ServerLoopInQuoteRequest that the client wants to quote for.

loopd: quoting for static address loop-ins

91a93f4

hieblmi force-pushed the static-addr-6 branch from 5184bbd to 850a75b Compare June 25, 2024 12:06

cmd: quote for static address loop-ins

850a75b

hieblmi merged commit 3e289a7 into lightninglabs:static-addr-staging Jun 25, 2024
4 checks passed

hieblmi deleted the static-addr-6 branch June 25, 2024 12:39

hieblmi mentioned this pull request Jun 28, 2024

[7/?] StaticAddr: Loop-In #786

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[6/?] StaticAddr: Quoting #750

[6/?] StaticAddr: Quoting #750

hieblmi commented May 22, 2024 •

edited

Loading

lightninglabs-deploy commented Jun 14, 2024

starius left a comment

starius Jun 19, 2024

starius Jun 19, 2024

starius Jun 19, 2024

starius Jun 19, 2024

starius Jun 19, 2024

starius Jun 19, 2024

starius Jun 19, 2024

starius Jun 19, 2024

starius Jun 19, 2024

starius Jun 19, 2024

starius left a comment

starius Jun 19, 2024

starius Jun 19, 2024

bhandras Jun 21, 2024

starius Jun 20, 2024

hieblmi commented Jun 20, 2024

hieblmi Jun 20, 2024

bhandras left a comment

bhandras Jun 21, 2024

bhandras Jun 21, 2024

bhandras Jun 21, 2024

bhandras Jun 21, 2024

		func (s *SqlStore) ExecTx(ctx context.Context, txOptions loopdb.TxOptions,
		txBody func(queries *sqlc.Queries) error) error {

		// The requested amount is 0 here if the request contained deposit
		// outpoints. In case we quote for deposits we send the server both the

[6/?] StaticAddr: Quoting #750

[6/?] StaticAddr: Quoting #750

Conversation

hieblmi commented May 22, 2024 • edited Loading

lightninglabs-deploy commented Jun 14, 2024

starius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieblmi commented Jun 20, 2024

Choose a reason for hiding this comment

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieblmi commented May 22, 2024 •

edited

Loading