Skip to content

AddInvoice correctly handles existing route hints #1627

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

GeorgeTsagk
Copy link
Member

Description

After merging #1359 we changed some of the logic around acquiring quotes in taprpc.AddInvoice. A special edge case where the user has already negotiated a quote and included it in the lnrpc.Invoice route hints would lead into a crash as a variable would remain unset.

Closes #1622

@coveralls
Copy link

Pull Request Test Coverage Report for Build 15973315571

Details

  • 0 of 48 (0.0%) changed or added relevant lines in 2 files are covered.
  • 29 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.04%) to 39.247%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapchannel/aux_invoice_manager.go 0 17 0.0%
rpcserver.go 0 31 0.0%
Files with Coverage Reduction New Missed Lines %
rpcserver.go 1 0.0%
tappsbt/create.go 2 26.74%
commitment/tap.go 4 71.82%
tapfreighter/interface.go 4 48.0%
asset/asset.go 5 46.62%
asset/mock.go 6 65.45%
tapgarden/caretaker.go 7 67.91%
Totals Coverage Status
Change from base Build 15930981047: 0.04%
Covered Lines: 30643
Relevant Lines: 78077

💛 - Coveralls

@levmi levmi added bug Something isn't working invoices rfq labels Jun 30, 2025
@levmi levmi moved this from 🆕 New to 🏗 In progress in Taproot-Assets Project Board Jun 30, 2025
@levmi levmi added this to the v0.6.1 milestone Jun 30, 2025
@levmi levmi requested review from guggero and ffranr July 3, 2025 15:10
@levmi levmi moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Jul 3, 2025
Comment on lines +431 to +445
for _, hint := range hints {
for _, h := range hint.HopHints {
scid := h.ChanId
buyQuote, ok := buyQuotes[rfqmsg.SerialisedScid(scid)]
if !ok {
continue
}

quoteSpecifier := buyQuote.Request.AssetSpecifier

if quoteSpecifier.String() == specifier.String() {
return &buyQuote
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check that there are no other quotes here? So exhaustively check: find the quote, but then keep looking and error if we find another matching quote.

Copy link
Member Author

@GeorgeTsagk GeorgeTsagk Jul 4, 2025

Choose a reason for hiding this comment

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

It's not really crucial to not have competing quotes here, as we're just picking one to create the invoice over.

For the case where the user wants to provide multiple quotes for a multi-rfq receive we're going to introduce the RPC level rfq_ids field, which would also eliminate the need to manually create a []RouteHints argument

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invoices rfq
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

[bug]: Runtime error when validating asset invoice amount
4 participants