refactor: migrate from web3 to viem (combined) + migration review fixes#780
refactor: migrate from web3 to viem (combined) + migration review fixes#780pahor167 wants to merge 38 commits into
Conversation
- Delete rpc-caller.ts, celo-transaction-object.ts, tx-result.ts, promi-event - Add contract-types.ts (CeloContract<TAbi>), viem-abi-coder.ts, wallet-adapter.ts - Replace web3 ABI types with viem-native equivalents in abi-types.ts - Rewrite connection.ts internals to use viem client - Remove BigNumber/bn.js from utils β use native bigint in solidity.ts, signatureUtils.ts, ecies.ts - Remove @ethereumjs/* dependencies - Patch buffer-equal-constant-time for Node 25 compatibility - Drop web3 from yarn.lock
- Migrate BaseWrapper from proxySend/proxyCall to contract.write/contract.read - Add strongly-typed generic ABIs to BaseWrapper and all 36 wrappers (~110 any eliminated) - Replace web3-contract-cache.ts with contract-factory-cache.ts - Delete PromiEventStub.ts - Remove web3 dependency from kit.ts and mini-kit.ts - Replace CeloTransactionObject with direct tx hash + waitForTransactionReceipt
- explorer: replace web3 Contract with viem getContract(), remove deprecated ABI helpers - governance: replace createViemTxObject with encodeFunctionData in ProposalBuilder - wallets: replace web3 signing utils with viem equivalents across all HSM/ledger/local wallets - dev-utils: chain-setup and test utils use viem client - transactions-uri: remove web3 dependency
- base.ts: remove web3 from base command class - utils/cli.ts: replace displaySendTx with displayViemTx - utils/command.ts: migrate transaction helpers to viem - utils/checks.ts, governance.ts, release-gold-base.ts: remove web3 types - test-utils/: chain-setup, cliUtils, mockRpc updated for viem - Remove exchange.ts and exchange.test.ts (dead code)
Replace web3-based transaction patterns with viem equivalents across account/, dkg/, election/, and epochs/ CLI commands and their tests.
Replace web3-based transaction patterns with viem equivalents across all governance/ CLI commands and their tests.
β¦r commands to viem Replace web3-based transaction patterns with viem equivalents across lockedcelo/, multisig/, network/, oracle/, identity/, rewards/, and transfer/ CLI commands and their tests.
β¦s to viem Replace web3-based transaction patterns with viem equivalents across releasecelo/, validator/, and validatorgroup/ CLI commands and their tests.
β¦shot - SimpleHttpProvider.request: add replacer to JSON.stringify that converts bigint to hex string, fixing "Do not know how to serialize a BigInt" when viem passes bigint blockNumber params via the provider - election/activate.test.ts: update inline snapshots to reflect correct sequential tx log order (finishNextEpoch txHash arrives before activate starts)
- Remove extra trailing txHash entry from two activate --wait snapshots (displayViemTx only emits one txHash per call, not two) - Increase reorders member beforeEach/test timeouts from 30s to 60s - Add 60s timeout to epoch block information tests (viem adds pre-flight gas estimation making each write ~50% more RPC calls than web3)
β¦t and checksum regressions
- connect/contract-types: inject connection-level default feeCurrency into
contract.write calls and build the dynamic chain with viem/celo chainConfig
(serializers + formatters) so CIP-64 transactions serialize correctly.
Previously kit.setFeeCurrency() was silently ignored for all wrapper writes.
- connect/connection: setFeeMarketGas no longer calls .toString(16) on a
possibly-string maxPriorityFeePerGas (String#toString ignores the radix,
inflating maxFeePerGas); BigInt() now handles all input types. Fall back to
eth_gasPrice when baseFeePerGas is null (non-EIP-1559 chains).
- contractkit/setupForKits: BigInt-safe JSON.stringify in SimpleHttpProvider
and SimpleIpcProvider, matching the dev-utils test provider.
- utils/address: isValidChecksumAddress again requires exact EIP-55 mixed-case
form; viem's isAddress({strict:true}) accepts all-lowercase addresses.
- signatureUtils: normalize v to a 0/1 recovery bit at all recoverPublicKey
sites; @ethereumjs/util ecrecover accepted both {0,1} and {27,28}, the
direct secp256k1 port only handled 27/28
- ecies: request uncompressed point encoding explicitly (toRawBytes defaults
to compressed), keeping the documented 64/65-byte code path
- connect: remove accidentally duplicated setFeeMarketGas describe block
Deduplicates viem in yarn.lock β ^2.0.0 resolved to a second copy (2.46.3) alongside the 2.33.2 used by the SDK packages.
β¦HTTP error status handling - SimpleIpcProvider resolved only on the socket 'end' event, but geth keeps IPC connections open between requests, so every IPC call hung forever. Parse the accumulated buffer on each 'data' chunk and settle once it forms valid JSON (verified against anvil --ipc). - getProviderForKit returned SimpleHttpProvider for ws:// and wss:// URLs; web3's `new Web3(url)` used to auto-create a WebsocketProvider. Route WebSocket URLs through viem's webSocket transport (verified against wss://forno.celo.org/ws). - SimpleHttpProvider now rejects on HTTP status >= 400 instead of resolving undefined when a proxy returns a non-JSON-RPC error body.
β¦ audit - Accounts.setAccount: null proof-of-possession path passed '0x0' as bytes32 r/s β viem rejects under-sized byte values, so every such call threw before sending. Use a full 32-byte zero value. - Attestations.getPendingWithdrawals: restore the (token, account) parameter order; the migration swapped the names while keeping callers positional, so callers following the new signature would query a wrong mapping slot. - BaseWrapper.getPastEvents: stop swallowing RPC errors (block-range limits etc.) β an empty array is indistinguishable from 'no events'. Unknown event names now throw, matching web3 behavior. - BaseWrapper.version(): throw a descriptive error when the contract does not implement getVersionNumber() instead of returning undefined and crashing later in onlyVersionOrGreater. - Election: thread the blockNumber parameter through to viem reads again β it was silently ignored, breaking historical queries (e.g. voter rewards at past epochs). - Governance: remove manual left-pad() of bytes32 hash/salt arguments (web3 right-padded; for valid 32-byte hashes both are no-ops, for short inputs left-padding silently produced different hashes β viem now fails loudly); make the VoteValue -> on-chain enum mapping explicit. - Escrow.escrowedPayments: include the receivedIndex struct field. - Erc20/OdisPayments: use toViemAddress/toViemBigInt for argument safety (raw floats or 0x-less addresses threw or lost precision in BigInt()).
executehotfix-l2-transactions.json and hashhotfix-l2-transactions.json are written by beforeAll and removed by afterAll in their test suites; tracking them means every test run deletes tracked files and dirties the worktree. The other three governance fixtures of the same pattern were never committed.
β¦Number - kit.test: the inflation-factor test mocked estimateGasWithInflationFactor itself, so the multiplication was never verified; mock the raw estimateGas and assert base * factor instead - contract-factory-cache.test: remove bare no-op expect - Accounts.getName: thread blockNumber through to the viem read (same regression class as Election historical queries)
- remove-rpc-contract-promievent: bump @celo/contractkit to major and list the kit-level breaking changes (sendTransaction return type, kit.web3 shim removal, isListening/isSyncing/gasPriceSuggestionMultiplier removal, CeloToken type export removal) - viem-native-migration: drop the stale 'public API unchanged' claim
β¦-indexed args The previous tests only checked that encoded data was defined; they now verify the decoded event name and argument values round-trip exactly.
β¦ent arg safety
- formatter.isHash compared string length to 32 (bytes) instead of 66
('0x' + 64 hex chars), so every valid storage key in an access list was
rejected (pre-existing bug, now load-bearing for viem tx parsing)
- CeloTokenWrapper.transferWithComment: apply toViemAddress/toViemBigInt like
the other token wrappers
β¦nctions Joining raw input types hashed 'fn(tuple)' instead of the expanded canonical signature, yielding a wrong 4-byte selector (e.g. Validators.initialize: 0xd7d794bf instead of 0xa09f7f55). Use viem's toFunctionSelector.
β¦ities
stringNumberToHex replaced web3's BN-backed numberToHex with
Number(num).toString(16), silently truncating integers above 2^53 β a
decimal-string value like '1000000000000000001' (1 CELO + 1 wei) was signed
as exactly 1 CELO. Use BigInt instead (handles decimal and 0x-hex strings),
add a regression test, and guard BigInt('0x') for r/s of unsigned txs in
getPublicKeyofSignerFromTx.
β¦oxy implementation, hex-encoded eth_call value - connect: ABI enrichment in getCeloContract used naive type-joining for signatures β tuple inputs hashed as 'fn(tuple)', producing wrong selectors and topic hashes; use toFunctionSelector/toEventSelector - explorer/log-explorer: same naive event signature recomputation in tryParseLog (tuple-param events silently never matched); skip pending logs whose block/index fields are null instead of reporting block 0 - explorer/sourcify: checksum the storage-slot-derived proxy implementation address again (map keys from registry.addressMapping() are checksummed) - cli/governance: hex-encode tx.value for raw eth_call β decimal quantity strings violate JSON-RPC and strict nodes reject them
- dkg/get: guard undefined call results (clear error instead of opaque decode throw), BigInt-safe JSON output, dedupe the six copy-pasted call+decode blocks - dkg/deploy: print txHash/contractAddress/status instead of dumping the raw viem receipt object - election/activate.test: restore exact-equality vote assertions that were weakened to >= during migration (verified passing; the two --wait snapshot failures are a pre-existing local timing flake, reproduced on clean HEAD)
β¦again
- releasecelo admin-revoke dropped the {from: voteSigner} override that
master passed to election and governance revoke transactions; without it
they are sent from the default account and revert when a separate vote
signer is authorized. Thread txParams through
ReleaseGold.revokeAllVotesForAllGroups/revokeAllVotesForGroup/
revoke{Pending,Active}Votes to make the override possible again.
- transfer-stable-base: affordability error message read res.flags.feeCurrency,
which never exists (flag is gasCurrency) β the chosen fee token was always
omitted from the message
The migration dropped the decodeEventsOpts argument from displayViemTx in governance:execute, executehotfix, approve, and withdraw β operators no longer saw ProposalExecuted / HotfixExecuted / HotfixApproved / ProposalApproved / Deposit confirmations, and the test snapshots were regenerated without them, hiding the regression. Also fail with a clear message when an approve target proposal is not in the dequeue instead of encoding index -1 into the multisig calldata.
β¦ultiplier checks The contracts compare against block.timestamp; using Date.now() gives wrong answers on devchains where block time diverges from wall-clock time. The validator-group variant was already fixed during the migration β align the validator deregister and slashing-multiplier-reset checks with it.
- dev-utils evmRevert: surface evm_revert returning false (stale snapshot) instead of silently running tests against unreverted state - cli test-utils: wait for receipts between dependent transactions in multisig proxy setup and ReleaseGold initialization (anvil automine currently masks the ordering race) - propose.test: pass functionName to encodeFunctionData explicitly
β¦, test conventions - wallet-base: normalize 0x prefix before RLP decoding and reject unprefixed input in getSignerFromTxEIP2718TX (slice(4) on unprefixed hex silently decoded a wrong transaction type) - cli base: _wallet getter returned itself (stack overflow if ever read); return the connection's wallet - cli test-utils: extractHostFromProvider throws instead of silently falling back to localhost:8545 for ws/ipc inner providers - releasecelo set-account.test: restore beneficiary/releaseOwner argument order used by every other releasecelo test - validator deregister.test: wait for receipts of the three requirement- setting transactions
π¦ Changeset detectedLatest commit: 11578c1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02106b046c
βΉοΈ About Codex in GitHub
Codex has been enabled to automatically 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 π.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const logs = await this.client.getLogs({ | ||
| address: this.contract.address, | ||
| event: eventAbi as any, | ||
| fromBlock, | ||
| toBlock, | ||
| }) |
There was a problem hiding this comment.
When callers supply PastEventOptions.filter or topics (for example, getPastEvents('Transfer', { filter: { from: account }, ... })), this new viem implementation only forwards the event name and block range, so it returns every matching event in the range instead of the requested subset. The old web3 getPastEvents honored those options, and PastEventOptions still exposes them, so consumers that rely on filtered histories will overcount or process unrelated logs.
Useful? React with πΒ / π.
# Conflicts: # packages/cli/src/base.ts # packages/cli/src/utils/governance.ts
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbfb12afcf
βΉοΈ About Codex in GitHub
Codex has been enabled to automatically 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 π.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| debugPayload('%O', payload) | ||
| request: EIP1193RequestFn = async ({ method, params }) => { | ||
| const safeParams: any[] = Array.isArray(params) ? params : params != null ? [params] : [] |
There was a problem hiding this comment.
When a caller uses the wrapped EIP-1193 provider for a method whose params is an object rather than an array (for example wallet-style methods such as wallet_watchAsset), this normalization wraps that object in an array and the default branch forwards the changed shape to existingProvider.request. That makes otherwise valid provider calls arrive as params: [{...}] instead of params: {...}, so downstream providers reject or misinterpret them; use the array form only for the intercepted methods and forward the original params for all other methods.
Useful? React with πΒ / π.
size-limit report π¦
|
Codecov Reportβ Patch coverage is β Your patch check has failed because the patch coverage (41.76%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #780 +/- ##
==========================================
- Coverage 69.61% 67.24% -2.37%
==========================================
Files 150 152 +2
Lines 7075 7559 +484
Branches 1163 1219 +56
==========================================
+ Hits 4925 5083 +158
- Misses 2080 2355 +275
- Partials 70 121 +51
π New features to boost your workflow:
|
- election:activate --wait tests: the CLI's polling races the concurrent epoch switch issued by the test, so the exact log interleaving is nondeterministic β snapshot sorted log lines instead (flaked locally and on CI) - releasecelo suites: raise hook timeouts to 60s where the hook deploys a multisig + ReleaseGold contract; the added receipt waits exceed jest's default 10s on slow CI runners
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8994c2d5f
βΉοΈ About Codex in GitHub
Codex has been enabled to automatically 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 π.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| async fetchBlock(blockNumber: number): Promise<Block> { | ||
| return this.kit.connection.getBlock(blockNumber) | ||
| return this.kit.connection.viemClient.getBlock({ blockNumber: BigInt(blockNumber) }) |
There was a problem hiding this comment.
When blocks come from fetchBlock()/fetchBlockByHash(), viem returns only transaction hashes unless includeTransactions: true is set, while parseBlock() only attempts to parse entries that are not strings. As a result, explorer callers that fetch a block and then parse it now get an empty parsedTx list for every block even when it contains known contract calls; the previous Connection.getBlock defaulted to full transaction objects.
Useful? React with πΒ / π.
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11578c1cf1
βΉοΈ About Codex in GitHub
Codex has been enabled to automatically 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 π.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (Array.isArray(value)) { | ||
| return value.map(bigintToString) | ||
| } | ||
| return value |
There was a problem hiding this comment.
When decodeParametersToObject() decodes a named tuple/struct that contains uint/int fields, viem returns an object with bigint leaves; this helper only converts top-level bigints and arrays, so those nested bigints remain in params. That regresses tuple/struct proposal decoding paths such as proposalToJSON() for initializer args, and callers that serialize the result (for example governance:show --jsonTransactions) can now hit JSON.stringify failures or receive bigint values where the legacy decoder returned strings.
Useful? React with πΒ / π.
1 participant
Summary
Combined branch containing the full web3 β viem migration (the union of the stacked PRs #766β#773) plus 24 commits of review fixes from a deep multi-agent audit of the cumulative diff (~350 files).
Migration (from the original stack)
@celo/connect,@celo/utils: web3 replaced with viem-native primitives (refactor(connect,utils): replace web3 with viem [1/8]Β #766)@celo/contractkit: viem-native contract interaction across all wrappers (refactor(contractkit): migrate to viem-native contract interaction [2/8]Β #767)@celo/explorer,@celo/governance, wallets, dev-utils (refactor(explorer,governance,wallets,dev-utils): migrate to viem [3/8]Β #768)@celo/celocli: all command groups migrated (refactor(cli): migrate base, utils, test-utils to viem [4/8]Β #769βrefactor(cli): migrate releasecelo, validator, validatorgroup to viem [8/8]Β #773)Review fixes on top (highlights, 38 findings fixed)
Funds/correctness critical:
1 CELO + 1 weisigned as exactly 1 CELO); now BigInt-backed with a regression testdefaultFeeCurrencywas silently ignored for everycontract.write; dynamic chain now carries the Celo serializers/formatters so CIP-64 serializes correctlyend); ws:// / wss:// node URLs fell through to the HTTP provider; HTTP error statuses resolvedundefinedsetAccountwithout proof-of-possession always threw ('0x0'as bytes32);Attestations.getPendingWithdrawalsparameter order restored to match the contractreleasecelo:admin-revokelost the{from: voteSigner}override β revokes reverted with an authorized vote signermaxFeePerGasfor string inputsHigh:
methodIds, log-explorer event matching)BaseWrapper.getPastEventsswallowed RPC errors as "no events"; unknown event names now throw (web3 parity)blockNumberqueries were silently ignored (voter rewards at past epochs)isValidChecksumAddressaccepted all-lowercase addresses; sourcify proxy implementation address checksummed againPlus test-semantics restorations (weakened assertions, mocked-method-under-test), test-infra robustness (receipt waits, strict
evm_revert), changeset corrections (contractkit correctly marked major with full breaking-change list), and documentation of behavior changes.Full audit trail (62 findings: 38 fixed, 12 refuted with evidence, the rest classified pre-existing/wontfix) lives in
.omc/ralph/pr-review-progress.mdon the working machine; findings are also traceable through the individual commit messages.Test plan
election:activate --waitsnapshot-order flake that reproduces on the unmodified stack HEADanvil --ipc, WebSocket provider againstwss://forno.celo.org/ws