Skip to content

feat(evm-simulation): add block gas limit and parentHash check config (SDK-1379) - #1303

Merged
Foulks-Plb merged 7 commits into
mainfrom
devin/1791375356-evm-sim-block-gas-limit
Oct 7, 2026
Merged

Foulks-Plb merged 7 commits into
mainfrom
devin/1791375356-evm-sim-block-gas-limit

Conversation

@Foulks-Plb

@Foulks-Plb Foulks-Plb commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds two optional settings to ChainSimulationConfig for chains whose eth_simulateV1 doesn't behave like geth, such as Stable (988).

readonly blockOverrides?: { readonly gasLimit?: bigint };
readonly parentHashCheck?: boolean;
  • blockOverrides.gasLimit: when set, it is sent as blockStateCalls[0].blockOverrides.gasLimit (numberToHex). When unset, blockOverrides is left out and the request is byte-for-byte what it was before. Individual calls never get a gas field. 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 throws SimulationValidationError in resolveChain, before any RPC call, so a misconfiguration can't surface as a bypassable ExternalServiceError.
  • parentHashCheck: controls whether a successor block's parentHash must equal the pinned block hash. It resolves as chain.parentHashCheck ?? chainId !== ChainId.StableMainnet: off by default on Stable (its nodes report a parentHash that never matches) and on everywhere else. Setting true or false overrides 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-call gas either way), gas limit validation, the default and explicit parentHashCheck resolution, a mismatched or missing parentHash accepted 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

@devin-ai-integration

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@linear-code

linear-code Bot commented Oct 7, 2026

Copy link
Copy Markdown

SDK-1379

@Foulks-Plb
Foulks-Plb marked this pull request as ready for review October 7, 2026 12:28
@Foulks-Plb
Foulks-Plb requested a review from a team as a code owner October 7, 2026 12:28
@devin-ai-integration devin-ai-integration Bot changed the title feat(evm-simulation): add block gas limit config and skip parentHash on Stable (SDK-1379) feat(evm-simulation): add block gas limit and parentHash check config (SDK-1379) Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @Foulks-Plb's task in 2m 13s —— View job


Review of PR #1303: approved, 2 non-blocking findings

  • Check the CI environment, fetch the PR head and build the diff
  • Gather project context
  • Run 9 review agents (8 baseline + ci-release-security). None failed.
  • Combine findings and post the formal review

Findings: 0 critical · 0 high · 1 medium · 1 low

  • Medium, eth-simulate-v1.ts:197: numberToHex(blockGasLimit) runs inside the rpc() callback, so an invalid gasLimit (for example -1n) throws a bypassable ExternalServiceError instead of a typed validation error. A caller that skips on that error would skip simulation because of its own misconfiguration.
  • Low, simulate.ts:72: the @throws JSDoc still says parentHash is "not checked on Stable". That stopped being accurate once parentHashCheck became configurable.

I did not run lint or tests in this review.

@devin-ai-integration devin-ai-integration Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin resolved the finding on 2683a8f

Fixed by Devin (1)

  • Parent-hash documentation misses caller overrides

View all findings in Devin Review

Devin Review

@devin-ai-integration

Copy link
Copy Markdown
Contributor

