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

switch to derived NUMS #89

Closed
wants to merge 3 commits into from

Conversation

MCJOHN974
Copy link

@MCJOHN974 MCJOHN974 commented Jan 23, 2025

Description

Currently we use NUMS internal key for taproot reclaim transactions. Unfortunately, Ledger don't allow users to sign trunsactions with not-deriven internal key, and Ledger users can't do reclaim. This PR switches from NUMS to key, derived from NUMS with path "0/0"

Checklist

  • Code is commented where needed
  • Unit test coverage for new or modified code paths
  • npm run test passes
  • Changelog is updated
  • Performed self-review on my code

@CLAassistant
Copy link

CLAassistant commented Jan 23, 2025

CLA assistant check
All committers have signed the CLA.

@MCJOHN974
Copy link
Author

@aldur will it be enough instead of writing derivation code here just put a comment with link to rust derivation code in sbtc repo. Like, if somebody don't trust us they can just see at rust code and even run it by themselves

@aldur aldur self-requested a review January 23, 2025 08:57
@@ -16,6 +16,13 @@ export const NUMS_X_COORDINATE = new Uint8Array([
0xee, 0x9a, 0xce, 0x80, 0x3a, 0xc0,
]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still used anywhere?

Copy link
Author

Choose a reason for hiding this comment

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

I'm just waiting if we want add derivation code here as well, or just a comment with link to sbtc derivation code. If we pick second option this should be deleted

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's OK to delete it

Copy link
Author

Choose a reason for hiding this comment

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

@aldur, WDYT about this? I personally would like just to left a comment, extra code will add unnecessary complexety while if somebody wants to check if this key is really derived from NUMS they can just execute derivation code by themselves

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I did comment it's OK to delete it?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, I deleted it already in 1a928e3. Sorry for extra ping, probably my PR page didn't refreshed automatically and I forgot to do it manually

Comment on lines 19 to 24
export const DERIVED_NUMS_X_COORDINATE = new Uint8Array([
0x4a ,0x30 ,0xb2 ,0xe4 ,0x61 ,0xb2 ,0x80, 0xc0,
0xb1, 0x3a, 0x03, 0x79, 0x90, 0x96, 0xab, 0x12,
0x56, 0x58, 0x91, 0x53, 0xfc, 0x4b, 0x9c, 0x8c,
0xea, 0xd1, 0x6d, 0xc0, 0xb6, 0x42, 0x06, 0x97,
]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix formatting

@aldur
Copy link
Collaborator

aldur commented Jan 23, 2025

Note for ourselves: this will need to be merged after stacks-network/sbtc#1255

@aldur
Copy link
Collaborator

aldur commented Jan 30, 2025

Closing this (since we can't switch to a derived pub-key for the signer aggregate key).

@aldur aldur closed this Jan 30, 2025
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.

3 participants