Wallet connect multi chain support - #4359
Conversation
|
@Tomasvrba could you rebase? staging-spark got merged, maybe that caused all the conflicts. |
a87ccf8 to
13e12d2
Compare
benma
left a comment
There was a problem hiding this comment.
Codex finds below.
P3 is tricky, as it's annoying to deal with - does it need to be dealt with?
-
[P1] Native-chain contract transactions can crash the backend.
) now stores WalletConnect transactions and schedules
broadcastTransaction() (
an account update. However, TransactionData() ( ) panics when an ETH
account’s pending transaction contains calldata. A pending approval or swap on Ethereum/Sepolia can hit this path. Pending-transaction presentation must support general contract calls before these
transactions enter that storage. The new tests only exercise empty calldata. -
[P2] "input": null is incorrectly rejected.
) rejects every non-nil json.RawMessage,
The new parser (
including JSON null. The previous implementation explicitly accepted it, and its corresponding test was removed during the refactor. This breaks previously accepted requests. Preserve the distinction between
an absent/null field and a populated unsupported alias. -
[P2] Other chains lack pending-transaction protection.
), and nonce selection trusts the remote
Non-native broadcasts skip storage (
nonce directly ( ). If the provider returns a stale pending nonce, consecutive
requests can reuse it, causing replacement or rejection. Transactions lost after an apparently successful broadcast also cannot be rebroadcast. The native path already accounts for these provider behaviors.
Tracking by chain and address would preserve those protections without mixing networks.
| Coin *Coin | ||
| Tx *types.Transaction | ||
| Fee *big.Int | ||
| ChainID uint64 |
There was a problem hiding this comment.
It seems conceptually off to use a coin/account instance tied to a specific chainID to handle transactions of walletconnect by arbitrary other chains. Now account.go has its own chainID and is supplied an external one, which is probably confusing and brittle in the long run, one has to always check to not mix anything up.
Is there a reason for mixing all of this here, or could walletconnect not simply call shared helper functions to get the next nonce, sign the tx, etc?
There was a problem hiding this comment.
addressed in 7095079
All other issues addressed in the fixup commits
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestEVMChainCapabilities(t *testing.T) { |
There was a problem hiding this comment.
This is basically a copypaste of the actual var, don't think this test is valuable and could be removed.
Build approved namespaces from required and optional WalletConnect capabilities shared with request dispatch. Support read-only sessions, reject unsupported proposals, and handle approval and rejection failures.
WalletConnect advertises several EVM networks, but the backend previously accepted transactions only on the account's native network. This caused requests for networks such as Optimism, Base, and Arbitrum to fail. Use the network requested by WalletConnect when getting transaction data, signing, and broadcasting. Use the network requested by WalletConnect for nonce and fee lookup, signing, and broadcasting. Do not mix native-chain pending transactions into another network's nonce selection. Reuse the normal Ethereum signing path instead of maintaining a separate WalletConnect signer. Keep the underlying transaction code independent of WalletConnect so it can also support native multichain features later.
- Centralize metadata for the ten EVM networks supported by the backend. - Derive WalletConnect chain entries from numeric IDs. - Render the registry's SVG assets in pairing and signing requests.
I don't think 3 is urgent to deal with. Given fast confirmation times and slow wallet connect workflow, I think it's unlikely that the provider would provide a stale nonce. We can deal with this in the future if it turns out to be an issue. |
Move WalletConnect transaction construction and fee estimation into standalone functions that use an explicit target chain and RPC client. Keep sender validation, keystore access, signing, and native pending tracking in the account.
13e12d2 to
7095079
Compare
|
|
||
| import { ReactNode, createElement } from 'react'; | ||
| import { IWalletKit } from '@reown/walletkit'; | ||
| import { ArbitrumLogo, OptimismLogo, BaseLogo, ETHLogo } from '@/components/icon'; |
There was a problem hiding this comment.
This removes the last uses of ArbitrumLogo, OptimismLogo, BaseLogo, and ETHLogo. Please also remove the four now-unused wrappers and their SVG imports from frontends/web/src/components/icon/icon.tsx.
| return signingResponse{Success: false, ErrorMessage: err.Error()}, nil | ||
| } | ||
| signedTx, err := ethAccount.SignTransaction(eth.SignTransactionArgs{ | ||
| ChainID: *args.ChainID, |
There was a problem hiding this comment.
My comment about confusing ownership still seems to apply?
chainID is not in the account struct anymore, but passed to functions on the account instance instead, which still means the account tied to one chain is handling other chains.
What do you think?
Codex suggestion:
I would make these changes:
-
Move Account.SignTransaction() into a package-level eth.SignTransaction() function.
Give it the request, target-chain client, signing configuration, the existing ConnectKeystore callback, and access to pending transactions for that chain/address. It should perform sender validation, nonce
selection, construction, signing, broadcasting, and tracking.It should receive no *Account. Derive the expected sender and keypath from the supplied signing configuration. Keeping ConnectKeystore as a callback preserves the current ordering: validate and prepare
before connecting the device. -
Move client selection out of Account.
Remove Account.chainClient() and the account’s chainClientProvider field. Backend wiring resolves the client from the requested chain ID and passes it to the function. It can reuse the existing native-chain
client when appropriate; that is dependency selection during setup.
This PR adds WalletConnect support for multiple EVM networks:
The BitBoxApp now:
transactions.
This depends on #4267 but would be nice to get into the next release. Only the top 3 commits are for this PR.