custom channels: use `tapd` as the aux component implementation for `lnd` in integrated mode #761

guggero · 2024-05-23T12:33:35Z

No description provided.

ellemouton

Had a quick look to gain familiarity - looks good and mostly plumbing 👍 main question is around why the cli commands belong here

ellemouton · 2024-06-06T17:56:56Z

config.go

@@ -379,6 +380,7 @@ func loadAndValidateConfig(interceptor signal.Interceptor) (*Config, error) {
 			cfg.Remote.LitDebugLevel, cfg.Lnd.LogWriter,
 		)
 	} else {
+		SetupLoggers(cfg.Lnd.LogWriter, interceptor)


is this not covered by SetupLoggers(preCfg.Lnd.LogWriter, interceptor) above?

Yes, you're right. I was debugging something logging related and forgot to remove this again. Confirmed it's not needed.

config.go

ellemouton · 2024-06-06T18:03:42Z

terminal.go

+	tapdWrapper, available := g.subServerMgr.GetServer(subservers.TAP)
+	if !available {
+		return nil, fmt.Errorf("tapd is not available, must be " +
+			"started in integrated mode for Taproot Assets " +


started in integrated mode -> do you mean that tapd must be integrated? cause 'integrated mode' to me means that lnd is integrated

Actually, both. I'll update the error message.

terminal.go

ellemouton · 2024-06-06T18:07:18Z

cmd/litcli/ln.go

+
+var lnCommands = []cli.Command{
+	{
+		Name:     "ln",


would tap be better? ln to me is general lightning network

Yeah, I agree. Though I think this is temporary and we'll remove them in a future release anyway (see comment below). But for now I think Polar already uses the commands in some demos, so I don't want to rename them just for renaming sake.

I suppose since it's inducing ln functionality but pairing with tapd it's a bit of both

ellemouton · 2024-06-06T18:11:47Z

cmd/litcli/ln.go

+	"github.com/lightningnetwork/lnd/lnwire"
+	"github.com/lightningnetwork/lnd/record"
+	"github.com/urfave/cli"
+)


just skimmed this commit really quickly but it isnt yet immediately obvious to me why this is here and not in the taproot assets cmd directory? ie what makes this different from loopcli or equivalent?

We do eventually want the logic that's done in this file be RPCs in tapd, then it will make sense to move them to tapcli. But for now we need access to both lnd's and tapd's RPCs from within a single command, which is done most conveniently in tapcli (see the way we bake a macaroon just to connect to tapd).

bitromortac

Really exciting to see how everything fits together 🎉🚀. Left some comments in this first (quick) pass.

bitromortac · 2024-06-07T05:43:41Z

terminal.go

+		return nil, fmt.Errorf("tapd is not available, must be " +
+			"started in integrated mode for Taproot Assets " +
+			"Channels to be available")


The error messages could be more distinguished between running remotely and being available.

Updated the error message according to @ellemouton's review. Hope it makes more sense now.

cmd/litcli/main.go

bitromortac · 2024-06-07T07:06:26Z

cmd/litcli/ln.go

+	msatPerUnit := acceptedQuote.BidPrice
+	numUnits := uint64(decodeResp.NumMsat) / msatPerUnit
+
+	fmt.Printf("Got quote for %v asset units at %v msat/unit from peer "+


We don't need to have user consent here, because sending is blocked by SendPaymentRequest where a user has time to read this?

Hmm, this message isn't really about consent, more of an information (and I guess initially mainly added for debugging purposes 😅 ).
The actual validation of the exchange rate is done by the RFQ and price oracle logic of tapd. The end user will be able to plug in their exchange API of choice to validate prices against, and then define a maximum percentage (PPM actually) of deviation due to spread.

bitromortac · 2024-06-07T07:43:05Z

cmd/litcli/ln.go

+		ValueMsat:       int64(numMSats),
+		DescriptionHash: descHash,
+		FallbackAddr:    ctx.String("fallback_addr"),
+		Expiry:          ctx.Int64("expiry"),


I think this may need to use the default expiry above?

Ah yes, good point. Fixed!

bitromortac · 2024-06-07T08:59:48Z

itest/litd_custom_channels_test.go

+	fabiaAssetBalance += fabiaInvoiceAssetAmount1
+
+	// ------------
+	// Test case 4: Create an asset invoice on Fabian and pay it with just


Why those are multipart payments isn't quite obvious to me, probably also need the assertions to check for the individual htlc attempts (early stage I guess 🙂).

It's a bit hidden, we hard code the max shard size to 80k in the payInvoiceWithXXX() helper. So choosing a large enough asset amount will guarantee at least two shards. Updated the comment to make it a bit more obvious.
And yes, we don't currently track the number of attempts. There are a lot of assertions still missing here, mainly due to the fact that the itest library isn't fully compatible with the refactored version in lnd, so we can't just re-use a lot of those assertions.

terminal.go

guggero · 2024-06-07T16:47:11Z

Thanks for the reviews! I addressed all comments and fixed the failing CI steps (hopefully).
Taking the PR out of draft, even if we still want to add the force-close code. But that will likely only affect the integration test (and the go.mod file).

This commit represents the main integration between lnd running in integrated mode and tapd providing auxiliary components for custom channels.

This change will speed up integration tests by not waiting a full 5 seconds before re-trying the connection to lnd if it fails the first time.

dstadulis assigned guggero May 23, 2024

guggero force-pushed the custom-channels-integration branch 6 times, most recently from 407c675 to b9bbaca Compare May 31, 2024 13:22

guggero force-pushed the custom-channels-integration branch from b9bbaca to 776813b Compare June 3, 2024 11:40

guggero linked an issue Jun 4, 2024 that may be closed by this pull request

itest: Incorporate custom channel itests into main #760

Closed

guggero force-pushed the custom-channels-integration branch from 776813b to 31d4fee Compare June 5, 2024 20:26

ellemouton reviewed Jun 6, 2024

View reviewed changes

guggero force-pushed the custom-channels-integration branch from 31d4fee to 2fa4b07 Compare June 7, 2024 08:42

bitromortac reviewed Jun 7, 2024

View reviewed changes

guggero force-pushed the custom-channels-integration branch 2 times, most recently from 067d09c to ee468ba Compare June 7, 2024 16:45

guggero marked this pull request as ready for review June 7, 2024 16:47

guggero requested review from ellemouton and bitromortac June 7, 2024 16:47

guggero force-pushed the custom-channels-integration branch from ee468ba to 57bf3a0 Compare June 8, 2024 13:45

guggero changed the base branch from master to 0-19-staging June 9, 2024 14:27

guggero added 8 commits June 9, 2024 17:51

multi: bump to lnd/tapd/lndclient custom channel branches

68ed0bd

make+perms: allow itest to run with dev build tag

7055689

subservers: add Impl to subservers, GetServer to mgr

9d7b2c5

litd: register tapd as aux component of lnd

00741e7

This commit represents the main integration between lnd running in integrated mode and tapd providing auxiliary components for custom channels.

litd: allow faster startup by making re-try configurable

a8ac4cd

This change will speed up integration tests by not waiting a full 5 seconds before re-trying the connection to lnd if it fails the first time.

cmd/litcli: add ln sub commands for custom channels

ec337aa

itest: add custom channel integration test

3ed4365

app+proto: update proto files

0b2b3eb

guggero force-pushed the custom-channels-integration branch from 57bf3a0 to 0b2b3eb Compare June 9, 2024 15:51

guggero merged commit 980ee93 into 0-19-staging Jun 9, 2024
12 checks passed

guggero deleted the custom-channels-integration branch June 9, 2024 16:13

guggero mentioned this pull request Jun 10, 2024

itest: Incorporate custom channel itests into main #760

Closed

custom channels: use tapd as the aux component implementation for lnd in integrated mode #761

custom channels: use tapd as the aux component implementation for lnd in integrated mode #761

Uh oh!

Conversation

guggero commented May 23, 2024

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guggero Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guggero commented Jun 7, 2024

Uh oh!

Uh oh!

Uh oh!

custom channels: use `tapd` as the aux component implementation for `lnd` in integrated mode #761

custom channels: use `tapd` as the aux component implementation for `lnd` in integrated mode #761

guggero Jun 7, 2024 •

edited

Loading