-
Notifications
You must be signed in to change notification settings - Fork 118
feat(wallet): implement HD multi-address support for message signing #2432
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
Changes from 6 commits
c5ccb30
7c61845
529f2a6
8a42b0e
b23b8fc
d323812
ad7e0b5
032faff
6d4332e
9e9081e
187284d
b10fd57
5fcef99
791c64b
8489bb0
67266b1
ca176d8
38eb5dd
b453126
e7add30
ecc6eb7
98a5b6c
df2bbed
935409d
9f0bde4
8180da3
4d91373
860c208
3ade61b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2084,7 +2084,7 @@ pub trait MarketCoinOps { | |
|
|
||
| fn sign_message_hash(&self, _message: &str) -> Option<[u8; 32]>; | ||
|
|
||
| fn sign_message(&self, _message: &str) -> SignatureResult<String>; | ||
| fn sign_message(&self, _message: &str, _derivation_path: Option<DerivationPath>) -> SignatureResult<String>; | ||
|
|
||
| fn verify_message(&self, _signature: &str, _message: &str, _address: &str) -> VerificationResult<bool>; | ||
|
|
||
|
|
@@ -2296,9 +2296,17 @@ pub enum ValidatorsInfoDetails { | |
| } | ||
|
|
||
| #[derive(Serialize, Deserialize)] | ||
| pub struct SignatureRequest { | ||
| coin: String, | ||
| message: String, | ||
| #[serde(tag = "derivation_method", rename_all = "snake_case")] | ||
| pub enum SignatureRequest { | ||
| Iguana { | ||
| coin: String, | ||
| message: String, | ||
| }, | ||
| HdWallet { | ||
| coin: String, | ||
| message: String, | ||
| derivation_path: RpcDerivationPath, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is part of the derivation path which should be static per coin (and defined in the coins file).
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might disagree with this. KDF already returns full derivation path for every addresses which If I see a better reason not to require the full
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we wanna keep it like this, we at least wanna perform a check (e.g.) to make sure we are not signing from a private key that isn't standard to this coin. people make mistakes.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done 032faff |
||
| }, | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does it make sense to tell the caller to state whether they want the signing with iguana or HD wallet given that KDF was initialized exclusively with either iguana or HD wallet? i mean, if i initialized KDF with what im saying is, the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. valid point 👍🏾 done 032faff |
||
|
|
||
| #[derive(Serialize, Deserialize)] | ||
|
|
@@ -5112,8 +5120,17 @@ pub async fn get_raw_transaction(ctx: MmArc, req: RawTransactionRequest) -> RawT | |
| } | ||
|
|
||
| pub async fn sign_message(ctx: MmArc, req: SignatureRequest) -> SignatureResult<SignatureResponse> { | ||
| let coin = lp_coinfind_or_err(&ctx, &req.coin).await?; | ||
| let signature = coin.sign_message(&req.message)?; | ||
| let (coin, message, derivation_path) = match &req { | ||
| SignatureRequest::Iguana { coin, message } => (coin, message, None), | ||
| SignatureRequest::HdWallet { | ||
| coin, | ||
| message, | ||
| derivation_path, | ||
| } => (coin, message, Some(derivation_path.0.clone())), | ||
| }; | ||
| let coin = lp_coinfind_or_err(&ctx, coin).await?; | ||
| let signature = coin.sign_message(message, derivation_path)?; | ||
|
|
||
| Ok(SignatureResponse { signature }) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ enable-sia = ["coins/enable-sia", "coins_activation/enable-sia"] | |
| sepolia-maker-swap-v2-tests = [] | ||
| sepolia-taker-swap-v2-tests = [] | ||
| test-ext-api = ["trading_api/test-ext-api"] | ||
| new-db-arch = [] # A temporary feature to integrate the new db architecture incrementally | ||
| new-db-arch = ["mm2_core/new-db-arch"] # A temporary feature to integrate the new db architecture incrementally | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this change is up #2398. you could omit it here if u want (to avoid conflicts) |
||
|
|
||
| [dependencies] | ||
| async-std = { version = "1.5", features = ["unstable"] } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.