[custom channels]: add new RPC methods for asset channel specific calls #1048

guggero · 2024-07-24T16:35:27Z

Adds standalone RPC calls to the tapchannelrpc package to simplify usage for RPC users.
These RPCs mirror the functionality that is currently only available in the command line in litd:

litcli ln addinvoice -> tapchannelrpc.AddInvoice
litcli ln payinvoice -> tapchannelrpc.SendPayment
litcli ln sendpayment -> tapchannelrpc.SendPayment

guggero · 2024-07-29T16:26:40Z

Ready for review. Will do manual and integration test validation in lightninglabs/lightning-terminal#807 (since these new RPCs can't really be tested well in tapd standalone).

guggero · 2024-07-30T15:27:16Z

Looking at the failure in the itest (https://github.com/lightninglabs/lightning-terminal/actions/runs/10148102664/job/28060046297?pr=807), I think we need to pass in the RFQ peer's public key to the new calls, so we know what peer to ask for a quote if there are multiple channels with the given asset.
So I'll need to update this PR slightly.

GeorgeTsagk

looks good!
have some Qs

GeorgeTsagk · 2024-07-31T09:10:13Z

rpcserver.go

+	switch {
+	// If there are multiple asset channels for the same asset, we need to
+	// ask the user to specify the peer public key. Otherwise, we don't know
+	// who to ask for a quote.


nit: we could convert this into a feature in next versions, the user may not want to pick a single peer here so instead we could ask for a quote from all peers and choose the "best" one

I agree. But that would be a larger overhaul of the whole RFQ system, so I'd rather do that in a follow-up PR.

GeorgeTsagk · 2024-07-31T09:13:11Z

rpcserver.go

+	// channels, it means we have multiple channels with the same asset and
+	// the same peer. So we just return the first channel.
+	case len(assetBalances) >= 1:
+		return &assetBalances[0], nil


nit: the first channel may be depleted, need to think of rotation

At this level the actual channel balance doesn't matter. We only need the channel object to get the peer public key. We don't want to do balance checks at this level really, as that should be handled by the sending logic.

GeorgeTsagk · 2024-07-31T09:15:03Z

rpcserver.go

+		// (invoice amount plus fee limit) in milli-satoshi, not the
+		// asset amount. Need to change the whole RFQ API to do that
+		// though.
+		maxAssetAmount := assetChan.assetInfo.LocalBalance


I don't get the TODO
Isn't this the asset local balance?

It is currently, but will the threshold will be lowered to match the payment rather than just the entire channel outgoing balance.

We set it to the asset balance because the unit is wrong (right now it's the maximum amount in assets, but the user doesn't know how many assets they need to send, that's the question the RFQ call should answer in the first place). We'll do some refactor and eventually this should be the maximum amount the user is willing to pay in satoshis (invoice amount plus the user-defined maximum routing fee they're willing to pay).

rpcserver.go

ffranr

Would it be valuable to add itests which exercise the new endpoints in this PR? Just checking for example that the outgoing HTLC contains the correct fields.

ffranr · 2024-07-31T10:26:32Z

rpcserver.go

+	// The last step is to create a hop hint that includes the fake SCID of
+	// the quote, alongside the channel's routing policy.
+	channelID := assetChan.channelInfo.ChannelID
+	ourPolicy, err := r.getOurPolicy(ctx, channelID, peerPubKey.String())


Perhaps instead of ourPolicy in both variable name and method name, just channelPolicy? Because "their" policy isn't something we deal with. I have the same sort of thoughts around the getOurPolicy method name and doc.

Every channel has two routing policies: those set by us (valid for the outbound direction) and those set by our peer (valid for the inbound direction). So the our/their kind of made sense IMO.
But while thinking about the name and how to make it more easy to understand, I actually noticed that we need to use the inbound policies (those set by the peer), since this is for a payment coming in.

While doing that, I renamed the function to getInboundPolicy.

lightninglabs-deploy · 2024-08-07T11:11:36Z

@guggero, remember to re-request review from reviewers when ready

This commit allows us to use the gRPC import statement to re-use lnd protobuf definitions. We'll use that to be able to embed lnd RPC messages in some of the new tapchannelrpc calls.

With this commit we make sure that the taproot asset channel functionality RPCs are correctly configured and return the expected error when being called. We can't actually test any of their intended functionality because that would require us to run within a litd context. But the integration tests in the litd repo make sure these RPCs work as expected.

guggero · 2024-08-07T12:31:25Z

Would it be valuable to add itests which exercise the new endpoints in this PR? Just checking for example that the outgoing HTLC contains the correct fields.

As mentioned in the PR body, the full functionality test is done in the litd integration tests.
But I added minimal calls to the RPCs to make sure they're hooked up correctly.

Roasbeef

LGTM 🌛

dstadulis assigned guggero Jul 24, 2024

dstadulis added gRPC tap-channels aux invoice labels Jul 24, 2024

dstadulis mentioned this pull request Jul 25, 2024

[feature]: Add Tapd Sub-Server RPC for simplified LN Functionality #1049

Closed

6 tasks

dstadulis linked an issue Jul 25, 2024 that may be closed by this pull request

[feature]: Add Tapd Sub-Server RPC for simplified LN Functionality #1049

Closed

6 tasks

guggero force-pushed the tap-channel-rpcs branch 2 times, most recently from 3b044ca to 62ded9c Compare July 29, 2024 14:39

guggero mentioned this pull request Jul 29, 2024

[custom channels]: Use new tapchannelrpc RPC methods lightninglabs/lightning-terminal#807

Merged

guggero force-pushed the tap-channel-rpcs branch from 62ded9c to 6acd611 Compare July 29, 2024 16:24

guggero marked this pull request as ready for review July 29, 2024 16:26

guggero requested review from ffranr and GeorgeTsagk July 29, 2024 16:26

guggero changed the title ~~[WIP]: add new RPC methods for asset channel specific calls~~ [custom channels]: add new RPC methods for asset channel specific calls Jul 29, 2024

guggero force-pushed the tap-channel-rpcs branch 2 times, most recently from 4000798 to 664a6a1 Compare July 30, 2024 16:07

GeorgeTsagk approved these changes Jul 31, 2024

View reviewed changes

ffranr reviewed Jul 31, 2024

View reviewed changes

dstadulis added this to the v0.4.2 milestone Aug 5, 2024

guggero added 5 commits August 7, 2024 13:30

mod: bump lndclient version

f72981e

taprpc: allow importing/referencing lnd proto files

266165a

This commit allows us to use the gRPC import statement to re-use lnd protobuf definitions. We'll use that to be able to embed lnd RPC messages in some of the new tapchannelrpc calls.

multi: add SendPayment RPC method

db31799

multi: add AddInvoice RPC method

66be6de

guggero force-pushed the tap-channel-rpcs branch from 664a6a1 to 23c09ff Compare August 7, 2024 12:28

guggero requested a review from ffranr August 8, 2024 15:38

Roasbeef approved these changes Aug 9, 2024

View reviewed changes

Roasbeef merged commit a8d8b5a into main Aug 9, 2024
16 checks passed

guggero deleted the tap-channel-rpcs branch August 9, 2024 06:43

This was referenced Aug 19, 2024

[enhancement] Decide: Refactor EncodeCustomRecords RPC or Introduce New Payment-Specific RPCs #945

Closed

[feature]: add RPC endpoint for generating LN Invoices for Tap assets #884

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[custom channels]: add new RPC methods for asset channel specific calls #1048

[custom channels]: add new RPC methods for asset channel specific calls #1048

guggero commented Jul 24, 2024 •

edited by dstadulis

Loading

guggero commented Jul 29, 2024

guggero commented Jul 30, 2024

GeorgeTsagk left a comment

GeorgeTsagk Jul 31, 2024

guggero Aug 7, 2024

GeorgeTsagk Jul 31, 2024

guggero Aug 7, 2024

GeorgeTsagk Jul 31, 2024

ffranr Jul 31, 2024

guggero Aug 7, 2024

ffranr left a comment

ffranr Jul 31, 2024

guggero Aug 7, 2024

lightninglabs-deploy commented Aug 7, 2024

guggero commented Aug 7, 2024

Roasbeef left a comment

[custom channels]: add new RPC methods for asset channel specific calls #1048

[custom channels]: add new RPC methods for asset channel specific calls #1048

Conversation

guggero commented Jul 24, 2024 • edited by dstadulis Loading

guggero commented Jul 29, 2024

guggero commented Jul 30, 2024

GeorgeTsagk 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

ffranr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Aug 7, 2024

guggero commented Aug 7, 2024

Roasbeef left a comment

Choose a reason for hiding this comment

guggero commented Jul 24, 2024 •

edited by dstadulis

Loading