Add coin purchase memo support#1785
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 514a062418
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
24a5edf to
ed19d60
Compare
|
Added address derivation field (only eth for now). |
Nice! How about adding validation of it to |
b666068 to
1438ce7
Compare
|
To validate the eth keypath I made |
d87fe65 to
3d9dd9e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d9dd9e9e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // TODO: add CoinPurchaseMemo arm when SwapKit UI is finalized | ||
| _ => return Err(Error::InvalidInput), |
There was a problem hiding this comment.
Accept CoinPurchaseMemo in payment request UI
The new memo type is still funneled into the default rejection path, so any payment request containing coin_purchase_memo is rejected during user_verify() before validate() runs in the BTC signing flow. This makes the added coin-purchase path unusable in practice (including builds with app-ethereum), because the transaction is aborted as invalid input instead of proceeding with the new verification logic.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| #[cfg(not(feature = "app-ethereum"))] | ||
| return Err(ValidationError::Other); |
There was a problem hiding this comment.
Maybe add ::Disabled and map it to Error::Disabled in signtx.rs?
| @@ -23,6 +23,19 @@ use pb::eth_response::Response; | |||
|
|
|||
| use core::convert::TryInto; | |||
|
|
|||
| pub(crate) fn derive_address( | |||
f91a5e4 to
d3da007
Compare
Ready to review again |
| if !keypath::is_valid_keypath_address(keypath) { | ||
| return Err(Error::InvalidInput); | ||
| } | ||
| let pubkey = crate::keystore::get_xpub_twice(hal, keypath) |
There was a problem hiding this comment.
Mabye not worth optimizing now, but to verify the address in a payment request, deriving the xpub once is enough, as a derivation error (bit flip) will be caught and not have any bad consequence.
| @@ -94,6 +94,10 @@ pub async fn user_verify( | |||
| pub enum ValidationError { | |||
| UnknownRecipient, | |||
| InvalidSignature, | |||
| #[cfg(feature = "app-ethereum")] | |||
There was a problem hiding this comment.
Is there a compiler warning/error without this guard here and below? You can leave it, but imho it's fine to also not guard, as it will change whenever a new coin is supported in the coinpurchase memo.
There was a problem hiding this comment.
CI complains because when app-ethereum is disabled and AddressMismatch is never constructed.
d3da007 to
1074610
Compare
|
Squashed, no change. |
Gate CoinPurchase handling behind a new app-swap feature to limit to BitBox Multi.
One could decide to not gate behind a feature (also including it in BTC only version) since this adds negligible binary size, introduces no new attack surface (receiving a
CoinPurchaseMemorequires a valid signature from a known identity), and the protobuf type cannot be cfg-gated anyway since it is generated code.I chose the strict way and gated behind a new feature, but it may be discussed.