Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
benthecarman
force-pushed
the
lnurl-auth
branch
3 times, most recently
from
655c1a2 to
b4b2f09
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks! Looks good I think, one comment.
There was a problem hiding this comment.
We probably also want to expose Node::lnurl_auth in bindings here?
benthecarman
force-pushed
the
lnurl-auth
branch
3 times, most recently
from
9392684 to
713af6e
Compare
|
Thanks for this. I also tested this with https://stacker.news and https://satsback.com, and I was authenticated successfully on both. |
|
We decided to make the |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
@benthecarman Do you still intend to finish this? Would need a rebase by now.
src/lib.rs
Outdated
| scorer: Arc<Mutex<Scorer>>, | ||
| peer_store: Arc<PeerStore<Arc<Logger>>>, | ||
| payment_store: Arc<PaymentStore>, | ||
| lnurl_auth: LnurlAuth, |
There was a problem hiding this comment.
Let's just hold an Arc, as cloning that is at least cheaper than always cloning the whole struct below?
src/lib.rs
Outdated
| /// [LNURL-auth]: https://github.com/lnurl/luds/blob/luds/04.md | ||
| pub fn lnurl_auth(&self, lnurl: String) -> Result<(), Error> { | ||
| let auth = self.lnurl_auth.clone(); | ||
| self.runtime.block_on(async move { auth.authenticate(&lnurl).await }) |
There was a problem hiding this comment.
We def. need to add a tokio::timeout to make sure we're not indefinitely keeping a blocking task around. Could be added here, or below around the actual calls to remote that could hang indefinitely if something goes wrong. Maybe here at the top level might even be safer.
benthecarman
force-pushed
the
lnurl-auth
branch
2 times, most recently
from
454560e to
d51d85e
Compare
|
Sorry forgot about this PR. Rebased and fixed for comments |
| log_debug!(self.logger, "Starting LNURL-auth process for URL: {lnurl_auth_url}"); | ||
|
|
||
| // Parse the URL to extract domain and parameters | ||
| let url = reqwest::Url::parse(&lnurl_auth_url).map_err(|e| { |
There was a problem hiding this comment.
We're about to drop our reqwest dependency soon. Can we avoid leaning on reqwest or Url and rather do parsing and some validation manually?
There was a problem hiding this comment.
Actually, let's hold on that for a day or two. I now started a new crate for URL parsing with minimal dependencies so we don't have to reimplement the same logic over and over again. Will let you know once that's in a usable state.
There was a problem hiding this comment.
addressed other comments, will leave this for now
benthecarman
force-pushed
the
lnurl-auth
branch
from
d51d85e to
ca9813f
Compare
Implements LNURL-auth (LUD-04) specification for secure, privacy-preserving authentication with Lightning services using domain-specific key derivation. I used LUD-13 for deriving the keys as this is what most wallets use today.
benthecarman
force-pushed
the
lnurl-auth
branch
from
ca9813f to
6a002d3
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Seems cargo test is currently broken:
error[E0308]: mismatched types
--> src/lnurl_auth.rs:173:18
|
173 | LnurlAuth::new(hashing_key, logger)
| -------------- ^^^^^^^^^^^ expected `&WalletKeysManager`, found `SecretKey`
| |
| arguments to this function are incorrect
|
note: associated function defined here
--> src/lnurl_auth.rs:43:16
|
43 | pub(crate) fn new(keys_manager: &KeysManager, logger: Arc<Logger>) -> Self {
| ^^^ --------------------------
4 participants
Implements LNURL-auth (LUD-04) specification for secure, privacy-preserving authentication with Lightning services using domain-specific key derivation.
I used LUD-13 for deriving the keys as this is what most wallets use today.
I was able to auth with stacker.news with this so it works!