Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #246 +/- ##
======================================
Coverage 6.88% 6.88%
======================================
Files 1 1
Lines 218 218
======================================
Hits 15 15
Misses 203 203 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
590c184 to
a24ff2b
Compare
Add support for resolving human-readable names (e.g., satoshi@example.com) to BOLT12 offers via DNSSEC-validated DNS lookups. The PayOfferRequest now accepts an optional `name` field that the server resolves to an offer string before processing payment.
Add dedicated error types for human-readable name parsing, DNS resolution failures, and invalid payment URI formats to improve error diagnostics.
01250f1 to
b3fd9c1
Compare
a-mpch
left a comment
There was a problem hiding this comment.
solid start, let me know about the test related comments
tests/integration_tests.rs
Outdated
| resolver: R, | ||
| ) { | ||
| if let Ok(mut guard) = lndk::dns_resolver::TEST_RESOLVER.write() { | ||
| *guard = Some(std::sync::Arc::new(resolver)); |
There was a problem hiding this comment.
This looks good, would it be easier using mockall crate that we use elsewhere?
There was a problem hiding this comment.
I think because pay_name calls directly to LndkDNSResolverMessageHandler, you can't inject a mock here without the global static. To use easily mockall we would have to pass the dns_resolver as param to pay_name
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn test_pay_offer_with_name_and_dns() { |
There was a problem hiding this comment.
Thanks for trying out actually doing the Server and then calling the client. We currently are using the server itself to trigger the messenger actions.
I'd stick to the same pattern (no so strongly, I'm happy to discuss it).
Anyway, I think we could have a different route to pay human_readable and not an option. In the future we could a have a "Pay" that just pay whatever it's sees.
Then we can test the PayHumanReadable flow not changing the gRPC of PayOffer. We could even have a "fetch_offer" in order to test that flow only.
Finally on the mocking side you can use the LDK node Offer to be resolved to the function so we can have a payment after the resolution
There was a problem hiding this comment.
To start I added a PayName endpoint
Add integration test validating end-to-end payment using human-readable names. Uses mock DNS resolver to test server-side name resolution and payment flow.
a-mpch
left a comment
There was a problem hiding this comment.
Looks good! Haven't tested It yet.
Left some minor comments and we should be good to go
Extract BIP-353 human-readable name payment into dedicated PayName RPC endpoint, separate from PayOffer. This improves API clarity by distinguishing direct offer payments from name-based payments that require DNS resolution.
|
Sorry the delay, I'll review it ASAP |
|
|
||
| let resolution = self | ||
| .resolver | ||
| .resolve_hrn(&hrn_parsed) |
There was a problem hiding this comment.
No, now I added
src/offers/handler.rs
Outdated
|
|
||
| /// Resolves a human-readable name (BIP-353) to an offer and pays it. | ||
| pub async fn pay_hrn(&self, cfg: PayHumanReadableAddressParams) -> Result<Payment, OfferError> { | ||
| let offer_str = LndkDNSResolverMessageHandler::new() |
There was a problem hiding this comment.
Maybe we could return an error if offer is not on the same network lndk is
There was a problem hiding this comment.
If the node that makes the resolution can't reach the node of the offer it will throw the respective error when trying to pay the offer. I think it's better to keep it like that
f3r10
left a comment
There was a problem hiding this comment.
As indicated, I used nacho@nachoporte.space and the resolution was successful.
I tried to use other names, such: send.some@satsto.me or matt@mattcorallo.com, but the resolution fails.
Replaces global TEST_RESOLVER static with proper dependency injection via a new with_dns_resolver() constructor. This makes the code more testable and eliminates potential race conditions from shared mutable state.
Adds 20-second timeout to HTTP client used for DNS resolution to prevent requests from hanging indefinitely on unresponsive servers. Replaces manual URI query parsing with url crate for proper handling of percent-encoded characters (%20, %3D, etc.) and robust parsing of BIP-21 bitcoin URIs.
I’ve pushed a draft to validate the approach, later I will split it into clean commits and align it with our standards.
Per BIP-353, my understanding is we don’t need to run a resolver ourselves. We can send a DNSSECQuery over BLIP-32 to an external resolver node, wait for the response, and then locally verify the returned DNSSEC proof.
BIP-353 flow (short):
User enters a name (e.g., alice.example).
Build and send DNSSECQuery (via BLIP-32 to resolver peers, or self-resolve).
Receive DNSSECProof over onion messages.
Locally validate DNSSEC (required by BIP-353).
Extract URI from TXT (typically lightning:).
Extract offer and pay it
Ref: DNSResolverMessageHandler::handle_dnssec_query — https://docs.rs/lightning/latest/lightning/onion_message/dns_resolution/trait.DNSResolverMessageHandler.html#tymethod.handle_dnssec_query
You need a valid DNS name with a TXT record following BIP-353 to fully test the flow. You can use nacho@nachoporte.space to partially test: it has a valid BIP-353 TXT record, so resolution will succeed, but the payment attempt will fail because there’s no route to the node.
EDIT:
As disscused with @a-mpch the first implementation will be done with self resolve and then we will implement the blip-32