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

Add OfferManager #624

Merged
merged 9 commits into from
Apr 25, 2024
Merged

Add OfferManager #624

merged 9 commits into from
Apr 25, 2024

Conversation

thomash-acinq
Copy link
Member

Add OfferManager that handles offer related onion messages (which is all the onion messages that we support).
It can request an invoice and pay it, answer to invoice requests.

Depends on #621 and #623

@thomash-acinq thomash-acinq requested review from dpad85 and t-bast April 3, 2024 08:26
@dpad85
Copy link
Member

dpad85 commented Apr 3, 2024

Assuming this PR stays in review for a while, and since it's a large change, we could use a temporary specific version name to differentiate this offer branch from the master one (they currently both use the same 1.6.2-SNAPSHOT version). It would make it easier to test on Phoenix's end.

@thomash-acinq thomash-acinq force-pushed the offer-manager branch 2 times, most recently from edbf21e to ed10fe2 Compare April 17, 2024 16:52
@thomash-acinq thomash-acinq marked this pull request as ready for review April 17, 2024 16:52
@t-bast
Copy link
Member

t-bast commented Apr 22, 2024

Can you rebase this? It will make it easier to test with the latest Bolt 12 changes available on master.

@thomash-acinq thomash-acinq force-pushed the offer-manager branch 2 times, most recently from daf6a19 to 4c856bb Compare April 22, 2024 17:02
override val paymentHash: ByteVector32 = paymentRequest.paymentHash
}

/** KeySend payments are spontaneous donations that don't need an invoice from the recipient. */
data class KeySend(val preimage: ByteVector32) : Details() {
Copy link
Member Author

Choose a reason for hiding this comment

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

KeySend doesn't seem to be supported, should we just remove all references to it?

Copy link
Member

Choose a reason for hiding this comment

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

True, we defined the types initially to ensure that the architecture allowed different types of lightning payments, but we don't support it. You can remove all references to it now that we have the Blinded case that shows a different type of lightning payment!

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, I iterated in #633 to add some improvements.

src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt Outdated Show resolved Hide resolved
src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt Outdated Show resolved Hide resolved
@t-bast
Copy link
Member

t-bast commented Apr 25, 2024

After converging on #633, we should rebase to include #626 and register that default offer:

private val offerManager = run {
  val (pathId, offer) nodeParams.defaultOffer(walletParams.trampolineNode)
  val manager = OfferManager(nodeParams, walletParams, _eventsFlow, logger)
  manager.registerOffer(offer, pathId)
  manager
}

And we should be done with the changes to lightning-kmp!

thomash-acinq and others added 9 commits April 25, 2024 16:26
We always use the invoice's `node_id`, so instead of duplicating this
argument we can extract it automatically from the invoice. It avoids
mistakes that could mess up our accounting.
And rename top-level `SendMessage` to `SendOnionMessage`.
We may want to display a spinner in the UI while requesting the invoice
for an offer payment, to quickly report back when the recipient cannot
be contacted. This means we need to fail as soon as the timeout provided
is hit, so we cannot rely on a regular background task.

We instead create a dedicated coroutine for each payment, with the
timeout provided in the `PayOffer` request.
This commit keeps the business logic mostly unchanged, but re-works the
error handling to ensure that we log as much details as possible to help
troubleshoot compatibility issues.

It adds support for sending `invoice_error` on some error cases to help
debug potential issues.

When the introduction node of the recipient is our trampoline node, we
remove an unnecessary hop in the onion message.
@thomash-acinq
Copy link
Member Author

I've merged your refactoring, implicit node id and default offer.

@thomash-acinq thomash-acinq merged commit 5547972 into master Apr 25, 2024
2 checks passed
@thomash-acinq thomash-acinq deleted the offer-manager branch April 25, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants