Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: add asset hodl invoice support #1184

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Nov 9, 2024

In this commit, we add hodl invoice support when creating invoice for TAP channels. The invoice creation proceeds as normal, but a special HodlInvoice field is set which contains a payment hash which must be set.

When invoices are created using this endpoint. They MUST be settled using the SettleInvoice RPC on the invoicesrpc server in lnd.

@Roasbeef Roasbeef requested review from a team, jharveyb and GeorgeTsagk and removed request for a team November 9, 2024 00:47
@Roasbeef Roasbeef marked this pull request as ready for review November 9, 2024 00:47
@coveralls
Copy link

coveralls commented Nov 9, 2024

Pull Request Test Coverage Report for Build 11789368763

Details

  • 0 of 51 (0.0%) changed or added relevant lines in 1 file are covered.
  • 27 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.05%) to 40.613%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcserver.go 0 51 0.0%
Files with Coverage Reduction New Missed Lines %
tappsbt/create.go 2 53.22%
tapdb/multiverse.go 2 60.95%
internal/test/helpers.go 2 86.85%
commitment/tap.go 2 84.43%
asset/asset.go 2 80.02%
tapgarden/caretaker.go 8 68.5%
universe/interface.go 9 47.09%
Totals Coverage Status
Change from base Build 11782733093: -0.05%
Covered Lines: 24684
Relevant Lines: 60779

💛 - Coveralls

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter isn't happy about line length and one question about a flag, otherwise LGTM 🎉

rpcserver.go Outdated Show resolved Hide resolved
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

rpcserver.go Outdated Show resolved Hide resolved
In this commit, we add hodl invoice support when creating invoice for
TAP channels. The invoice creation proceeds as normal, but a special
HodlInvoice field is set which contains a payment hash which must be
set.

When invoices are created using this endpoint. They MUST be settled
using the SettleInvoice RPC on the invoicesrpc server in lnd.
@Roasbeef Roasbeef enabled auto-merge November 12, 2024 02:14
@Roasbeef Roasbeef added this pull request to the merge queue Nov 12, 2024
Merged via the queue into lightninglabs:main with commit 882e6db Nov 12, 2024
18 checks passed
@ZZiigguurraatt
Copy link

why call it hodl invoice whereas everywhere in LND documentation hold invoice is used?

@ZZiigguurraatt
Copy link

Why do you have a separate HodlInvoice (https://lightning.engineering/api-docs/api/taproot-assets/taproot-asset-channels/add-invoice#tapchannelrpchodlinvoice) object with just a payment_hash in it? Do you expect to add more fields? LND's AddHoldInvoice (https://lightning.engineering/api-docs/api/lnd/invoices/add-hold-invoice) does not have this.

Also, AddHoldInvoice in LND uses hash instead of payment_hash. For new users, it is confusing if there is a difference. I think they should have common names everywhere. I think payment_hash is better to use than just hash.

@ZZiigguurraatt
Copy link

Also, at https://lightning.engineering/api-docs/api/taproot-assets/taproot-asset-channels/add-invoice/index.html#tapchannelrpcaddinvoicerequest it says to use an invoice_request generated from an Invoice (https://lightning.engineering/api-docs/api/lnd/lightning/add-invoice/index.html#lnrpcinvoice). Do we use this one for a HOLD invoice too, or an AddHoldInvoiceRequest (https://lightning.engineering/api-docs/api/lnd/invoices/add-hold-invoice#invoicesrpcaddholdinvoicerequest)? Note: AddHoldInvoiceRequest has the hash field, so that would be defined twice if it were to be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants