Allow setting sats/msats to `taprpc.AddInvoice` #1448

GeorgeTsagk · 2025-03-21T13:47:01Z

Description

This PR allows for the user to set the invoice value via the Value/ValueMsat fields.

Previously we'd take the asset units and the agreed upon quote, then we'd convert that to an msat amount. Now the value of the invoice can be directly set via the previously ignored fields, while also still acquiring a sufficient quote and returning that to the user.

Closes #1440
Fixes #1306

coveralls · 2025-03-21T14:07:10Z

Pull Request Test Coverage Report for Build 14488405809

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

14 of 147 (9.52%) changed or added relevant lines in 3 files are covered.
48 unchanged lines in 10 files lost coverage.
Overall coverage decreased (-0.05%) to 28.327%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rfq/manager.go	0	21	0.0%
rpcserver.go	0	112	0.0%

Files with Coverage Reduction	New Missed Lines	%
rpcserver.go	1	0.0%
asset/group_key.go	2	57.89%
tapchannel/aux_leaf_signer.go	2	43.08%
tappsbt/create.go	2	26.74%
asset/asset.go	3	48.1%
address/address.go	6	67.47%
asset/mock.go	6	63.56%
commitment/tap.go	6	71.36%
tapgarden/caretaker.go	6	68.63%
address/mock.go	14	89.54%

Totals
Change from base Build 14474635473:	-0.05%
Covered Lines:	25922
Relevant Lines:	91511

💛 - Coveralls

rpcserver.go

Roasbeef

Nice work! Pretty straight forward implementation. The main thing we need to ensure is that the negotiated quote has a total amount that's greater than the sats input of the user.

Roasbeef · 2025-03-21T18:56:40Z

rpcserver.go