/lupin review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  • bigint for onchain quantities; WAD-scaled where appropriate
  • Reuses SDK types (Address, MarketId, ChainId, BigIntish)
  • Type-only imports where possible
  • Relative imports use .js suffix (NodeNext)
  • Public APIs explicitly re-exported from src/index.ts
  • Domain failures are typed Error subclasses (invalid gasLimit surfaces as ExternalServiceError; 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.

Comment thread packages/evm-simulation/src/simulate/backends/eth-simulate-v1.ts
Comment thread packages/evm-simulation/src/simulate/simulate.ts Outdated
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Running review…

View job run

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @devin-ai-integration[bot]'s task in 2m 40s —— View job


Review of PR #1303: approved, 4 non-blocking findings

  • Check the CI environment and fetch the PR head (5c7462b)
  • Build the diff (14 files, 8c3f02a..5c7462b) and gather project context
  • Run 9 review agents (8 baseline + ci-release-security). None failed.
  • Combine findings and post the formal review

The two findings from the last review are fixed in ec64039 and 86f1273.

Findings: 0 critical · 0 high · 1 medium · 3 low

  • Medium, types.ts:23: THREAT_MODEL.md:121 still says every successor block must carry parentHash === stateBlockHash. With this PR the check is off by default on Stable and any caller can turn it off. The doc should say so and record the weaker link to the pinned block as an accepted residual.
  • Low, eth-simulate-v1.ts:226: no test calls executePlan with parentHashCheck: false, so nothing checks that the flag reaches the response parser.
  • Low, resolve-chain.ts:26: parentHashCheck isn't checked at runtime. An untyped config that passes 0 or "" would silently turn the check off.
  • Low, resolve-chain.ts:29: the gasLimit error message doesn't include the rejected value or say what to do instead.

I did not run lint or tests in this review.

0xbulma
0xbulma previously approved these changes Oct 7, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  • bigint for onchain quantities; WAD-scaled where appropriate
  • Reuses SDK types (Address, MarketId, ChainId, BigIntish)
  • Type-only imports where possible
  • Relative imports use .js suffix (NodeNext)
  • Public APIs explicitly re-exported from src/index.ts
  • Domain failures are typed Error subclasses
  • 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.

Comment thread packages/evm-simulation/src/types.ts
Comment thread packages/evm-simulation/src/simulate/backends/eth-simulate-v1.ts
Comment thread packages/evm-simulation/src/simulate/pipeline/resolve-chain.ts
Comment thread packages/evm-simulation/src/simulate/pipeline/resolve-chain.ts Outdated
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @devin-ai-integration[bot]'s task in 2m 28s —— View job


Review of PR #1303: approved, 2 non-blocking findings

  • Check the CI environment and fetch the PR head (328414e)
  • Build the diff (15 files, 8c3f02a..328414e) and gather project context
  • Run 9 review agents (8 baseline + ci-release-security). None failed.
  • Combine findings and post the formal review

All findings from the two earlier reviews are fixed in 328414e: the THREAT_MODEL residual, parentHashCheck runtime validation, the error message wording and the executePlan test with parentHashCheck: false.

Findings: 0 critical · 0 high · 1 medium · 1 low

  • Medium, simulate.ts:59: the public simulate() @throws {SimulationValidationError} list does not include the new parentHashCheck (not a boolean) check or the non-bigint gasLimit case. The @param config description also doesn't mention the two new per-chain options.
  • Low, resolve-chain.ts:31: the new fieldErrors entries are bare paths. The rest of the package uses the "<path>: <reason>" format.

The scope filter dropped one more finding because the agent cited a line outside the file: a blockOverrides that isn't an object is ignored instead of rejected.

I did not run lint or tests in this review.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • resolveChain now checks gasLimit and parentHashCheck at runtime and throws SimulationValidationError for an invalid value.
  • THREAT_MODEL.md records the weaker block binding with the check off as an accepted residual.
  • executePlan has tests that cover parentHashCheck: 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 the parentHashCheck validation added in the last commit.
  • The new fieldErrors entries 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-object blockOverrides from 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
  • bigint for onchain quantities; WAD-scaled where appropriate
  • Reuses SDK types (Address, MarketId, ChainId, BigIntish)
  • Type-only imports where possible
  • Relative imports use .js suffix (NodeNext)
  • Public APIs explicitly re-exported from src/index.ts
  • Domain failures are typed Error subclasses
  • 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.

Comment thread packages/evm-simulation/src/simulate/simulate.ts Outdated
Comment thread packages/evm-simulation/src/simulate/pipeline/resolve-chain.ts Outdated
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @devin-ai-integration[bot]'s task in 2m 12s —— View job


Review of PR #1303: approved, no findings

  • Check the CI environment and fetch the PR head (9928a3e)
  • Build the diff (15 files, 8c3f02a..9928a3e) and gather project context
  • Run 9 review agents (8 baseline + ci-release-security). None failed.
  • Combine findings and post the formal review

The agents found no issues.

Both findings from the last review are fixed:

  • fieldErrors now use the "<path>: <reason>" format.
  • The simulate() @throws list and @param config description now cover the gasLimit and parentHashCheck validation.

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Overview

This PR adds two optional ChainSimulationConfig settings to evm-simulation:

  • blockOverrides.gasLimit is sent as blockStateCalls[0].blockOverrides.gasLimit only when it is set, so the request body is unchanged when it is unset.
  • parentHashCheck resolves to chain.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:

  • fieldErrors entries now use the package's "<path>: <reason>" format.
  • The simulate() @throws list and @param config description now cover both new options and their validation.
  • The THREAT_MODEL residual, the error message wording and the executePlan tests with parentHashCheck: false were 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
  • bigint for onchain quantities; WAD-scaled where appropriate
  • Reuses SDK types (Address, MarketId, ChainId, BigIntish)
  • Type-only imports where possible
  • Relative imports use .js suffix (NodeNext)
  • Public APIs explicitly re-exported from src/index.ts
  • Domain failures are typed Error subclasses
  • Biome clean (pnpm lint) (not run in this review; agents found no style issues)

Verdict

Approved - Code looks good!

@Foulks-Plb
Foulks-Plb enabled auto-merge (squash) October 7, 2026 12:49
@Foulks-Plb
Foulks-Plb requested a review from 0xbulma October 7, 2026 12:49
@Foulks-Plb
Foulks-Plb merged commit 4a52357 into main Oct 7, 2026
28 checks passed
@Foulks-Plb
Foulks-Plb deleted the devin/1791375356-evm-sim-block-gas-limit branch October 7, 2026 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants