Repository navigation
feat(evm-simulation): add block gas limit and parentHash check config (SDK-1379) - #1303
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Claude finished @Foulks-Plb's task in 2m 13s —— View job Review of PR #1303: approved, 2 non-blocking findings
Findings: 0 critical · 0 high · 1 medium · 1 low
I did not run lint or tests in this review. |
There was a problem hiding this comment.
✅ Devin resolved the finding on 2683a8f
Fixed by Devin (1)
- Parent-hash documentation misses caller overrides
|
/lupin review |
There was a problem hiding this comment.
Code Review Summary
Overview
This PR adds an optional ChainSimulationConfig.blockOverrides.gasLimit. It is forwarded as the eth_simulateV1 block's blockOverrides.gasLimit and omitted when unset. It also adds a consumer-configurable parentHashCheck, which defaults to off on Stable (988) and on for every other chain. The block number, timestamp and reorg checks still run when the parentHash check is off. The implementation is small, has tests for both paths, and the docs and minor changeset are consistent. The only remaining findings: an invalid gas limit is reported as a bypassable ExternalServiceError instead of a typed validation error, and one @throws JSDoc is stale after the second commit.
Findings
- Critical: 0 issues
- High: 0 issues
- Medium: 1 issue
- Low: 1 issue
See inline comments for details.
Guidelines Compliance
- Follows TypeScript strict mode
- Uses early returns over nested conditionals
-
bigintfor onchain quantities; WAD-scaled where appropriate - Reuses SDK types (
Address,MarketId,ChainId,BigIntish) - Type-only imports where possible
- Relative imports use
.jssuffix (NodeNext) - Public APIs explicitly re-exported from
src/index.ts - Domain failures are typed
Errorsubclasses (invalidgasLimitsurfaces asExternalServiceError; see inline) - Biome clean (
pnpm lint) (not run in this review; no style findings)
Verdict
Approved - Code looks good! The medium and low findings are non-blocking.
|
Claude finished @devin-ai-integration[bot]'s task in 2m 40s —— View job Review of PR #1303: approved, 4 non-blocking findings
The two findings from the last review are fixed in Findings: 0 critical · 0 high · 1 medium · 3 low
I did not run lint or tests in this review. |
There was a problem hiding this comment.
Code Review Summary
Overview
This PR adds two optional ChainSimulationConfig settings. blockOverrides.gasLimit is sent as blockOverrides.gasLimit only when it is set, and resolveChain checks it before any RPC call, throwing SimulationValidationError for an invalid value. parentHashCheck resolves to chain.parentHashCheck ?? chainId !== StableMainnet. The findings from the previous review are fixed in ec64039 and 86f1273. The change is small and well tested, the request body is unchanged when the gas limit is unset, and the block number, timestamp and reorg checks still run when the parentHash check is off.
The main remaining gap is that THREAT_MODEL.md still describes the parentHash check as unconditional. The other three findings are low-severity follow-ups.
Findings
- Critical: 0 issues
- High: 0 issues
- Medium: 1 issue
- Low: 3 issues
See inline comments for details.
Guidelines Compliance
- Follows TypeScript strict mode
- Uses early returns over nested conditionals
-
bigintfor onchain quantities; WAD-scaled where appropriate - Reuses SDK types (
Address,MarketId,ChainId,BigIntish) - Type-only imports where possible
- Relative imports use
.jssuffix (NodeNext) - Public APIs explicitly re-exported from
src/index.ts - Domain failures are typed
Errorsubclasses - Biome clean (
pnpm lint) (not run in this review; no style findings from agents)
Verdict
Approved - Code looks good! The medium and low findings are non-blocking.
|
Claude finished @devin-ai-integration[bot]'s task in 2m 28s —— View job Review of PR #1303: approved, 2 non-blocking findings
All findings from the two earlier reviews are fixed in Findings: 0 critical · 0 high · 1 medium · 1 low
The scope filter dropped one more finding because the agent cited a line outside the file: a I did not run lint or tests in this review. |
There was a problem hiding this comment.
Code Review Summary
Overview
This PR adds two optional ChainSimulationConfig settings: blockOverrides.gasLimit, sent as eth_simulateV1 blockOverrides.gasLimit only when set, and parentHashCheck, which defaults to chain.parentHashCheck ?? chainId !== StableMainnet. All findings from the two earlier reviews are fixed:
resolveChainnow checksgasLimitandparentHashCheckat runtime and throwsSimulationValidationErrorfor an invalid value.THREAT_MODEL.mdrecords the weaker block binding with the check off as an accepted residual.executePlanhas tests that coverparentHashCheck: false, including the reorg re-fetch.- The error messages quote the rejected value and say what to pass instead.
The change is small and well tested. Two non-blocking findings remain:
- The public
simulate()JSDoc does not list theparentHashCheckvalidation added in the last commit. - The new
fieldErrorsentries don't follow the package's"<path>: <reason>"format.
Findings
- Critical: 0 issues
- High: 0 issues
- Medium: 1 issue
- Low: 1 issue
See inline comments for details.
Dropped by the scope filter (1)
resolve-chain.ts(line-pre-existing, cited line 389): a non-objectblockOverridesfrom untyped config is ignored instead of rejected. The agent cited a line outside the file, so the finding was dropped. It is very low impact.
Guidelines Compliance
- Follows TypeScript strict mode
- Uses early returns over nested conditionals
-
bigintfor onchain quantities; WAD-scaled where appropriate - Reuses SDK types (
Address,MarketId,ChainId,BigIntish) - Type-only imports where possible
- Relative imports use
.jssuffix (NodeNext) - Public APIs explicitly re-exported from
src/index.ts - Domain failures are typed
Errorsubclasses - Biome clean (
pnpm lint) (not run in this review; agents found no style issues)
Verdict
Approved - Code looks good! The medium and low findings are non-blocking.
|
Claude finished @devin-ai-integration[bot]'s task in 2m 12s —— View job Review of PR #1303: approved, no findings
The agents found no issues. Both findings from the last review are fixed:
The fixes from the earlier reviews are still in place. Findings: 0 critical · 0 high · 0 medium · 0 low I did not run lint or tests in this review. Sentinel: REVIEW_DONE_PR — PR #1303, 0 findings, mode=CI, commit=9928a3e |
There was a problem hiding this comment.
Code Review Summary
Overview
This PR adds two optional ChainSimulationConfig settings to evm-simulation:
blockOverrides.gasLimitis sent asblockStateCalls[0].blockOverrides.gasLimitonly when it is set, so the request body is unchanged when it is unset.parentHashCheckresolves tochain.parentHashCheck ?? chainId !== ChainId.StableMainnet. With the check off, the block number, timestamp and reorg re-fetch checks still run.
resolveChain checks both values before any RPC call. The findings from the three earlier reviews are all fixed:
fieldErrorsentries now use the package's"<path>: <reason>"format.- The
simulate()@throwslist and@param configdescription now cover both new options and their validation. - The THREAT_MODEL residual, the error message wording and the
executePlantests withparentHashCheck: falsewere fixed earlier and are still in place.
Review scope: 15 files, 8c3f02a..9928a3e. 9 agents ran (8 baseline + ci-release-security) and none failed.
No issues found in this review.
Findings
- Critical: 0 issues
- High: 0 issues
- Medium: 0 issues
- Low: 0 issues
Guidelines Compliance
- Follows TypeScript strict mode
- Uses early returns over nested conditionals
-
bigintfor onchain quantities; WAD-scaled where appropriate - Reuses SDK types (
Address,MarketId,ChainId,BigIntish) - Type-only imports where possible
- Relative imports use
.jssuffix (NodeNext) - Public APIs explicitly re-exported from
src/index.ts - Domain failures are typed
Errorsubclasses - Biome clean (
pnpm lint) (not run in this review; agents found no style issues)
Verdict
Approved - Code looks good!