+	// will help us establish a quote with the correct amount of asset
+	// units.
+	if satsMode {
+		amtMsat = lnwire.MilliSatoshi(req.InvoiceRequest.ValueMsat +


Typically we just accept either sats _or msat and not a combination of them. Also you can use the helper function ToSatoshis() if you need to go back n forth.

Yeah, I check that only one of the two is set above. In order to avoid assigning to each one separately, I just add them as one of them at this point is 0. Tried to code-golf it

Roasbeef · 2025-03-21T19:00:18Z

rpcserver.go

+		if oracleRes.Err != nil {
+			return nil, fmt.Errorf("cannot query oracle: %v",
+				oracleRes.Err.Error())
+		}


Would be good to start to split up some of this logic into helper functions. Looks like we have the distinct phases of:

initial parsing/validation

creation of the receive/buy order

invoice creation

rpcserver.go

saraogiraj94 · 2025-03-24T02:56:14Z

rpcserver.go

+	// avoid ambiguous behavior. This field dictates the actual value of the
+	// invoice so let's be strict and only allow one possible value to be
+	// set.
+	if req.AssetAmount > 0 && (req.InvoiceRequest.ValueMsat > 0 ||


shouldn't the proto files be updated in the same Merge Request ? like adding of new keys ValueMsat

These fields were already included in the proto request, see here

They were just being ignored.

Thanks for the comment, realized I need to update the doc!

ZZiigguurraatt · 2025-03-25T13:01:20Z

Getting the following error when trying to build litd with this PR

$ make

===== skipping part about the web UI build ==========

 Building lightning-terminal.
go build -v -tags="litd autopilotrpc signrpc walletrpc chainrpc invoicesrpc watchtowerrpc neutrinorpc peersrpc" -ldflags " -X github.com./lightningnetwork/lnd/build.Commit=lightning-terminal-v0.14.1-alpha-82-gbad957bedc7f37950de33f5f15a3f00ed29e8f8d-dirty -X github.com./lightningnetwork/lnd/build.CommitHash=bad957bedc7f37950de33f5f15a3f00ed29e8f8d -X github.com./lightningnetwork/lnd/build.GoVersion= -X github.com./lightningnetwork/lnd/build.RawTags=litd,autopilotrpc,signrpc,walletrpc,chainrpc,invoicesrpc,watchtowerrpc,neutrinorpc,peersrpc -X github.com./lightninglabs/lightning-terminal.appFilesPrefix= -X github.com./lightninglabs/lightning-terminal.Commit=v0.14.1-alpha-82-gbad957bedc7f37950de33f5f15a3f00ed29e8f8d-dirty -X github.com./lightninglabs/loop.Commit=v0.29.0-beta -X github.com./lightninglabs/pool.Commit=v0.6.5-beta.0.20241015105339-044cb451b5df -X github.com./lightninglabs/taproot-assets.Commit=github.com." -o litd-debug github.com./lightninglabs/lightning-terminal/cmd/litd
go: updates to go.mod needed; to update it:
	go mod tidy
make: *** [Makefile:132: go-build] Error 1

then when I do go mod tidy as it says, I get

go: finding module for package github.com./lightningnetwork/lnd/graph/db/models
go: github.com./lightninglabs/lightning-terminal/cmd/litcli imports
	github.com./lightninglabs/taproot-assets/rfq imports
	github.com./lightningnetwork/lnd/graph/db/models: module github.com./lightningnetwork/lnd@latest found (v0.0.2), but does not contain package github.com./lightningnetwork/lnd/graph/db/models

I can build taproot-assets with this PR outside of litd though. I can also build litd with taproot-assets v0.5.1.

ZZiigguurraatt · 2025-03-25T13:13:16Z

OK, looks like I am having the same problem if I try to build litd with the taproot assets main branch tip too.

ZZiigguurraatt · 2025-03-25T13:23:35Z

@guggero Not sure if this issue is the cause?

https://github.com./lightninglabs/taproot-assets/blame/5290f9c3e811bb327a8f8a446d7b3519daf2b453/rfq/order.go#L16

guggero · 2025-03-25T14:18:48Z

@guggero Not sure if this issue is the cause?

https://github.com./lightninglabs/taproot-assets/blame/5290f9c3e811bb327a8f8a446d7b3519daf2b453/rfq/order.go#L16

This will be required first: lightninglabs/lightning-terminal#998

ZZiigguurraatt · 2025-03-25T14:27:19Z

@guggero Not sure if this issue is the cause?
https://github.com./lightninglabs/taproot-assets/blame/5290f9c3e811bb327a8f8a446d7b3519daf2b453/rfq/order.go#L16

This will be required first: lightninglabs/lightning-terminal#998

OK, so https://github.com./lightninglabs/taproot-assets/blame/5290f9c3e811bb327a8f8a446d7b3519daf2b453/rfq/order.go#L16 is not the cause, but instead, the fix for taproot-assets and other dependencies are basically missing that same fix?

guggero · 2025-03-25T14:28:52Z

The update to lnd 19 was already done in taproot-assets, which came with a bunch of changes. Those changes also need to be done in litd for things to be compile-time compatible. Which is what the PR I linked to does.

ZZiigguurraatt · 2025-03-25T15:09:51Z

OK, so no way to test this (and any other open taproot-assets) PR with lightning channels then until lightninglabs/lightning-terminal#998 is merged?

guggero · 2025-03-25T15:15:32Z

Correct. I'm working on it.

ZZiigguurraatt · 2025-03-27T16:25:02Z

Now that lightninglabs/lightning-terminal#998 is merged, I'm getting.

# github.com./lightninglabs/taproot-assets/tapchannel
../../taproot-assets/addinvoice-sats-amt/tapchannel/aux_funding_controller.go:2259:38: cannot use (*FundingController)(nil) (value of type *FundingController) as funding.AuxFundingController value in variable declaration: *FundingController does not implement funding.AuxFundingController (wrong type for method SendMessage)
		have SendMessage(msgmux.PeerMsg) bool
		want SendMessage("context".Context, msgmux.PeerMsg) bool
make: *** [Makefile:132: go-build] Error 1

However, I do have a cached version of branch lnd-19 at lightninglabs/lightning-terminal@453835f which does compile. So, I'm wondering if there was some change to taproot-assets that was required to merge the latest version of branch lnd-19 in lightning-terminal and addinvoice-sats-amt (this PR) needs to be rebased on that?

GeorgeTsagk · 2025-03-27T16:39:31Z

@ZZiigguurraatt this PR needs a rebase, will soon update it

GeorgeTsagk · 2025-03-28T07:49:59Z

Rebased on latest main

rpcserver.go

ZZiigguurraatt · 2025-04-07T14:59:36Z

rpcserver.go

+			amtMsat, *askAssetRate,
+		).ScaleTo(0).ToUint64()
+
+		// Now let's see if the negotiated quote can actually route the


are we actually checking if this quote can "route" the payment? seems like this has nothing to do with routing through channels, more if the RFQ is big enough to accept it? or do we still consider this part of "routing" because the HTLC will be rejected if it doesn't fit within the RFQ?

yeah what we check here is if the quote, along with the agreed upon max units, can evaluate to the amount we want to add to the invoice

we don't do any liquidity related check with channels, as that's out of the scope of this func

similarly to LND, when I call AddInvoice for 1BTC I don't want to get an error if at the time I don't have 1BTC of inbound

Yes, we have lightningnetwork/lnd#9380 that could add such checks for AddInvoice.

Just thinking the word "route" should not be used here so we don't confuse people to think we are actually checking if the payment could really be routed.

We add a simple helper to expose the configured price deviation ppm to be used by other systems. This is used in a following commit.

We move the validation around in preparation for the next commit that adds more logic. The diff will be smaller and focused on the important change thanks to this move.

The previously ignored value/value_msat fields are now properly being accounted for. We now accept only one value related field to be set.

Since we now support setting the existing fields value/value_msat we need to update the godoc in order to reflect the new behavior to the RPC user.

guggero · 2025-04-16T08:40:15Z

I pushed up a version that simplifies the commit structure, so we don't touch the same code multiple times. Should also make it easier to spot what exactly in the logic is changed (don't want to introduce a potential siphon or other kind of exploitable vulnerability here, so yes, I'm extra strict with reviewer friendliness of commit structure).

guggero · 2025-04-16T09:25:08Z

Litd test will succeed after merging lightninglabs/lightning-terminal#1040.

guggero · 2025-04-16T09:26:16Z

Also fixes #1306.

lightninglabs-deploy · 2025-04-23T09:51:36Z

@Roasbeef: review reminder
@GeorgeTsagk, remember to re-request review from reviewers when ready

Roasbeef · 2025-04-24T00:17:05Z

Seeing this failure in the litd itest;, not sure if related:

    assets_test.go:1428: Asking peer 03a024e1ee071b31d510d75a7794f979ed1131b1b021492f7613f82fda9530c84a for quote to buy assets to receive for invoice over 1 units; waiting up to 300s
    assets_test.go:1445: 
        	Error Trace:	/home/runner/work/taproot-assets/taproot-assets/lightning-terminal/itest/assets_test.go:1445
        	            				/home/runner/work/taproot-assets/taproot-assets/lightning-terminal/itest/litd_custom_channels_test.go:2163
        	            				/home/runner/work/taproot-assets/taproot-assets/lightning-terminal/itest/test_harness.go:103
        	            				/home/runner/work/taproot-assets/taproot-assets/lightning-terminal/itest/litd_test.go:87
        	Error:      	Error "rpc error: code = Unknown desc = error validating invoice amount: cannot create invoice for 1 asset units, as the minimal transportable amount is 20 units with the current rate of coefficient:\"5820600\" units/BTC" does not contain "cannot create invoice over 1 asset units, as the minimal transportable amount"
        	Test:       	TestLightningTerminal/custom_channels_liquidity
    harness.go:381: finished test: , start height=438, end height=483, mined blocks=45
    harness.go:403: !============================================!
        Too many blocks (45) mined in one test! Tips:
        1. break test into smaller individual tests, especially if this is a table-drive test.
        2. use smaller CSV via `--bitcoin.defaultremotedelay=1.`
        3. use smaller CLTV via `--bitcoin.timelockdelta=18.`
        4. remove unnecessary CloseChannel when test ends.
        5. use `CreateSimpleNetwork` for efficient channel creation.

Roasbeef · 2025-04-24T00:32:56Z

Ah I see this comment now , so does this just need a rebase now?

Roasbeef

LGTM 🥦

guggero · 2025-04-24T06:31:38Z

Litd itest is expected to fail now, because the referenced branch requires #1478 to be merged. So this will become stable again soon.

GeorgeTsagk self-assigned this Mar 21, 2025

ZZiigguurraatt reviewed Mar 21, 2025

View reviewed changes

rpcserver.go Outdated Show resolved Hide resolved

Roasbeef requested changes Mar 21, 2025

View reviewed changes

Roasbeef mentioned this pull request Mar 21, 2025

[feature]: generate asset invoice with fix amount of SATs instead of asset amount #1440

Closed

saraogiraj94 reviewed Mar 24, 2025

View reviewed changes

GeorgeTsagk force-pushed the addinvoice-sats-amt branch from 3dd15ef to 76212e2 Compare March 24, 2025 13:06

GeorgeTsagk requested review from Roasbeef and ZZiigguurraatt March 24, 2025 13:08

levmi added this to Taproot-Assets Project Board Mar 24, 2025

github-project-automation bot moved this to 🆕 New in Taproot-Assets Project Board Mar 24, 2025

levmi added ux P2 invoices payments labels Mar 24, 2025

levmi moved this from 🆕 New to 👀 In review in Taproot-Assets Project Board Mar 24, 2025

levmi added P1 and removed P2 labels Mar 24, 2025

GeorgeTsagk force-pushed the addinvoice-sats-amt branch from 76212e2 to 2614f53 Compare March 28, 2025 07:49

ZZiigguurraatt reviewed Apr 7, 2025

View reviewed changes

rpcserver.go Outdated Show resolved Hide resolved

ZZiigguurraatt reviewed Apr 7, 2025

View reviewed changes

rpcserver.go Outdated Show resolved Hide resolved

ZZiigguurraatt reviewed Apr 7, 2025

View reviewed changes

GeorgeTsagk force-pushed the addinvoice-sats-amt branch from e488107 to 47dd304 Compare April 14, 2025 12:06

GeorgeTsagk requested review from guggero and ZZiigguurraatt April 14, 2025 12:13

GeorgeTsagk added 7 commits April 16, 2025 10:29

rfq: manager exposed price deviation ppm

a9ac111

We add a simple helper to expose the configured price deviation ppm to be used by other systems. This is used in a following commit.

rpcserver: extract validateInvoiceAmount

5ccd981

rfqmath: add AddTolerance helper

4a6b5b6

rfq: add EstimateAssetUnits helper

387d50f

rpcserver: refactor validateInvoiceAmount

fce1fce

We move the validation around in preparation for the next commit that adds more logic. The diff will be smaller and focused on the important change thanks to this move.

rpcserver: calculate and validate invoice with sats amount

ce6a485

The previously ignored value/value_msat fields are now properly being accounted for. We now accept only one value related field to be set.

taprpc: update AddInvoice docs

cb13676

Since we now support setting the existing fields value/value_msat we need to update the godoc in order to reflect the new behavior to the RPC user.

guggero force-pushed the addinvoice-sats-amt branch from 47dd304 to cb13676 Compare April 16, 2025 08:38

guggero approved these changes Apr 16, 2025

View reviewed changes

guggero mentioned this pull request Apr 16, 2025

itest: fix wording "over" -> "for" lightninglabs/lightning-terminal#1040

Merged

guggero linked an issue Apr 16, 2025 that may be closed by this pull request

fix typo in error message #1306

Closed

GeorgeTsagk requested review from ffranr and removed request for ZZiigguurraatt April 22, 2025 19:10

Roasbeef approved these changes Apr 24, 2025

View reviewed changes

guggero merged commit 25ff294 into main Apr 24, 2025
17 of 18 checks passed

github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board Apr 24, 2025

guggero deleted the addinvoice-sats-amt branch April 24, 2025 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting sats/msats to `taprpc.AddInvoice` #1448

Allow setting sats/msats to `taprpc.AddInvoice` #1448

GeorgeTsagk commented Mar 21, 2025 •

edited by Roasbeef

Loading

coveralls commented Mar 21, 2025 •

edited

Loading

Roasbeef left a comment

Roasbeef Mar 21, 2025

GeorgeTsagk Mar 24, 2025

Roasbeef Mar 21, 2025

saraogiraj94 Mar 24, 2025

GeorgeTsagk Mar 24, 2025

ZZiigguurraatt commented Mar 25, 2025

ZZiigguurraatt commented Mar 25, 2025

ZZiigguurraatt commented Mar 25, 2025

guggero commented Mar 25, 2025

ZZiigguurraatt commented Mar 25, 2025

guggero commented Mar 25, 2025

ZZiigguurraatt commented Mar 25, 2025

guggero commented Mar 25, 2025

ZZiigguurraatt commented Mar 27, 2025 •

edited

Loading

GeorgeTsagk commented Mar 27, 2025

GeorgeTsagk commented Mar 28, 2025

ZZiigguurraatt Apr 7, 2025

GeorgeTsagk Apr 14, 2025

ZZiigguurraatt Apr 14, 2025

guggero commented Apr 16, 2025

guggero commented Apr 16, 2025

guggero commented Apr 16, 2025

lightninglabs-deploy commented Apr 23, 2025

Roasbeef commented Apr 24, 2025

Roasbeef commented Apr 24, 2025

Roasbeef left a comment

guggero commented Apr 24, 2025

Allow setting sats/msats to taprpc.AddInvoice #1448

Allow setting sats/msats to taprpc.AddInvoice #1448

Conversation

GeorgeTsagk commented Mar 21, 2025 • edited by Roasbeef Loading

Description

coveralls commented Mar 21, 2025 • edited Loading

Pull Request Test Coverage Report for Build 14488405809

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Roasbeef 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

ZZiigguurraatt commented Mar 25, 2025

ZZiigguurraatt commented Mar 25, 2025

ZZiigguurraatt commented Mar 25, 2025

guggero commented Mar 25, 2025

ZZiigguurraatt commented Mar 25, 2025

guggero commented Mar 25, 2025

ZZiigguurraatt commented Mar 25, 2025

guggero commented Mar 25, 2025

ZZiigguurraatt commented Mar 27, 2025 • edited Loading

GeorgeTsagk commented Mar 27, 2025

GeorgeTsagk commented Mar 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Apr 16, 2025

guggero commented Apr 16, 2025

guggero commented Apr 16, 2025

lightninglabs-deploy commented Apr 23, 2025

Roasbeef commented Apr 24, 2025

Roasbeef commented Apr 24, 2025

Roasbeef left a comment

Choose a reason for hiding this comment

guggero commented Apr 24, 2025

Allow setting sats/msats to `taprpc.AddInvoice` #1448

Allow setting sats/msats to `taprpc.AddInvoice` #1448

GeorgeTsagk commented Mar 21, 2025 •

edited by Roasbeef

Loading

coveralls commented Mar 21, 2025 •

edited

Loading

ZZiigguurraatt commented Mar 27, 2025 •

edited

Loading