Add support for sending to human-readable names (BIP 353) #528
Conversation
…2 Offers BIP 353 introduced human-readable names which has been implemented in LDK (0.1). This commit adds support for sending to HRNs that resolve to Bolt12 Offers by using the LDK pay_for_offer_from_human_readable_name method in ChannelManager.
|
👋 Thanks for assigning @tnull as a reviewer! |
| pub fn send_to_human_readable_name( | ||
| &self, name: &str, amount_msat: u64, dns_resolvers: Vec<Destination>, | ||
| ) -> Result<PaymentId, Error> { |
There was a problem hiding this comment.
I should probably expose this method to uniffi bindings?
Also, for dns_resolvers maybe I should add a method in graph.rs which helps find all nodes that have a certain feature flag. This could make it simpler for the user to find all dns_resolver nodes.
There was a problem hiding this comment.
I should probably expose this method to uniffi bindings?
Yes, you'll need to expose this method and all types it requires.
Also, for dns_resolvers maybe I should add a method in graph.rs which helps find all nodes that have a certain feature flag. This could make it simpler for the user to find all dns_resolver nodes.
Yes, populating the list of default Destinations (see above) from the network graph is probably a good idea.
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
| /// [HumanReadableName]: crate::lightning::onion_message::dns_resolution::HumanReadableName | ||
| /// [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki | ||
| /// [bLIP 32]: https://github.com/lightning/blips/blob/master/blip-0032.md | ||
| HrnBolt12Offer { |
There was a problem hiding this comment.
Hmm, I'm not sure we want to add a separate PaymentKind variant for this. Likely it should just be a Bolt12Offer? In the upcoming 'payment metadata store' (see #425), we might then store the HRN as associated metadata though.
| /// These nodes can be found by running a search through the `NetworkGraph` to find nodes that announce the | ||
| /// `dns_resolver` feature flag. | ||
| pub fn send_to_human_readable_name( | ||
| &self, name: &str, amount_msat: u64, dns_resolvers: Vec<Destination>, |
There was a problem hiding this comment.
I think rather then replicating LDK's API and have the user give the Destination for each send, in LDK Node we should probably allow users to set default DNS resolvers via the Builder and/or the Config. Here, we'd then error-out if the user didn't configure them upfront, and in the future we might be able to supply some default values.
| pub fn send_to_human_readable_name( | ||
| &self, name: &str, amount_msat: u64, dns_resolvers: Vec<Destination>, | ||
| ) -> Result<PaymentId, Error> { |
There was a problem hiding this comment.
I should probably expose this method to uniffi bindings?
Yes, you'll need to expose this method and all types it requires.
Also, for dns_resolvers maybe I should add a method in graph.rs which helps find all nodes that have a certain feature flag. This could make it simpler for the user to find all dns_resolver nodes.
Yes, populating the list of default Destinations (see above) from the network graph is probably a good idea.
| @@ -429,6 +430,18 @@ impl UniffiCustomTypeConverter for DateTime { | |||
| } | |||
| } | |||
|
|
|||
| impl UniffiCustomTypeConverter for HumanReadableName { | |||
There was a problem hiding this comment.
Let's avoid introducing yet another UniffiCustomTypeConverter for this, as we're actually currently working towards removing them for Bolt11Invoice, Offer, etc.
So we should just expose the HumanReadableName as a Uniffi interface, and if we find its API can't fully exposed, we'll need to create a type wrapper in uniffi_types.rs for it that is switched based on the uniffi flag (see #522 for reference).
|
Thank you for the feedback @tnull. I will address the comments and push the updated changes as soon as possible. |
|
Had a quick look at why some of the CI tests could be failing. I think the cln test is potentially failing because of the EDIT: Nevermind, I think this is the CLN issue: #527 The test seems to be flaky. |
Still actively working on this. Will push updates once I'm done (likely within the next week). |
This PR is a first step to add support for human-readable names as outlined in BIP 353 and implemented in LDK 0.1.
Refs #435
Changes
send_to_human_readable_namefunction was added underBolt12Payment.HrnBolt12OfferPaymentKind variant was added.HrnParsingFailedError variant was added.I plan to follow up with a PR for #436 and another PR that will add HRN sending for on-chain payments.