perf(wallet): port MetaMask hook off web3 to viem - #1330
Open
Danswar wants to merge 8 commits into
Open
Conversation
web3 is CommonJS-only with no sideEffects declaration, so it can't be tree-shaken and ships in full wherever it's imported. Since the MetaMask hook is wired into the app-wide WalletContextProvider (not lazy-loaded), this pulled the entire web3 dependency tree into the main bundle for every visitor regardless of wallet choice. Cuts main.js by ~1.02 MiB (29.8%) in a local production build.
The web3-based implementation explicitly nulled maxFeePerGas/ maxPriorityFeePerGas on every send, forcing legacy pricing regardless of whether a gasPrice override was given (see DFXswiss#163, DEV-2129: some MetaMask/ chain combinations misbehave with EIP-1559 fields). The viem port dropped this for the no-override path, letting viem's default fee estimation pick EIP-1559 again. Resolve gasPrice via publicClient.getGasPrice() when no override is passed, so every transaction still resolves to a legacy-type send.
…ed logic readBalance()'s native-coin branch returned an un-awaited promise from inside a try block, so a rejection (e.g. RPC failure) never hit the function's own catch and the documented throwExceptions=false fallback never fired. Mirror the already-correct ERC20 branch by awaiting the balance before returning. Also add unit tests for readBalance, createTransaction (including the gasPrice/legacy-pricing resolution), and sign against mocked viem clients -- this logic previously had no coverage at all. Soften the gasPrice comment to not overstate certainty about wallet-side behavior that isn't independently verified in this PR.
Cover every path of the ported metamask.hook.ts and of web3.hook.ts at 100% statement/branch/function/line, per the coverage rule in CONTRIBUTING.md. Remove the unreachable default clause in toChainObject: every blockchain that passes the chain-id guard has its own case.
4 tasks
Review findings on the viem port: - send no fee fields unless a gasPrice override is given: the old code's nulled EIP-1559 fields only suppressed web3's own fee filling, so the wallet did its own estimation; pinning eth_gasPrice was drift - unwrap viem's error wrapping in createTransaction so callers keep the raw EIP-1193 code (sell screen swallows code 4001 on deliberate cancel) - wait 750s for the receipt (web3's transactionPollingTimeout), not viem's 180s default - read decimals only when converting the amount; isWeiAmount transfers no longer depend on a decimals() call - toFixed() without digits: fractional base-unit amounts fail loudly again instead of silently rounding - sign hex-shaped messages as raw bytes (web3 inputSignFormatter parity) - retryCount 0 on the custom transports: a fast-failing provider must not exceed checkConnection's 1s race and reload the page
…ge amounts web3 rejected a mined receipt with status false; viem resolves regardless and, on wallet replacement, resolves with the replacement receipt. Check the receipt: reverted rejects, a cancelled or replaced transaction rejects, a repriced one returns the hash that actually mined. BigNumber emits exponential notation from 1e21, which BigInt/parseEther reject - serialize amounts with toFixed() so a thousand-unit 18-decimals payment does not crash before reaching the wallet (parity-neutral: the old web3 path rejected scientific notation the same way).
web3's toWei threw on more than 18 decimals; viem's parseEther silently rounds. Convert with BigNumber and BigInt so sub-wei precision fails loudly on the coin path too, matching the ERC20 path.
Drives a real MetaMask 11.9.1 extension (Sepolia, unfunded throwaway wallet) through connect, personal_sign, a balance-read RPC round-trip, and a wallet-reject path, capturing every window.ethereum.request call so results are asserted against the actual RPC methods/params observed, not screen-scraped popup text. Covers most of the PR's own "manual QA against a live MetaMask wallet" checklist item automatically. Two things still need a funded testnet wallet and aren't covered here: asserting eth_sendTransaction carries no fee fields (this wallet has 0 ETH, so the backend offers EIP-7702 gasless sponsorship instead and the plain-send popup is never reached), and a completed native/ERC20 mined transfer. Run: npm run test:e2e:metamask -- e2e/synpress/pr1330-manual-qa.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #1182: same change rebased onto current develop. The original head branch lives in the base repository and can no longer be updated, so this PR is opened from the fork.
Summary
src/hooks/wallets/metamask.hook.ts: replaced allweb3.eth.*/Contract.methods.*calls withviem'spublicClient/walletClient(getBalance,readContract,writeContract,signMessage,sendTransaction,getAddresses), mirroring the pattern already used inwallet-connect.hook.ts.src/hooks/web3.hook.ts: the one remainingweb3.utils.toHexcall replaced with viem'snumberToHex. Additionally, thedefaultclause intoChainObjectwas removed: it was unreachable (every blockchain that passes the chain-id guard is inchainIds, and everychainIdsentry has its own case), and the coverage rule in CONTRIBUTING.md says to delete lines that genuinely cannot be exercised.web3frompackage.json(the only one of the three that was ever a direct dependency —web3-core/web3-eth-contractwere transitive/type-only imports resolved through it) — confirmed no other file in the repo imports any of them.TextEncoder/TextDecoderpolyfill tosrc/setupTests.ts: jsdom's test environment doesn't provide these, and viem needs them at import time. This was latent —wallet-connect.hook.tsalready used viem but had no test coverage, so nothing had hit this before.jest.mock('web3', ...)blocks from the two existing test files, and brought both touched hooks to full unit coverage against mocked viem clients (readBalance,createTransaction,sign,register, account/chain handling, the EIP-5792 paymaster flow and EIP-7702 authorization signing — this logic had little to no coverage before this PR).Why
web3is CommonJS-only with nosideEffects: falsedeclaration (confirmed via its publishedpackage.json), so it cannot be tree-shaken — importing any part of it always ships the whole package, plus its own legacy crypto-polyfill dependency tree (elliptic,bn.js,eth-lib,secp256k1,asn1.js, IPFS-eramultihashes/multibasefor the long-deadweb3-bzzSwarm support, etc.). SinceuseMetaMaskis wired into the app-wideWalletContextProvider(not behind a lazy route), all of that shipped in the main entry chunk for every visitor, regardless of wallet choice.Measured impact
Re-measured after the rebase —
npm run build:devon Node 20, same checkout and toolchain for both sides (before = develop at 844cb03, after = this branch):main.jsweb3)viem)Behavior preserved intentionally
Each of these was verified against the old web3 1.8 implementation and viem 2.44.0 source, and is pinned by a unit test:
.send()/.sendTransaction()blocked until the transaction was mined before returning the hash (web3'sPromiEventresolves with the receipt), polling for up to 750 s (transactionPollingTimeout). viem returns the hash immediately and itswaitForTransactionReceiptgives up after 180 s by default, so the port waits explicitly with a 750 s timeout — same blocking behavior, same patience. Note MetaMask and WalletConnect (wallet-connect.hook.ts, which does not wait) already behave differently here — out of scope for this PR, flagging for awareness.maxFeePerGas/maxPriorityFeePerGason every send ([DEV-2129] Metamask fees [DEV-2109] WC sign message [DEV-2067] Limit request [DEV-2142] Signature hint #163/DEV-2129). That nulling only suppressed web3's own fee filling — the request reached MetaMask without fee fields and the wallet did its own estimation;gasPricewas passed only when a caller supplied an override. The port keeps that exact wire shape: no fee fields unless agasPriceoverride is given (viem's JSON-RPC account path adds no fee estimation of its own and drops undefined fields — verified insendTransaction/formatTransactionRequestsource). An earlier iteration of this branch pinned a fetched legacyeth_gasPriceon every send; that was drift from the old behavior and is gone.sendTransaction/writeContractinTransactionExecutionError/ContractFunctionExecutionError, which carry no top-levelcode— but callers rely on the raw EIP-1193 shape (sell.screen.tsxswallowserror.code === 4001on a deliberate cancel).createTransactiontherefore rethrows the first cause that still carries a numericcode.requestAddresses/signMessageare not wrapped by viem (the RPC error classes setcodeon the instance), so the existinghandleErrormapping (4001 →AbortError, −32002 →TranslatedError) is unchanged.inputSignFormatterpassed hex-shaped messages through unchanged (signed as the bytes they encode); viem would UTF-8-encode the literal text.sign()passes hex messages as{ raw }to keep the signatures identical. No current caller sends hex (the only caller signs the DFX auth message) — parity kept for the exposed hook API.BigIntthrows on the fractional base-unit string) on both paths, instead of being silently rounded:.toFixed()without digits for the ERC20 conversion, and an explicit BigNumber wei conversion for native coins — web3'stoWeithrew on more than 18 decimals, and viem'sparseEtherwould silently round, soparseEtheris not used. Token decimals are only read when the amount actually needs converting —isWeiAmounttransfers don't depend on adecimals()call, as before.status: false("Transaction has been reverted by the EVM"), and kept polling the original hash when the wallet replaced or cancelled the transaction. viem resolveswaitForTransactionReceiptregardless of status, and on replacement resolves with the replacement receipt — so the port checks the receipt: reverted rejects, a wallet-cancelled or replaced transaction rejects, and a repriced (fee-bumped) one resolves with the hash that actually mined instead of the stale submitted one.toFixed()instead oftoString(): BigNumber emits exponential notation from 1e21 (a thousand units of an 18-decimals token), whichBigIntrejects. The old web3 path (number-to-bn) rejected scientific notation the same way, so this edge — reachable through payment-link URIs — crashed before reaching the wallet on both sides; now it goes through. Fractional wei amounts still fail loudly.readBalance()fix. The native-coin branch had a pre-existing bug carried over from the web3 code (return promise.then(...)inside atrydoesn't get caught by thattry's owncatch), which the ERC20 branch didn't have. Fixed to match.Coverage
Per touched file, measured with the full suite (
npm run test -- --coverage --collectCoverageFrom='<file>'):src/hooks/wallets/metamask.hook.tssrc/hooks/web3.hook.tssrc/setupTests.tsand the test files themselves produce no coverage rows — Jest does not instrument them.Test plan
npm run test— all 1157 tests pass (89 suites), including the new hook coveragenpx tsc -p tsconfig.build.json --noEmitcleannpm run lintandnpm run format:md:checkcleannpm run build:devandnpm run widget:devsucceed;main.jsmeasured before/after with the same toolchaine2e/synpress/pr1330-manual-qa.spec.ts— automated evidence against a real MetaMask 11.9.1 extension on Sepolia, replacing most of the manual-QA item this PR originally left unchecked:getAddresses— real popup flow,eth_requestAccountsobserved on the wirereadBalance()has no reachable UI path without a live DFX payment-link quote (the repo's owne2e-stackpayment-link tests hit the same gap); instead issued the identicaleth_getBalance/eth_call balanceOf(address)calls it makes, directly against a live connected MetaMask provider — both round-trip correctlypersonal_sign— real "Sign" popup, approved,personal_signobserved on the wirehandleError()/ code-4001 cancel path in the same file, just a sibling function tocreateTransaction()rather than that function itselfeth_sendTransactioncarries no fee fields (blocked by the gasless routing above: needs enough Sepolia ETH that the backend stops offering gasless sponsorship)A green unit run does not prove the ported calls work against a live wallet: the viem clients are replaced in these tests, which is what the unit layer means by "surroundings replaced." The automated MetaMask pass above closes most of that gap; a funded testnet wallet for the two remaining boxes is the only thing left before merging.
Noted while verifying, in code this PR does not touch:
src/__tests__/transaction-list-txinfo.test.tsx("sorts combined detail and unassigned transactions newest-first") fails deterministically in timezones behind UTC (e.g.TZ=America/New_York: the fixtures pin midnight UTC on 2026-01-01, which a negative offset shifts into December 2025) and passes in UTC and zones ahead of it — CI never sees it because runners are UTC.