Common BIP353 address parser #1149
Conversation
… one hickory-resolver adn exported from the sdk_common
…d to accept the dns_resolver from outside
libs/sdk-common/src/input_parser.rs
Outdated
| @@ -183,8 +191,21 @@ use crate::prelude::*; | |||
| pub async fn parse( | |||
| input: &str, | |||
| external_input_parsers: Option<&[ExternalInputParser]>, | |||
| dns_resolver: Option<&AsyncResolver<GenericConnector<TokioRuntimeProvider>>>, | |||
There was a problem hiding this comment.
Do you see a need to provide a "custom" dns resolver to parse?
The interface would be leaner and it would be easier for the caller if it didn't need to provide one. We can create a new one on every call.
There was a problem hiding this comment.
I agree with you. I think that I misunderstood this comment to the liquid PR (https://github.com/breez/breez-sdk-liquid/pull/619/files/51cf0e48605d5df88f26a3b12a21e2332c89e607)
There was a problem hiding this comment.
Not sure about re-creating it on every call, since it's something that we expect to be frequently evoked from the user's side.
I'm not against instantiating the resolver internally altogether though, you could try taking a look at the lazy_static crate and starting it there.
libs/sdk-common/src/input_parser.rs
Outdated
| @@ -183,8 +191,21 @@ use crate::prelude::*; | |||
| pub async fn parse( | |||
| input: &str, | |||
| external_input_parsers: Option<&[ExternalInputParser]>, | |||
| dns_resolver: Option<&AsyncResolver<GenericConnector<TokioRuntimeProvider>>>, | |||
There was a problem hiding this comment.
Any reason to have this as an optional parameter? It won't be exposed to the bindings anyway so why not use our own inside the function and simplify the interface?
There was a problem hiding this comment.
I agree with you. I think that I misunderstood this comment to the liquid PR (https://github.com/breez/breez-sdk-liquid/pull/619/files/51cf0e48605d5df88f26a3b12a21e2332c89e607)
| @@ -186,6 +195,19 @@ pub async fn parse( | |||
| ) -> Result<InputType> { | |||
| let input = input.trim(); | |||
|
|
|||
| let mut dns_resolvers_opts = ResolverOpts::default(); | |||
| dns_resolvers_opts.validate = true; | |||
There was a problem hiding this comment.
I'm getting a line in the log:
validate option is only available with 'dnssec' feature
Tip:
Add the following to cargo.toml:
hickory-proto = { version = "0.24.2", features = ["dnssec"] }
hickory-resolver = { version = "0.24.2", features = ["dnssec"] }
However, when I try that on some BIP-353 domains I get: proto error: self-signed dnskey is invalid
Perhaps this is useful? https://github.com/TheBlueMatt/dnssec-prover
| return None; | ||
| } | ||
|
|
||
| if let Some((_, bolt12_address)) = |
There was a problem hiding this comment.
I would suggest to return the full BIP21 string here. And have the rest of the input parser handle the resulting string. For example, I found this string in the wild: bitcoin:?lno=lno1zrxq8pjw7qjlm68mtp7e3yvxee4y5xrgjhhyf2fxhlphpckrvevh50u0qffqdtuhyd77jpju7prz0qvkcm0xfzfpz844z9dwkaexuapm3e66vqsrxkhw43wz8wfa349nyl2wcsdp2u62ykz4lzuke37p75f4x286v6eqqva25lyzapjs7h424f7v65hv3k539558aqdztn24tjtkyqrrnr3f4vxwxhz0hhrkmzt4mz228sgs3tjl2vzhq0y3s4g8c65nvl35gyy2r833d6qaf64e2qwkey8zkm2q4yln60xgkqqsz8u6nrn984hru24yunnk0p093c&sp=sp1qqvwqe7yvuk9hwy6keczg2h0xajp6zeqzamzpcpfm0nrpgjr42yyq2q6hzkuldhpj5frduwjlzndf26rk9atzt49qwufun7drfw3c8yxypvk57ux5, combining an offer and a silent payments address.
There was a problem hiding this comment.
The problem I see here is that this doesn't look like a valid bip21 as it misses the address part (before the query params).
There was a problem hiding this comment.
@roeierez The address field may not be mandatory and some parsers support it, so it should be alright
Edit: Seems like it's not supported in our parser, I think that should be addressed as other packages do seem to work (e.g. NodeJS bip21)
There was a problem hiding this comment.
@roeierez The mandatory address is a disputed property. I think it shouldn't be necessary to enforce that, especially as a client.
There was a problem hiding this comment.
I agree, just stated that our current bip21 parser (the crate we use) doesn't seem to support that.
feat: I implemented within the sdk-common the function to parse a BIP353 address and extract the corresponding bolt12 or lnurl-pay offer. To achieve that I needed a dns-resolver and I decided to use hickory-resolver crate.
feat: edited also the cli code to integrate and inject from outside the dns_resolver dependency
This PR is the continuing part of this one breez/breez-sdk-liquid#619 in the breez-sdk-liquid repo.
I tried to implement the advice, but for the decoding part of the TXT data I decided at the moment to keep things separate from the parse_core, because of the time required to better analyze how messages are processed within the parse core.