refactor: replace ethers runtime dependency with internal ethereUtils#185
refactor: replace ethers runtime dependency with internal ethereUtils#185sherifahmed990 wants to merge 6 commits into
Conversation
Move ethers from a runtime dependency to a dev dependency only. The new src/ethereUtils.ts copies the relevant ethers v6 surface used across the codebase (keccak256, id, hashMessage, ABI codec, solidityPacked, EIP-712 typed-data hashing, RLP, address checksum, secp256k1 sign/recover) with some modification, backed by @noble/curves and @noble/hashes. Every internal call site is migrated to import from ./ethereUtils. Also drops the sharedAbiCoder wrapper in Calibur7702Account in favor of direct encodeAbiParameters / decodeAbiParameters calls, and adds comprehensive parity tests (test/ethereUtils.test.js) — deterministic spec fixtures plus seeded property tests asserting byte-identical output against ethers v6 and viem v2 across 100 random inputs per function. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sherifahmed990
requested review from
Sednaoui and
andrewwahid
and removed request for
Sednaoui
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new local crypto/ABI utility module (src/ethereUtils.ts), updates package.json, replaces selected ethers.js imports across accounts, signers, paymasters, transport, utils, and Tenderly tooling with ethereUtils helpers, and adds comprehensive parity/property tests for the new module. ChangesEthers to EtherUtils Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
package.json declares @noble/curves ^1.2.0 and @noble/hashes ^1.3.2 but the lockfile still carried the prior ^1.9.1 / ^1.8.0 ranges, causing `yarn install --frozen-lockfile` to fail in CI. Resolved versions stay at 1.9.7 / 1.8.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ethereUtils.ts`:
- Around line 679-723: The decode routines (decodeValue and decodeTupleAt)
currently slice the input buffer without validating lengths or offsets, which
causes silent truncation; update decodeValue and decodeTupleAt to validate
bounds before every slice/offset computation: ensure base and head offsets are
within data.length, verify WordSize reads (e.g., for offsets/lengths read with
toNumber/toBigInt) are available, check that offsets are non-negative and that
base+off and base+WordSize+len (for dynamic bytes/string/array) do not exceed
data.length, and for fixed-size reads (uint/address/bool/bytesN) ensure the full
required bytes exist (address needs base+WordSize and base+12..base+WordSize
range). On any violation throw a descriptive Error (include the type and
offending offset/required length). Use existing helpers isDynamic and staticSize
to compute expected static lengths when validating tuples and arrays.
- Around line 439-443: The forEach callback in solidityPacked currently uses a
concise arrow that returns the numeric result of tight.push(...), which triggers
the lint error; change the callback to a void-returning block or replace the
forEach with an explicit loop so it does not return a value—for example, convert
types.forEach((type, index) => tight.push(_pack(type, values[index]))) to a
block-style callback that calls tight.push(_pack(type, values[index])); or use a
for loop iterating over types and calling tight.push(_pack(...)); reference
solidityPacked, the tight array, and the _pack call when making the change.
- Around line 733-736: The function export decodeAbiParameters currently uses an
explicit any[] which violates the noExplicitAny rule; change its return type to
a non-any type (e.g., unknown[] or Array<unknown>) in the signature and remove
the "// eslint-disable-next-line `@typescript-eslint/no-explicit-any`" comment,
keeping the implementation calling parseType, getBytes, and decodeTupleAt as-is
(referencing decodeAbiParameters, types, data, parseType, getBytes,
decodeTupleAt to locate the code).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8eca8ffc-01d2-4894-a09a-d71fbeda3a36
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
package.jsonsrc/account/Calibur/Calibur7702Account.tssrc/account/Safe/MerkleTree.tssrc/account/Safe/SafeAccount.tssrc/account/Safe/SafeMultiChainSigAccount.tssrc/account/Safe/adapters.tssrc/account/Safe/modules/SafeModule.tssrc/account/Safe/multisend.tssrc/account/Safe/safeMessage.tssrc/account/simple/Simple7702Account.tssrc/ethereUtils.tssrc/paymaster/CandidePaymaster.tssrc/paymaster/WorldIdPermissionlessPaymaster.tssrc/signer/adapters.tssrc/signer/types.tssrc/transport/JsonRpcNode.tssrc/utils.tssrc/utils7702.tssrc/utilsTenderly.tstest/_loadEthereUtils.cjstest/ethereUtils.test.js
![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: 93301ed9e5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const raw = toBigInt(data.slice(base, base + WordSize)); | ||
| const masked = mask(raw, t.bits); | ||
| return t.signed ? fromTwos(masked, t.bits) : masked; |
There was a problem hiding this comment.
The integer decode path accepts short buffers and silently zero-fills them (data.slice(base, base + 32) can be empty), so decodeAbiParameters(["uint256"], "0x") returns 0n instead of throwing like ethers does. This changes failure behavior for eth_call decoding (e.g., empty return data from undeployed/reverting targets) into seemingly valid numeric values, which can mask RPC/contract errors and produce incorrect state decisions downstream.
Useful? React with 👍 / 👎.
| return t.signed ? fromTwos(masked, t.bits) : masked; | ||
| } | ||
| case "address": return getAddress(hexlify(data.slice(base + 12, base + WordSize))); | ||
| case "bool": return data[base + 31] !== 0; |
There was a problem hiding this comment.
This bool decoder reads data[base + 31] without bounds checks; when the buffer is shorter than 32 bytes, that expression is undefined, and undefined !== 0 evaluates to true. As a result, malformed/empty ABI data (e.g., "0x") is decoded as true, which can cause false positives in call-result checks such as module/flag queries instead of surfacing a decode error.
Useful? React with 👍 / 👎.
Add `ensureRange` guard around all ABI decode reads to surface out-of-bounds reads with descriptive errors, and replace the `any[]` return on `decodeAbiParameters` with `unknown[]`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ethereUtils.ts`:
- Around line 715-719: After reading len in the "array" case, validate that len
is safe before allocating Array<AbiType>(len): ensure len is non-negative and
that the buffer has at least len * WordSize bytes remaining for the array head
(i.e. check (data.length - (base + WordSize)) >= len * WordSize), throw a
deterministic ABI decode error if not, then allocate the element-type array and
call decodeTupleAt; reference the "array" case, the variables len, base,
WordSize, ensureRange, t.child and the call to decodeTupleAt to locate where to
insert this check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| case "array": { | ||
| if (t.size === -1) { | ||
| ensureRange(data, base, WordSize, "array length"); | ||
| const len = toNumber(data.slice(base, base + WordSize)); | ||
| return decodeTupleAt(Array<AbiType>(len).fill(t.child), data, base + WordSize); |
There was a problem hiding this comment.
Validate dynamic array length before allocating the element type list.
A malformed payload can advertise a huge len and reach Array<AbiType>(len) before we prove the buffer can contain even the minimum array head. That bypasses the new bounds-hardening path and turns an invalid decode into an oversized allocation/failure path instead of a deterministic ABI decode error.
Suggested fix
case "array": {
if (t.size === -1) {
ensureRange(data, base, WordSize, "array length");
const len = toNumber(data.slice(base, base + WordSize));
+ const minHeadSize = isDynamic(t.child) ? WordSize : staticSize(t.child);
+ if (len > Math.floor((data.length - (base + WordSize)) / minHeadSize)) {
+ throw new Error(
+ `ABI decode: array length out-of-bounds ` +
+ `(length=${len}, base=${base}, data.length=${data.length})`,
+ );
+ }
return decodeTupleAt(Array<AbiType>(len).fill(t.child), data, base + WordSize);
}
return decodeTupleAt(Array<AbiType>(t.size).fill(t.child), data, base);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ethereUtils.ts` around lines 715 - 719, After reading len in the "array"
case, validate that len is safe before allocating Array<AbiType>(len): ensure
len is non-negative and that the buffer has at least len * WordSize bytes
remaining for the array head (i.e. check (data.length - (base + WordSize)) >=
len * WordSize), throw a deterministic ABI decode error if not, then allocate
the element-type array and call decodeTupleAt; reference the "array" case, the
variables len, base, WordSize, ensureRange, t.child and the call to
decodeTupleAt to locate where to insert this check.
The unknown[] return introduced in aad98f2 forced narrowing at ~30 call sites that relied on the prior any[] passthrough (matching ethers' Result type). Restore any[] to keep call sites working without per-site casts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
I think A simpler middle ground than the full ABI-string inference is to make the decoder generic and let each call site state the tuple it expects: export function decodeAbiParameters<T extends readonly unknown[] = unknown[]>(
types: ReadonlyArray<string>,
data: BytesLike,
): T {
return decodeTupleAt(types.map(parseType), getBytes(data), 0) as unknown as T;
}Then call sites can be explicit without a complex type-level ABI parser: const decoded = decodeAbiParameters<[bigint]>(["uint256"], result);
const decoded = decodeAbiParameters<[string[], string]>(["address[]", "address"], result);
const decoded = decodeAbiParameters<[[bigint, string]]>(["(uint8,bytes)"], result);I tested this locally and it passes:
This keeps the unsafe assumption localized to each decode site, avoids On the IIFE issue: I’m okay with fixing that in a follow-up PR if you prefer, since the tsdown config is easy to get subtly wrong. The important part is that this PR should not be released/published before the follow-up lands, because the current IIFE build still expects |
2 participants
Summary
Move ethers from a runtime dependency to a dev dependency only. The new src/ethereUtils.ts copies the relevant ethers v6 surface used across the codebase (keccak256, id, hashMessage, ABI codec, solidityPacked, EIP-712 typed-data hashing, RLP, address checksum, secp256k1 sign/recover) with some modification, backed by @noble/curves and @noble/hashes. Every internal call site is migrated to import from ./ethereUtils.
Also drops the sharedAbiCoder wrapper in Calibur7702Account in favor of direct encodeAbiParameters / decodeAbiParameters calls, and adds comprehensive parity tests (test/ethereUtils.test.js) — deterministic spec fixtures plus seeded property tests asserting byte-identical output against ethers v6 and viem v2 across 100 random inputs per function.
Summary by CodeRabbit
New Features
Tests
Chores