Summary
Adds two optional settings to
ChainSimulationConfigfor chains whoseeth_simulateV1doesn't behave like geth, such as Stable (988).blockOverrides.gasLimit: when set, it is sent asblockStateCalls[0].blockOverrides.gasLimit(numberToHex). When unset,blockOverridesis left out and the request is byte-for-byte what it was before. Individual calls never get agasfield. The SDK has no Stable-specific gas limit; the consumer chooses the value (VVRM uses 2^24). A value that isn't a positive bigint throwsSimulationValidationErrorinresolveChain, before any RPC call, so a misconfiguration can't surface as a bypassableExternalServiceError.parentHashCheck: controls whether a successor block'sparentHashmust equal the pinned block hash. It resolves aschain.parentHashCheck ?? chainId !== ChainId.StableMainnet: off by default on Stable (its nodes report aparentHashthat never matches) and on everywhere else. Settingtrueorfalseoverrides the default on any chain. With the check off, the block number, timestamp, pinned-block re-fetch and reorg checks still run.Tests cover: the request with and without
gasLimit(no per-callgaseither way), gas limit validation, the default and explicitparentHashCheckresolution, a mismatched or missingparentHashaccepted when the check is off and rejected when it's on, and a bad block number or timestamp rejected even with the check off. The README, AGENTS.md and a minor changeset are updated.Consumer: morpho-org/morpho-apps#7318 (needs this released before it can merge).
Closes SDK-1379
Link to Devin session: https://app.devin.ai/sessions/3112f350458242bcb81bba539ccee9ae
Open in Devin Desktop: https://app.devin.ai/desktop/session/3112f350458242bcb81bba539ccee9ae?variant=devin
Requested by: @Foulks-Plb