Review follow-ups: transport, Tenderly, Safe v0.6, paymaster, and allowance fixes - #229
Conversation
hasRpcError treated any non-null "error" field as a JSON-RPC error, so an
HTTP failure with a body like {"error": "rate limited"} bypassed the
HTTP-status branch and surfaced only "malformed JSON-RPC response",
losing the status code and text (e.g. 429 Too Many Requests). Require a
properly shaped JSON-RPC error object (numeric code, string message)
before trusting the body over the status.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The EIP-7702 delegation-code injection looked up the sender in caller stateOverrides by lowercase key only, so an entry keyed by a checksummed address was missed and the payload carried two state_objects keys for the same EVM address — Tenderly then resolved the duplicate unpredictably, dropping either the caller's override or the delegation code. Merge the sender entry case-insensitively at both injection sites, and in callTenderlySimulateBundle normalize all override keys to lowercase, rejecting same-address keys that differ only in case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…estimation overrides.initCode was applied only after the base builder had derived isInit from factoryAddress and estimated gas with a factory-derived initCode, so a caller-supplied "0x" or custom initCode produced a mismatched WebAuthn dummy-signature encoding and estimation op. Resolve the final initCode first and derive isInit from it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Only preVerificationGas was zeroed before estimateUserOperationGas, so caller-supplied verificationGasLimit and callGasLimit leaked into the estimation request. Zero all three (matching SafeAccount and Erc7677Paymaster); supplied limits still act as minimums afterward. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An out-of-range recurringAllowanceValidityPeriodInMinutes previously surfaced as the ABI encoder's generic "value out-of-bounds for uint16" Error. Reject it up front with a RangeError that names the parameter, consistent with the existing timestamp validation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
newestMtimeMs ignored directory mtimes, so deleting a src file (which only updates the parent directory) never triggered a rebuild and tests kept running against stale dist code. Include each directory's own mtime in the scan. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Partially reverts 6f7156b. On v0.7+ verificationGasLimit and callGasLimit are paymaster-independent (paymaster validation has its own gas field), so the caller's prior estimates stay valid and are passed through — bundlers that skip estimating supplied fields answer faster. Only preVerificationGas is zeroed for re-estimation, since the paymaster proof calldata grows it. Documented that supplied limits are trusted as prior estimates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe changes resolve Safe v0.6 initialization state, validate allowance periods, clarify and test paymaster gas estimation, tighten HTTP error parsing, normalize Tenderly state overrides, and improve test distribution freshness checks. ChangesSafe initialization
Allowance validation
Paymaster estimation
Transport error handling
Tenderly state overrides
Test build freshness
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
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/utilsTenderly.ts`:
- Around line 53-60: Update the state-overrides handling before the merge loop
in callTenderlySimulateBundle to detect multiple sender override keys
case-insensitively and reject them before any entries are combined. Preserve the
duplicate entries long enough to validate them, and only merge overrides when no
duplicate sender keys exist.
🪄 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: 17dd5bc4-f0c2-40ab-9493-9275f207b49e
📒 Files selected for processing (11)
src/account/Safe/SafeAccount.tssrc/account/Safe/modules/AllowanceModule.tssrc/paymaster/WorldIdPermissionlessPaymaster.tssrc/transport/HttpTransport.tssrc/utilsTenderly.tstest/_ensureFreshDist.cjstest/paymaster/worldIdPermissionlessPaymaster.test.jstest/safe/allowanceModule.test.jstest/safe/initCodeOverrideIsInit.test.jstest/transport/httpErrorHandling.test.jstest/utilsTenderly7702.test.js
withSenderCodeOverride silently folded multiple caller entries matching the sender under different casings (last key's fields winning), which also collapsed them before callTenderlySimulateBundle's duplicate-key rejection could see them — so duplicates for the sender merged silently while duplicates for any other address threw. Reject them with the same RangeError as the choke point; merging only happens when the sender has at most one entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes a batch of issues surfaced by bot review findings, each independently verified against the code before applying. Every fix ships with regression tests; the full suite (548
tests, 26 suites) and type tests pass.
Transport
body like {"error": "rate limited"} bypassed the HTTP-status branch and surfaced only a generic "malformed JSON-RPC response", losing the status code (e.g. 429). Only a properly
shaped JSON-RPC error object (numeric code, string message) now falls through to response parsing.
Tenderly simulation
checksummed caller entry was missed, producing two state_objects keys for the same EVM address and undefined server-side resolution. Both injection sites now merge via a shared
withSenderCodeOverride helper, and callTenderlySimulateBundle normalizes all override keys to lowercase, rejecting same-address keys that differ only in case.
Safe accounts
and estimated gas with a factory-derived initCode. A caller-supplied "0x" or custom initCode therefore produced a mismatched WebAuthn dummy-signature encoding and estimation op. The
final initCode is now resolved first and isInit derives from it.
"value out-of-bounds for uint16" error; it's now rejected up front with a RangeError that names the parameter.
WorldId paymaster
is what invalidates it. Supplied verificationGasLimit/callGasLimit are paymaster-independent on v0.7+ and pass through as trusted prior estimates — bundlers that skip estimating
supplied fields answer faster — with the existing max(supplied, estimated) clamp as a guard. The precondition is documented on the method.
Test infrastructure
against stale dist code.
Review findings intentionally skipped
change.
Summary by CodeRabbit
Bug Fixes
Tests
Chores