-
Notifications
You must be signed in to change notification settings - Fork 111
feat(wallet-connect): impl BTC (UTxO) activation via WalletConnect #2499
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
Conversation
trustwallet is the one having limited methods support and doesn't support eth_signTransaction. Metamask supports nearly all methods
not relevant for now till KDF could be used as a wallet
…terpart might be fallible we assumed it's infallible yet we used try_from in eth_coin_from_conf_and_request :/ anyway, we want this to be fallible since we will later add WalletConnect to PrivKeyBuildPolicy and the conversion in this case might not always succeed
even tho this struct is intertwined with eth and tendermint code. it's actually never utilized there :/
having these weird one func trait made a messy trace all over. coins who don't even support hw wallet had to implement the trait for hw wallet builder and not use it.
not btc, but rather any utxo that has a caip bip122 chain_id and is supported by the wallet side
|
@mariocynicys please don't forget to fix fmt and clippy errors |
onur-ozkan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nits from my end.
I will give a final look once conflicts are resolved.
and shorten its doc comment
and fix some conflicts
| // Get and parse the chain_id of the coin. | ||
| let chain_id = builder.conf()["chain_id"].as_str().ok_or_else(|| { | ||
| WalletConnectError::InvalidChainId(format!( | ||
| "coin={} doesn't have chain_id (bip122 standard) set in coin config which is required for WalletConnect", | ||
| builder.ticker() | ||
| )) | ||
| })?; | ||
| let chain_id = WcChainId::try_from_str(chain_id).map_mm_err()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chain_id is being prepared way too early as used much later after many fallible calls. This makes its preparation potentially wasteful. I would move it closer to where it's actually used (right above get_walletconnect_address call).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chain_id is also a fallible call.
p.s. and it's not a heavy computation or something.
| async fn build_utxo_fields_with_walletconnect<Builder>( | ||
| builder: &Builder, | ||
| session_topic: &str, | ||
| ) -> UtxoCoinBuildResult<UtxoCoinFields> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 questions.
- Can we have a stricter type on session topic? Right now this function can be called with any value that isn't related with wallet connect at all.
- Can you split this into some smaller functions? It's nearly impossible to add test coverage on its current form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. will put some code in class methods.
for the string thing, there is a Topic struct provided by walletconnect-rust. it's a weird arc thing, but might make sense here in this situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2- 9bf308a addresses the second point.
a side note regarding testing: we really don't have a way to test these walletconnect connections programmatically as of now. we need a walletconnect wallet client run within our test suite for us to accomplish this.
we currently rely on already implemented mobile wallets and demo impls to test our implementation.
addressing point 1 next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…activation we used to convert it using `priv_key_policy.into()` and then do a check to rule out the bad activation policies not accepted by legacy activation. changing it now to just do the conversion inline and reject bad activation policies from the beginning. this way we don't accidently add a new activation policty but forget to rule it out in legacy activation
shamardy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments. Please merge with dev to fix conflicts.
mm2src/coins/eth/v2_activation.rs
Outdated
| let chain_id = chain_spec | ||
| .ok_or(EthActivationV2Error::ChainIdNotSet)? | ||
| .chain_id() | ||
| .ok_or(EthActivationV2Error::ChainIdNotSet)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we only use it for wallet connect activation, got it, it's a bit confusing though. Will resolve the 2 comments above it for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think we should instead handle it like the below
https://github.com/KomodoPlatform/komodo-defi-framework/blob/35d73537cd5d1e7b102ae6a87fc73cb6c3cb1a3d/mm2src/coins/eth/v2_activation.rs#L699-L703
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updating to the latest toolchain
|
upd: the problem seem to be bifrost only. bifrosts signature seem to hard code the recovery id to |
per the spec, the siganture should be in hex format. we fallback to base64 though since bifrost wrongfully uses base64 for signature encoding. ref. https://docs.reown.com/advanced/multichain/rpc-reference/bitcoin-rpc\#returns-4
shamardy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge with dev again for latest toolchain, also the comment below is for next PRs. After fixing the chain_id thing, we can merge this.
| address: &str, | ||
| sign_message_prefix: &str, | ||
| ) -> MmResult<String, WalletConnectError> { | ||
| const AUTH_MSG: &str = "Authenticate with KDF"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both UTXO and ETH (and other coins of course), there are some formats that such message should follow (e.g. https://github.com/ethereum/ercs/blob/master/ERCS/erc-4361.md) and for UTXO it can be something like the below
Komodo DeFi Framework requests your signature. (or komodo wallet, the app should be specified in the activation request)
Purpose: This signature is required to securely derive the public key for your trading address. This is not a transaction and will not spend any funds.
Address: bc1q...
Issued At: 2025-07-30T10:00:00Z
Expires At: 2025-07-30T10:05:00Z
Session: wc:a286b...
Please research this more to see if there is a standard or we should create one / follow the ethereum one.
this is consistent with eth
a569a60 to
33868a6
Compare
and resolve a todo that was blocked on this PR
|
lots of failing tests @mariocynicys please fix them |
looks like serde won't regonize an object that has no protocol_data when the enum content is inlined as struct representation in the enum. to curcumvent this without forcing us to use protocol_data everywhere, we let the enum hold its content in a tuple representation and let that tuple hold a struct that has the needed content.
the optional tagging should be at the enum level. this is how it masks protocol_data to make it optional as well. structurally, this doesn't look right. but it keeps backward compatability with coins file and only coins that add chain_id need to play with protocol_data field.
mm2src/coins/lp_coins.rs
Outdated
| #[display(fmt = "No such coin {coin}")] | ||
| NoSuchCoin { coin: String }, | ||
| #[display(fmt = "Invalid hash: {_0}")] | ||
| InvalidHashError(String), | ||
| #[from_stringify("web3::Error")] | ||
| #[display(fmt = "Transport error: {_0}")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should start adding this to coins configs and docs @mariocynicys please open an issue in coins repo and in docs repo. c.c. @cipig @smk762
* dev: (21 commits) feat(wallet-connect): impl BTC (UTxO) activation via WalletConnect (#2499) feat(utxo): add new fixed txfee option for DINGO-like coins (#2454) ci(pull-requests): review notification bot (#2468) improvement(walletconnect): return the `pairing_topic` in `new_connection` response (#2538) bless clippy (#2560) refactor(toolchain): use latest available stable compiler (#2557) feat(wallet): implement unified offline private key export API (#2542) improve note for docker test start failure (#2550) fix(DOCS): add note for macos to fix docker containers startup failure (#2544) refactor(toolchain): general stabilization for stable rust (#2528) fix(ci): adds nodejs 20 to ci-container (#2536) fix(WASM and Debian): fix build failures (#2534) improvement(event-streaming): impl DeriveStreamerId trait for all streamers (#2489) fix(eth): Propagate structured EIP-1559 fee errors (#2532) fix(eth): Correctly implement ETH max withdrawal logic (#2531) feat(use-clap-for-cli): use clap to parse CLI-Args #2215 (#2510) feat(orderbook): expirable maker orders (#2516) improvement(eth): drop parity support (#2527) chore(release): finalize changelog for v2.5.0-beta (#2524) chore(toolchain): upgrade toolchain to nightly 1.86.0 (#2444) ... # Conflicts: # mm2src/coins/lp_coins.rs # mm2src/coins/rpc_command/get_new_address.rs # mm2src/trezor/src/eth/eth_command.rs
* dev: feat(wallet-connect): impl BTC (UTxO) activation via WalletConnect (#2499) feat(utxo): add new fixed txfee option for DINGO-like coins (#2454) ci(pull-requests): review notification bot (#2468) improvement(walletconnect): return the `pairing_topic` in `new_connection` response (#2538) bless clippy (#2560) refactor(toolchain): use latest available stable compiler (#2557) feat(wallet): implement unified offline private key export API (#2542) improve note for docker test start failure (#2550) fix(DOCS): add note for macos to fix docker containers startup failure (#2544) refactor(toolchain): general stabilization for stable rust (#2528) fix(ci): adds nodejs 20 to ci-container (#2536) fix(WASM and Debian): fix build failures (#2534) improvement(event-streaming): impl DeriveStreamerId trait for all streamers (#2489) chore(release): v2.3.0-beta (#2284) # Conflicts: # mm2src/coins/eth.rs # mm2src/coins/eth/eth_swap_v2/eth_maker_swap_v2.rs # mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs # mm2src/coins/eth/eth_tests.rs # mm2src/coins/eth/eth_withdraw.rs # mm2src/coins/eth/v2_activation.rs # mm2src/coins/nft.rs # mm2src/coins/qrc20.rs # mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs # mm2src/mm2_main/src/rpc/lp_commands/one_inch/rpcs.rs # mm2src/mm2_main/src/rpc/lp_commands/tokens.rs # mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs
* dev: (24 commits) fix(ordermatch): ignore loop-back; clear on null root; reject stale keep-alives (#2580) fix(clippy): fix clippy warnings for #2565 (#2589) fix(Trezor): fix utxo and eth calls due to firmware changes (#2565) fix(utxo): calculate min_trading_vol based on fixed tx fees (#2564) feat(protocol): [0] solana support (#2586) fix(utxo): fix header deserialization; guard AuxPoW (#2583) chore(rust 1.89): make CI clippy/fmt pass (wasm32, all-targets) (#2581) fix(utxo): deserialize sapling root for PIVX block headers (#2572) improvement(dep-stack): security bumps (#2562) fix(utxo): correct block header deserialization for AuxPow and KAWPOW coins (#2563) feat(wallet-connect): impl BTC (UTxO) activation via WalletConnect (#2499) feat(utxo): add new fixed txfee option for DINGO-like coins (#2454) ci(pull-requests): review notification bot (#2468) improvement(walletconnect): return the `pairing_topic` in `new_connection` response (#2538) bless clippy (#2560) refactor(toolchain): use latest available stable compiler (#2557) feat(wallet): implement unified offline private key export API (#2542) improve note for docker test start failure (#2550) fix(DOCS): add note for macos to fix docker containers startup failure (#2544) refactor(toolchain): general stabilization for stable rust (#2528) ... # Conflicts: # mm2src/coins/eth.rs # mm2src/coins/eth/eth_swap_v2/eth_maker_swap_v2.rs # mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs # mm2src/coins/eth/eth_tests.rs # mm2src/coins/eth/eth_withdraw.rs # mm2src/coins/eth/v2_activation.rs # mm2src/coins/nft.rs # mm2src/coins/qrc20.rs # mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs # mm2src/mm2_main/src/rpc/lp_commands/one_inch/rpcs.rs # mm2src/mm2_main/src/rpc/lp_commands/tokens.rs # mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs
* dev: improvement(`static mut`s): `static mut` removal (#2590) fix(orders): set subscription on kickstart and skip GC of own pubkeys (#2597) fix(ordermatch): ignore loop-back; clear on null root; reject stale keep-alives (#2580) fix(clippy): fix clippy warnings for #2565 (#2589) fix(Trezor): fix utxo and eth calls due to firmware changes (#2565) fix(utxo): calculate min_trading_vol based on fixed tx fees (#2564) feat(protocol): [0] solana support (#2586) fix(utxo): fix header deserialization; guard AuxPoW (#2583) chore(rust 1.89): make CI clippy/fmt pass (wasm32, all-targets) (#2581) fix(utxo): deserialize sapling root for PIVX block headers (#2572) improvement(dep-stack): security bumps (#2562) fix(utxo): correct block header deserialization for AuxPow and KAWPOW coins (#2563) feat(wallet-connect): impl BTC (UTxO) activation via WalletConnect (#2499) feat(utxo): add new fixed txfee option for DINGO-like coins (#2454) ci(pull-requests): review notification bot (#2468) improvement(walletconnect): return the `pairing_topic` in `new_connection` response (#2538) bless clippy (#2560) refactor(toolchain): use latest available stable compiler (#2557) feat(wallet): implement unified offline private key export API (#2542) chore(release): v2.3.0-beta (#2284) # Conflicts: # mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs
* dev: (28 commits) fix build script failing to find .git/HEAD (GLEECBTC#2601) refactor(EVM): rename fn, fix timeouts, add activation req validation (GLEECBTC#2543) improvement(`static mut`s): `static mut` removal (GLEECBTC#2590) fix(orders): set subscription on kickstart and skip GC of own pubkeys (GLEECBTC#2597) fix(ordermatch): ignore loop-back; clear on null root; reject stale keep-alives (GLEECBTC#2580) fix(clippy): fix clippy warnings for GLEECBTC#2565 (GLEECBTC#2589) fix(Trezor): fix utxo and eth calls due to firmware changes (GLEECBTC#2565) fix(utxo): calculate min_trading_vol based on fixed tx fees (GLEECBTC#2564) feat(protocol): [0] solana support (GLEECBTC#2586) fix(utxo): fix header deserialization; guard AuxPoW (GLEECBTC#2583) chore(rust 1.89): make CI clippy/fmt pass (wasm32, all-targets) (GLEECBTC#2581) fix(utxo): deserialize sapling root for PIVX block headers (GLEECBTC#2572) improvement(dep-stack): security bumps (GLEECBTC#2562) fix(utxo): correct block header deserialization for AuxPow and KAWPOW coins (GLEECBTC#2563) feat(wallet-connect): impl BTC (UTxO) activation via WalletConnect (GLEECBTC#2499) feat(utxo): add new fixed txfee option for DINGO-like coins (GLEECBTC#2454) ci(pull-requests): review notification bot (GLEECBTC#2468) improvement(walletconnect): return the `pairing_topic` in `new_connection` response (GLEECBTC#2538) bless clippy (GLEECBTC#2560) refactor(toolchain): use latest available stable compiler (GLEECBTC#2557) ... # Conflicts: # mm2src/coins/eth.rs # mm2src/coins/eth/eth_rpc.rs # mm2src/coins/eth/eth_swap_v2/eth_maker_swap_v2.rs # mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs # mm2src/coins/eth/eth_withdraw.rs # mm2src/coins/eth/v2_activation.rs # mm2src/coins/lightning.rs # mm2src/coins/lp_coins.rs # mm2src/coins/nft.rs # mm2src/coins/qrc20.rs # mm2src/coins/siacoin.rs # mm2src/coins/tendermint/tendermint_token.rs # mm2src/coins/test_coin.rs # mm2src/coins/utxo/bch.rs # mm2src/coins/utxo/qtum.rs # mm2src/coins/utxo/slp.rs # mm2src/coins/utxo/utxo_standard.rs # mm2src/coins/z_coin.rs # mm2src/coins_activation/src/bch_with_tokens_activation.rs # mm2src/coins_activation/src/erc20_token_activation.rs # mm2src/coins_activation/src/eth_with_token_activation.rs # mm2src/coins_activation/src/init_erc20_token_activation.rs # mm2src/coins_activation/src/init_token.rs # mm2src/coins_activation/src/platform_coin_with_tokens.rs # mm2src/coins_activation/src/slp_token_activation.rs # mm2src/coins_activation/src/tendermint_with_assets_activation.rs # mm2src/coins_activation/src/token.rs # mm2src/mm2_main/src/lp_swap.rs # mm2src/mm2_main/src/lp_swap/check_balance.rs # mm2src/mm2_main/src/lp_swap/maker_swap.rs # mm2src/mm2_main/src/lp_swap/max_maker_vol_rpc.rs # mm2src/mm2_main/src/lp_swap/swap_v2_rpcs.rs # mm2src/mm2_main/src/lp_swap/taker_swap.rs # mm2src/mm2_main/src/lp_swap/trade_preimage.rs # mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs # mm2src/mm2_main/src/rpc/lp_commands/legacy.rs # mm2src/mm2_main/src/rpc/lp_commands/lr_swap.rs # mm2src/mm2_main/src/rpc/lp_commands/lr_swap/lr_impl.rs # mm2src/mm2_main/src/rpc/lp_commands/one_inch/errors.rs # mm2src/mm2_main/src/rpc/lp_commands/one_inch/rpcs.rs # mm2src/mm2_main/src/rpc/lp_commands/tokens.rs # mm2src/mm2_main/tests/docker_tests/docker_tests_common.rs # mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs # mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs # mm2src/mm2_main/tests/integration_tests_common/mod.rs # mm2src/trading_api/src/one_inch_api/client.rs
…2499) This implements activation only, full WalletConnect implementation for UTXO coins will follow.
This PR implements BTC (generic UTxO, but gotta be supported by the wallet) activation via WalletConnect.
Activation flow is exactly the same as #2223. You initiate a session with the required properties/permissions and then use that session id/topic to activate the coin (examples to be added).
Q: In such activation flow, how do we know which session topic to grab and use for activation of the coin? pragmatically?
To test this, you need to add
chain_idfield in the coins file for the select UTxO coins to be tested. And use a wallet that supports WalletConnect UTxO integration (ledger mobile, and this example web wallet by reown).These are the caip compliant chain_ids for some utxo coins:
The example wallet by reown supports only btc & tbtc. Ledger mobile supports btc, tbtc, ltc, doge. No wallet support KMD as of now.
Activation via Ledger mobile will fail because it doesn't implement
GetAccountAdderessesmethod as of now. With reown web example it should work fine.Suggested to review this commit by commit (only 14, and coherent).
For Testing
Sample
new_connectionrequest:{ "method": "wc_new_connection", "userpass": "{{userpass}}", "mmrpc": "2.0", "params": { "required_namespaces": { "bip122": { "chains": [ "bip122:000000000019d6689c085ae165831e93" //"bip122:4966625a4b2851d9fdee139e56211a0d" ], "methods": [ "getAccountAddresses" //,"signPsbt" ,"signMessage" ], "events": [] } } //,"optional_namespaces": {} } }