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

[feature] support reclaim from Ledger #1255

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

MCJOHN974
Copy link
Collaborator

Description

Closes: #1253

Changes

Testing Information

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@MCJOHN974 MCJOHN974 changed the title [feature] support reclaim from ledger [feature] support reclaim from Ledger Jan 22, 2025
@MCJOHN974
Copy link
Collaborator Author

MCJOHN974 commented Jan 22, 2025

From my understanding this adds all we need on sbtc side. I'd maybe also add some explanations why derived key is also unspendable, but didn't find a good link for it yet

upd: After some research (basically asking Vlad) I think this explanations don't needed and reason that derived key is unspendable is same that the reason original key is unspendable -- we publicly show the way we generated this pubkey

@MCJOHN974 MCJOHN974 marked this pull request as ready for review January 22, 2025 16:12
@MCJOHN974 MCJOHN974 requested review from djordon and aldur January 22, 2025 16:12
Copy link
Collaborator

@djordon djordon 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.

One suggestion is to put the logic for how we derive the key in the LazyLock's function definition. The derivation is fast and only happens once. We would still want a test that the output bytes equal what we expect them to be.

Also, after doing some digging, it seems like this change is indeed required. Ledger requires a wallet policy all the time, and so we need to prove that we know the derivation path. I'd still like to hear it from a Ledger engineer if possible (in a reasonable amount of time).

Comment on lines +58 to +70
LazyLock::new(|| XOnlyPublicKey::from_slice(&DERIVED_NUMS_X_COORDINATE).unwrap());

#[cfg(test)]
mod tests {
use super::*;
use bitcoin::bip32::{ChainCode, ChildNumber, DerivationPath, Fingerprint, Xpub};
use secp256k1::{PublicKey, Secp256k1};
use std::str::FromStr;

/// This test proves, that the DERIVED_NUMS_X_COORDINATE is correct,
/// and is derived from the NUMS_X_COORDINATE using the derivation path "0/0".
#[test]
fn test_derivation() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

One alternative is to define the UNSPENDABLE_TAPROOT_KEY using the same logic of this test.

@MCJOHN974
Copy link
Collaborator Author

@djordon if I understand correctly you suggesting to move derivation logic from test to LazyLock (and still have test comparing results of derivation to hardcoded constant).

I think this two options are +- equal and I don't prefer much one to another. But I see a small benefit from option I initially chose: we move derivation logic out of main code making it simpler and easier to read. I think in most cases when you are looking through the sbtc code you are not very interested how we choose the internal key, and when you interested in it, you will look at the test

What benefits you think we will get moving derivation logic from tests to main code?

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.

[Feature]: Support a Ledger friendly NUMS internal key for deposits
2 participants