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

Common BIP353 address parser #1149

Merged
merged 18 commits into from
Jan 6, 2025
Merged

Common BIP353 address parser #1149

merged 18 commits into from
Jan 6, 2025

Conversation

lorenzoronzani
Copy link
Contributor

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.

libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Thank you @lorenzoronzani for the PR

libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Looks good.

libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@danielgranhao danielgranhao left a comment

Choose a reason for hiding this comment

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

Sorry for nitpicking 😄

Also, please take into account the failed clippy check. There's some unnecessary returns that it complains about.

libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/lib.rs Outdated Show resolved Hide resolved
libs/sdk-core/src/lib.rs Outdated Show resolved Hide resolved
libs/sdk-common/src/input_parser.rs Outdated Show resolved Hide resolved
@lorenzoronzani
Copy link
Contributor Author

Oh no, thanks for nitpicking, I'm here to improve my skills and having someone to give me advice is a nice pro 😀.

Copy link
Contributor

@danielgranhao danielgranhao left a comment

Choose a reason for hiding this comment

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

LGTM - we should just keep in mind BOLT12 offers behind BIP353 addresses will only be successfully parsed when we move BOLT12 parsing into the sdk-common.

Copy link
Member

@hydra-yse hydra-yse left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

LGTM

@danielgranhao danielgranhao merged commit b81a5eb into breez:main Jan 6, 2025
9 checks passed
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.

6 participants