refactor(cli): migrate base, utils, test-utils to viem [4/8]#769
refactor(cli): migrate base, utils, test-utils to viem [4/8]#769pahor167 wants to merge 1 commit into
Conversation
- 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)
|
![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: 6929a3371c
ℹ️ 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".
|
|
||
| // TODO: How can we deploy contracts with the Celo provider w/o a CeloTransactionObject? | ||
| export async function displayWeb3Tx(name: string, txObj: any, tx?: Omit<CeloTx, 'data'>) { | ||
| export async function displayTx(name: string, txObj: any, tx?: Omit<CeloTx, 'data'>) { |
There was a problem hiding this comment.
displaySendTx exported for existing commands
Renaming displaySendTx to displayTx here breaks the current command surface because many command modules still import and call displaySendTx (for example packages/cli/src/commands/account/register.ts, packages/cli/src/commands/governance/approve.ts, and numerous others in this commit state). Without a compatibility export, TypeScript build/runtime module loading for those commands fails until every caller is migrated in the same change.
Useful? React with 👍 / 👎.
| async newProvider(): Promise<Provider> { | ||
| if (!this._provider) { | ||
| const nodeUrl = await this.getNodeUrl() | ||
| this._provider = getProviderForKit(nodeUrl, undefined) | ||
| } | ||
| return this._provider | ||
| } |
There was a problem hiding this comment.
BaseCommand.getWeb3 until callers are migrated
This refactor removes the getWeb3() API from BaseCommand, but multiple command files in this commit still invoke this.getWeb3() (e.g. commands/account/lock.ts, commands/account/unlock.ts, and governance commands). That leaves those commands broken as soon as they execute, so this change needs a temporary shim or an atomic migration of all call sites.
Useful? React with 👍 / 👎.
| export const createSafe = async ( | ||
| provider: Provider, | ||
| signer: StrongAddress, | ||
| safeAddress: StrongAddress | ||
| ) => { |
There was a problem hiding this comment.
Replacing createSafeFromWeb3 / safeTransactionMetadataFromCeloTransactionObject with new names here is a breaking API change for existing governance commands that still import the old symbols (commands/governance/approve.ts, propose.ts, withdraw.ts in this commit). Those imports will fail unless these legacy exports remain available until callers are updated.
Useful? React with 👍 / 👎.
…es (#780) ## 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 (#766) - `@celo/contractkit`: viem-native contract interaction across all wrappers (#767) - `@celo/explorer`, `@celo/governance`, wallets, dev-utils (#768) - `@celo/celocli`: all command groups migrated (#769–#773) ### Review fixes on top (highlights, 38 findings fixed) Funds/correctness critical: - wallet-base: tx quantities above 2^53 were silently truncated when hex-encoding (`1 CELO + 1 wei` signed as exactly 1 CELO); now BigInt-backed with a regression test - connect: `defaultFeeCurrency` was silently ignored for every `contract.write`; dynamic chain now carries the Celo serializers/formatters so CIP-64 serializes correctly - contractkit: IPC provider hung forever (parsed only on socket `end`); ws:// / wss:// node URLs fell through to the HTTP provider; HTTP error statuses resolved `undefined` - contractkit: `setAccount` without proof-of-possession always threw (`'0x0'` as bytes32); `Attestations.getPendingWithdrawals` parameter order restored to match the contract - cli: `releasecelo:admin-revoke` lost the `{from: voteSigner}` override → revokes reverted with an authorized vote signer - connect: fee-math radix bug inflated `maxFeePerGas` for string inputs High: - tuple-blind ABI signatures produced wrong selectors/topic hashes (ABI enrichment, `methodIds`, log-explorer event matching) - `BaseWrapper.getPastEvents` swallowed RPC errors as "no events"; unknown event names now throw (web3 parity) - Election historical `blockNumber` queries were silently ignored (voter rewards at past epochs) - governance CLI commands no longer displayed on-chain event confirmations (ProposalExecuted / HotfixExecuted / HotfixApproved / Deposit) — restored, including the silently-dropped test snapshots - `isValidChecksumAddress` accepted all-lowercase addresses; sourcify proxy implementation address checksummed again - BigInt-safe JSON-RPC serialization in production providers Plus 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.md` on the working machine; findings are also traceable through the individual commit messages. ## Test plan - [x] Full workspace build green (all 23 packages) - [x] Full test suite: all packages green locally except known env-flaky timeouts under full parallel load (actions/keystores) and a pre-existing `election:activate --wait` snapshot-order flake that reproduces on the unmodified stack HEAD - [x] Anvil-backed suites green: contractkit 258/258, celocli 337 passed - [x] Live smoke tests: IPC provider against `anvil --ipc`, WebSocket provider against `wss://forno.celo.org/ws` - [ ] CI green on this PR --------- Co-authored-by: Pavel Hornak <pavel.hornak@clabs.co> Co-authored-by: Paul Lange <palango@gmx.de>
1 participant
Stack
Changes
base.ts: remove web3 from base command classutils/cli.ts: replacedisplaySendTxwithdisplayViemTxutils/command.ts: migrate transaction helpers to viemutils/checks.ts,governance.ts,release-gold-base.ts: remove web3 typestest-utils/: chain-setup, cliUtils, mockRpc updated for viemexchange.tsandexchange.test.ts(dead code)Part of the web3 → viem migration. No CI requirement on this PR.
PR-Codex overview
This PR primarily focuses on refactoring the codebase to replace instances of
Web3with a newProviderinterface, enhancing consistency and functionality across the code. It also removes outdated methods and optimizes the transaction handling process.Detailed summary
exchange.tsandexchange.test.tsfiles.tsconfig.jsonto exclude all test files.requireCallfunction fromrequire.ts.Web3withProviderin several files.viemactions for receipts.Provider.Providerinterface.