Offers: send invoice request #87

orbitalturtle · 2024-01-02T01:20:20Z

Rewrote the send invoice logic with #85 in mind. I agree it seems cleaner :) I realize #85 wasn't intended for merge, but I didn't need to make many significant changes to the structure, so I just made a few tweaks to the commits here.

Also about to write a few thoughts/questions inline.

Replaces #81.

src/lndk_offers.rs

src/lib.rs

orbitalturtle · 2024-01-23T20:16:27Z

It looks like the clippy error is coming from configure_me.

carlaKC · 2024-01-29T20:39:11Z

I realize #85 wasn't intended for merge, but I didn't need to make many significant changes to the structure, so I just made a few tweaks to the commits here.

Happy for you to add those as fixups to the original commits + squash and you can just take it over 👍 ie, we merge what's in #85 in here

README.md

src/lndk_offers.rs

src/lib.rs

src/onion_messenger.rs

carlaKC · 2024-01-29T21:09:38Z

src/lib.rs

+// MessengerState tells us whether our onion messenger is still starting up is ready to start
+// forwarding messages.
+#[derive(Debug)]
+pub enum MessengerState {


Rather than add a whole state machine, can we have a one-shot channel and close it when we're done with startup? Also comes with the benefit that we can select on shutdown / channel close and then we don't have to have any X second / magic number wait -> it'll either exit if we've given up or yield exactly when the onion messenger has started up.

Yeah that sounds good. I changed the MessengerState to a channel, though I couldn't quite get a oneshot channel to work because it requires a "self" method receiver, which doesn't work for us. So I used a normal mpsc channel instead.

src/lib.rs

carlaKC

Major comments about "readynesss" channel and checking up on whether we can always rely on having an advertised IP (not sure that we can).

src/lib.rs

carlaKC · 2024-02-02T18:22:15Z

src/lib.rs

+pub struct LndkOnionMessenger {
+    pub offer_handler: OfferHandler,


I don't follow the logic of an onion messenger containing an offer handler?
To me the reverse makes more sense: we have an offer handler, and one of the things that it managed is an onion messenger?

I guess to me it seems like LndkOnionMessenger encompasses any sort of activity dealing with onion messaging, and then offer_handling is a more specific application of onion messaging that we support. And perhaps in the future we might add other applications of onion messaging that might eventually branch off the LndkOnionMessenger struct

I guess to me it seems like LndkOnionMessenger encompasses any sort of activity dealing with onion messaging,

That ordering doesn't make sense to me. Sure we may have multiple things using onion messaging one day, but ideally that would just be an abstracted messaging layer that each application interacts with. Maybe I'm missing something, but this layout is analogous to having a top level HTTP struct which has multiple websites inside of it?

Especially with the way that LDK has structured the API for the onion messenger (handlers for different functionality), I think it's far cleaner to have each piece of functionality standalone and referring to the onion messenger. Say we add asynchronous payments, I think that approach 2 here makes way more sense for a modular codebase:

Approach 1:

LndkOnionMessenger{ OfferHandler, AsyncHandler, }

vs

Approach 2:

OfferHandler{ OnionMessenger } AsyncHandler{ OnionMessenger }

src/lndk_offers.rs

src/lib.rs

src/lndk_offers.rs

src/lib.rs

carlaKC

I'm not fully on board with the architecture chosen here, but I don't want to unnecessarily block on a difference of opinion.

Would be in support of:

Merging the first few commits that have been unchanged for a while
Merging as is (pending small fixups suggested in here) once we have E2E testing paying an offer working and then coming back to refactor

carlaKC · 2024-02-08T20:56:47Z

src/lndk_offers.rs

+    /// otherwise we creae a blinded path directly to ourselves.
+    pub async fn create_reply_path(
+        &self,
+        mut connector: impl PeerConnector + std::marker::Send + 'static,


Doesn't need to be 'static?

without it I get error[E0310]: the parameter type impl MessageSigner + std::marker::Send may not live long enough

And the parameter type impl MessageSigner + std::marker::Send must be valid for the static lifetime...

carlaKC · 2024-02-08T20:59:16Z

src/lndk_offers.rs

+        // Wait for onion messenger to give us the signal that it's ready. Once the onion messenger drops
+        // the channel sender, recv will return None and we'll stop blocking here.
+        while (started.recv().await).is_some() {
+            println!("Error: we shouldn't receive any messages on this channel");


Error rather than println?

Do we need a loop here? Can't we just recv().await and then proceed?

if started.recv().await.is_some(){ return Err(unexpected signal) } ... continue with logic here

Ah yup good catch, I think that was leftover from when I had put a RefCell in there

orbitalturtle · 2024-02-08T22:54:49Z

@carlaKC Sounds good, I like option 2. I can see what you're saying above about modularity and I'd be down to switch to your approach. But I like the idea of merging it as is (once we have e2e working, etc), then going back to refactor. Going back and interactive rebasing that big of a change sounds like a royal pain and I'd love to avoid it if possible, ha ha.

To allow external callers to shutdown lndk, we provide a shutdown trigger and listener to the onion messenger, replacing the single use channels that we previously used. This is useful for itests, so that we can cleanly shut down and for callers of the library to manage execution.

Provides the end user with something to interact with when they want to pay offers.

For our integration tests to work properly we need to use a custom version of lnd's RPC that allows signing the tagged hash (BIP 340) of a message. This was introduced in LND at commit dacb86f.

For the integration tests to work we need to add the walletrpc subservice to the lnd make process so we can use the DeriveKey RPC call.

Bumps rust-lightning to a custom version that allows us to sign the invoice request's tagged hash.

With Rust 1.75.0, we get an unused import error from configure_me, which we can't change on our end. We ignore this warning so the CI runs properly.

Before we can send an invoice request, LNDK's onion messenger needs to be fully started and running. We use a channel close to signal when the messenger is ready.

We bump ldk-sample to a version that can open channels and broadcasts the node announcement at a more frequent interval so we can pull ldk's address from the node graph in our integration tests.

carlaKC

Thanks for the quick rebase!

orbitalturtle requested a review from carlaKC January 2, 2024 01:20

orbitalturtle commented Jan 2, 2024

View reviewed changes

src/lndk_offers.rs Show resolved Hide resolved

orbitalturtle force-pushed the send-invoice-req branch from 18e33d6 to 6aa2814 Compare January 2, 2024 01:24

orbitalturtle commented Jan 2, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

orbitalturtle commented Jan 2, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

orbitalturtle force-pushed the send-invoice-req branch from 6aa2814 to a1f47ef Compare January 2, 2024 02:21

orbitalturtle force-pushed the send-invoice-req branch from a1f47ef to 9af927d Compare January 20, 2024 07:34

orbitalturtle changed the title ~~offers: send invoice request~~ Offers: send invoice request Jan 23, 2024

orbitalturtle mentioned this pull request Jan 29, 2024

Cli: add pay offer command #91

Merged

carlaKC reviewed Jan 29, 2024

View reviewed changes

orbitalturtle force-pushed the send-invoice-req branch 10 times, most recently from d932363 to d64e534 Compare February 2, 2024 07:40

carlaKC reviewed Feb 2, 2024

View reviewed changes

orbitalturtle force-pushed the send-invoice-req branch 5 times, most recently from cb6764b to e14f2b4 Compare February 8, 2024 01:34

carlaKC reviewed Feb 8, 2024

View reviewed changes

This was referenced Mar 1, 2024

Offers: invoice verification #89

Merged

readme: update branch instructions #95

Merged

carlaKC and others added 24 commits March 6, 2024 15:17

multi: refactor to create OfferHandler and LndkOnionMessenger

b104164

Provides the end user with something to interact with when they want to pay offers.

lib: implement and use OfferMessageHandler on OfferHandler

8241ca1

lib: refactor create_invoice_request to be a method of OfferHandler

277e53e

offers: rename create_invoice_request

6f4ae68

itests: update lnd submodule to tagged hash change

9bfed00

For our integration tests to work properly we need to use a custom version of lnd's RPC that allows signing the tagged hash (BIP 340) of a message. This was introduced in LND at commit dacb86f.

itests: add walletrpc subserver to lnd Makefile/README

8b00be5

For the integration tests to work we need to add the walletrpc subservice to the lnd make process so we can use the DeriveKey RPC call.

Pin rust-lightning to a custom version

53640aa

Bumps rust-lightning to a custom version that allows us to sign the invoice request's tagged hash.

Add tokio clock default to satisfy clippy

3f9982f

utils: add Default for MessengerUtilities to satisfy clippy

26b039f

main: ignore unused imports from configure_me

fbd5ae1

With Rust 1.75.0, we get an unused import error from configure_me, which we can't change on our end. We ignore this warning so the CI runs properly.

tests: specify log level in ldk nodes

6de424f

README: Fix bakemacaroon typo

be673e6

Fix Makefile test typo

a469d71

Export MessengerUtilities for integration tests

8263b9e

offers: add logic for connecting to the introduction node peer

780d0fb

offers: validate offer amount user input

ac957e4

offers: wait for onion messenger ready signal before sending request

f97bd3d

Before we can send an invoice request, LNDK's onion messenger needs to be fully started and running. We use a channel close to signal when the messenger is ready.

offers: Build a reply path for invoice request

3b09ec2

itests: add lnd API calls needed to set up channels

df50a92

itests: export bitcoind for tests

491e42e

itests: bump ldk-sample to newer version

8bfb006

We bump ldk-sample to a version that can open channels and broadcasts the node announcement at a more frequent interval so we can pull ldk's address from the node graph in our integration tests.

itests: advertise ldk node address

8af07f4

offers: send invoice request

b85cad4

orbitalturtle force-pushed the send-invoice-req branch from d4694ef to b85cad4 Compare March 6, 2024 21:23

carlaKC approved these changes Mar 7, 2024

View reviewed changes

carlaKC merged commit 276c676 into master Mar 7, 2024
4 checks passed

orbitalturtle deleted the send-invoice-req branch May 30, 2024 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offers: send invoice request #87

Offers: send invoice request #87

orbitalturtle commented Jan 2, 2024 •

edited

orbitalturtle commented Jan 23, 2024

carlaKC commented Jan 29, 2024

carlaKC Jan 29, 2024

orbitalturtle Jan 31, 2024

carlaKC left a comment

carlaKC Feb 2, 2024

orbitalturtle Feb 6, 2024 •

edited

carlaKC Feb 8, 2024

carlaKC left a comment

carlaKC Feb 8, 2024

orbitalturtle Feb 24, 2024

carlaKC Feb 8, 2024

carlaKC Feb 8, 2024

orbitalturtle Feb 24, 2024

orbitalturtle commented Feb 8, 2024

carlaKC left a comment

		pub struct LndkOnionMessenger {
		pub offer_handler: OfferHandler,

Offers: send invoice request #87

Offers: send invoice request #87

Conversation

orbitalturtle commented Jan 2, 2024 • edited

orbitalturtle commented Jan 23, 2024

carlaKC commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbitalturtle Feb 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC 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

orbitalturtle commented Feb 8, 2024

carlaKC left a comment

Choose a reason for hiding this comment

orbitalturtle commented Jan 2, 2024 •

edited

orbitalturtle Feb 6, 2024 •

edited