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

client: Added code that implements the lnurl lud06 spec. #226

Merged
merged 5 commits into from
Aug 21, 2023

Conversation

Randy808
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Wow arti is huge :-) Slowly getting concerned about the binary size that we have to ship to various bindings. As promised I went through it with a really fine-toothed comb, so there are a lot of comments, but feel free to ignore them.

The mix of async and blocking is a bit strange, and some operations, such as initializing Tor circuits, could be deferred a bit so we don't pay the costs even though we don't use it later.

libs/gl-client/Cargo.toml Outdated Show resolved Hide resolved
libs/gl-client/src/lnurl/mod.rs Outdated Show resolved Hide resolved
libs/gl-client/src/node/mod.rs Outdated Show resolved Hide resolved
libs/gl-client/src/lnurl/tor_http_client.rs Outdated Show resolved Hide resolved
address_filter.build()?;
config_builder.build()?;

let tor_client = TorClient::create_bootstrapped(config_builder.build().unwrap()).await?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let tor_client = TorClient::create_bootstrapped(config_builder.build().unwrap()).await?;
let tor_client = TorClient::create_bootstrapped(config_builder.build()?).await?;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Notice too that create_bootstrapped is actually going out and establishing Tor circuits, which can take considerable time. If we then end up not using that circuit all of that work is wasted. It'd likely be a better idea to keep the builder around, and defer bootstrapping to when we know we need the circuit.

libs/gl-client/src/lnurl/pay/models.rs Outdated Show resolved Hide resolved
libs/gl-client/src/lnurl/pay/models.rs Outdated Show resolved Hide resolved
libs/gl-client/src/lnurl/pay/models.rs Outdated Show resolved Hide resolved
libs/gl-client/src/lnurl/pay/models.rs Outdated Show resolved Hide resolved
libs/gl-client/src/lnurl/pay/mod.rs Outdated Show resolved Hide resolved
@cdecker cdecker marked this pull request as draft August 3, 2023 10:08
@cdecker cdecker changed the title [Draft] Added code that implements the lnurl lud06 spec. client: Added code that implements the lnurl lud06 spec. Aug 3, 2023
@Randy808 Randy808 requested a review from cdecker August 7, 2023 14:59
cdecker
cdecker previously approved these changes Aug 7, 2023
Copy link
Collaborator

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Great cleanup @Randy808 👍

libs/gl-client/src/lnurl/mod.rs Outdated Show resolved Hide resolved
libs/gl-client/src/lnurl/pay/mod.rs Outdated Show resolved Hide resolved
libs/gl-client/src/lnurl/pay/mod.rs Outdated Show resolved Hide resolved
Comment on lines +136 to +141
fn convert_to_async_return_value<T: Send + 'static>(
value: T,
) -> Pin<Box<dyn std::future::Future<Output = T> + Send>> {
let ready_future: Ready<_> = future::ready(value);
Pin::new(Box::new(ready_future)) as Pin<Box<dyn std::future::Future<Output = T> + Send>>
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very clever, definitely something I need to remember 👍

libs/gl-client/Cargo.toml Outdated Show resolved Hide resolved
@Randy808 Randy808 force-pushed the lnurl-lud06 branch 3 times, most recently from 41fbf1d to 4ce559f Compare August 10, 2023 10:24
@Randy808 Randy808 marked this pull request as ready for review August 14, 2023 12:59
@Randy808 Randy808 force-pushed the lnurl-lud06 branch 3 times, most recently from 6838ad2 to e631b19 Compare August 17, 2023 15:01
@Randy808
Copy link
Collaborator Author

The build is failing after rebasing. I'm still looking into what it could be

Copy link
Collaborator

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Adding rustls-tls-native-roots to the reqwest features frees us from the openssl-dev dependency

log = "^0.4"
pin-project = "*"
prost = "0.11"
reqwest = {version="0.11.18", features=["json"]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is implicitly pulling in openssl-sys which depends on the openssl headers. We already use rustls through tonic, so we likely want to use the rustls-tls-native-roots feature.

… methods needed to manually resolve an lnurl to an invoice.
@@ -679,7 +679,6 @@ mod tests {
signer_state: vec![],
requests: Vec::new(),
},
vec![],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removing this line since it's preventing the tests from building.

The method signature change was causing it to fail:
https://github.com/Blockstream/greenlight/pull/227/files#diff-8d5abc718d7a05d6432918246fcf5f9919681ca385b057f72a2117e9878e1932R284

@cdecker cdecker merged commit d3998d5 into Blockstream:main Aug 21, 2023
17 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.

2 participants