-
Notifications
You must be signed in to change notification settings - Fork 4
Implement parsing & resolving lnurls #2
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
Conversation
3acd077
to
a8644fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, found some comments I had still pending :(
src/lib.rs
Outdated
@@ -944,6 +947,38 @@ impl PaymentInstructions { | |||
)) | |||
}, | |||
} | |||
} else if let Some((_, data)) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this supposed to support decoding any part of the string that contains lnurl*
rather than only if the full string is lnurl*
? I saw a checkout not long ago that used a QR code to display, eg https://a.website/to-visit?lnurl=lnurlasdfwerqwer
so that users who just scan the QR code go to their website but if a wallet scans it it will resolve the lnurl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just learned thats a thing, will add
https://github.com/lnurl/luds/blob/luds/01.md#fallback-scheme
@@ -111,10 +111,14 @@ pub trait HrnResolver { | |||
/// can be further parsed as payment instructions. | |||
fn resolve_hrn<'a>(&'a self, hrn: &'a HumanReadableName) -> HrnResolutionFuture<'a>; | |||
|
|||
/// Resolves the given Lnurl into a [`HrnResolution`] containing a result which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An Lnurl
isn't a well-defined thing, I believe? This needs to specify what, exactly, the url
parameter is (a URL for an LNURL endpoint, and not the encoded LNURL...... strings).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An lnurl is just a bech32 encoded url https://github.com/lnurl/luds/blob/luds/01.md
Everything works off of calling the first first URL as a GET request and then seeing which kind it is and handling it from there. This function is that first GET request
@@ -111,10 +111,14 @@ pub trait HrnResolver { | |||
/// can be further parsed as payment instructions. | |||
fn resolve_hrn<'a>(&'a self, hrn: &'a HumanReadableName) -> HrnResolutionFuture<'a>; | |||
|
|||
/// Resolves the given Lnurl into a [`HrnResolution`] containing a result which | |||
/// can be further parsed as payment instructions. | |||
fn resolve_lnurl<'a>(&'a self, url: &'a str) -> HrnResolutionFuture<'a>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename the trait now that its resolving more than HRNs? I think I'm fine with no, because we really want this to be about HRNs, we just happen to support LNURL too, but still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I figured it was simpler to keep the name. At this point basically every lnurl-pay is a lightning address, for most cases this is just splitting out the 2 lnurl calls into separate functions instead of a single one
Added support for the fallback lnurl stuff and added tests for the lnurl parsing |
src/dns_resolver.rs
Outdated
let err = "resolve_lnurl shouldn't be called when we don't reoslve LNURL"; | ||
fn resolve_lnurl<'a>(&'a self, _url: &'a str) -> HrnResolutionFuture<'a> { | ||
let err = "resolve_lnurl shouldn't be called when we don't resolve LNURL"; | ||
debug_assert!(false, "{}", err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still gets called, it just returns a failure. and that's fine, we shouldn't panic :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah was just keeping what it was before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is wrong, too, though. The previous resolve_lnurl
/now resolve_lnurl_to_invoice
method is not called by the code in lib.rs
(and shouldn't be, per the API), but this code is called. Instead, the error message should be around not supporting LNURL, and resolve_lnurl_to_invoice
should presumably retain its debug_assert
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah i see now, good call out
@@ -944,6 +947,47 @@ impl PaymentInstructions { | |||
)) | |||
}, | |||
} | |||
} else if let Some(idx) = instructions.to_lowercase().rfind("lnurl") { | |||
let lnurl_str = &instructions[idx..]; | |||
if let Ok((_, data)) = bitcoin::bech32::decode(lnurl_str) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to be from lnurl
to the end of the string, or can it be a substring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it so it'll handle relevant sub strings
282f844
to
0562fac
Compare
src/lib.rs
Outdated
// first try to decode as a bech32-encoded lnurl, if that fails, try to drop a | ||
// trailing `&` and decode again, this could a http query param | ||
let decode = bitcoin::bech32::decode(lnurl_str).or_else(|e| { | ||
if let Some(idx) = lnurl_str.find('&') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we'll also want to split by #
. Might as well just do this always rather than trying to decode twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, when would #
be used though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URLs can be https://example.com?lightning=LNURL#section
, which jumps to the HTML object section
on load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna go ahead and land this but will swap the lowercase call in a followup.
} else if let Some(idx) = instructions.to_lowercase().rfind("lnurl") { | ||
let mut lnurl_str = &instructions[idx..]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that unicode lowercasing will always keep the char the same length. I assume that's probably true in practice, but its a weird assumption to take. Instead, using to_ascii_lowercase
only assumes that for ASCII chars (which is trivially true).
No description provided.