Uh oh!
There was an error while loading. Please reload this page.
feat: add explicit gas override to writeContract/deployContract - #205
feat: add explicit gas override to writeContract/deployContract#205ygd58 wants to merge 2 commits into
Conversation
An exact eth_estimateGas result can itself cause the outer addTransaction EVM transaction to revert before GenVM is reached — confirmed on Bradbury (#402): gas limit 1,319,997 (== a fresh eth_estimateGas result) reverted twice; replaying identical calldata with 2,000,000 succeeded and finalized normally. No caller-side way existed to work around a bad estimate. Note: this repo's automatic gas headroom (withTransactionGasHeadroom, a 2x margin) already covers the specific numbers in #402's repro, but it isn't published to npm yet — I checked the actual genlayer-js@1.1.8 tarball from the registry and confirmed the headroom code isn't in it. This change adds the second, complementary part of #402's ask: a way to bypass estimation entirely for cases the headroom doesn't cover. Added an optional `gas?: bigint` field to writeContract and deployContract. When provided, it's used exactly as given — no eth_estimateGas call, no headroom markup — since an explicit override is the caller stating what to use. Both local-account (signTransaction) and external-wallet (eth_sendTransaction) send paths respect it; both flow through the same shared `_sendTransaction`/`sendWithEncodedData` helper that writeContract and deployContract already share, so there was a single spot to plumb this through for both. Tests: added 4 cases to tests/contracts-actions.test.ts using the existing setupWriteContractHarness — override bypasses estimation entirely, no-override path still estimates+applies headroom (asserting the exact 2,639,994 = 1,319,997 * 2x from #402's numbers), and the override threads through deployContract the same way. Full suite: 87 passed. eslint: clean.
This PR targeted I retargeted it to |
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe contract client now accepts optional explicit gas limits for ChangesGas override support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ContractAction as writeContract/deployContract
participant SendTransaction as _sendTransaction
participant EthEstimateGas as eth_estimateGas
participant SignedTransaction as signed transaction
ContractAction->>SendTransaction: pass optional gas override
alt gas override provided
SendTransaction->>SignedTransaction: use explicit gas unchanged
else gas override absent
SendTransaction->>EthEstimateGas: estimate gas
EthEstimateGas-->>SendTransaction: estimated gas
SendTransaction->>SignedTransaction: use estimated gas with 200% headroom
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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/contracts/actions.ts`:
- Around line 2062-2064: Update _sendTransaction to validate gasOverride before
transaction preparation, accepting only bigint values greater than 0n and
rejecting 0n or negative overrides with the existing validation error behavior.
Add coverage for both zero and negative gasOverride values.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8763eea8-2496-43ce-a29f-67486f2497bb
📒 Files selected for processing (3)
src/contracts/actions.tssrc/types/clients.tstests/contracts-actions.test.ts
| if (gasOverride !== undefined) { | ||
| estimatedGas = gasOverride; | ||
| } else { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject non-positive gas overrides.
A caller can pass 0n or a negative bigint. A negative value reaches the external-wallet request as an invalid value such as 0x-1. A zero value cannot execute an EVM transaction.
Validate gasOverride > 0n in _sendTransaction before transaction preparation. Add tests for 0n and a negative value.
Proposed fix
+ if (gasOverride !== undefined && gasOverride <= 0n) {+ throw new Error("gas must be greater than zero.");+ }+
const sendWithEncodedData = async (transactionVariant: EncodedTransactionVariant) => {🤖 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/contracts/actions.ts` around lines 2062 - 2064, Update _sendTransaction
to validate gasOverride before transaction preparation, accepting only bigint
values greater than 0n and rejecting 0n or negative overrides with the existing
validation error behavior. Add coverage for both zero and negative gasOverride
values.
_sendTransaction accepted any bigint for the gas override, including 0n or a negative value. Zero can't execute an EVM transaction, and a negative value would reach the external-wallet request as an invalid hex string like "0x-1". Added a check right alongside the existing early validations (consensus contract address, empty transaction variants) in _sendTransaction, before any transaction preparation happens. Added tests for both 0n and -1n, confirming the error is thrown before eth_estimateGas or signTransaction are ever called. Full suite: 89 passed. eslint: clean.
Summary
Adds the second part of #402's ask: an explicit
gasoverride forwriteContract/deployContract, bypassingeth_estimateGasentirely.Context
#402 reports that an exact
eth_estimateGasresult was used as the outer EVM gas limit on Bradbury, and the resulting transaction reverted twice — identical calldata succeeded when replayed with a larger explicit limit. No way existed to override the estimate.On the automatic-headroom part of the ask: this repo already has
withTransactionGasHeadroom(a 2x margin over the estimate), which would cover the exact numbers in #402's repro (1,319,997 estimated → 2,639,994 with headroom, comfortably above the 2,000,000 that #402 confirmed works). I checked the actualgenlayer-js@1.1.8tarball from the npm registry directly and confirmed this headroom code isn't in the published package yet — so that part of the fix exists but hasn't shipped. This PR is the complementary, still-needed part: a caller-side override for cases even the headroom doesn't cover, or for callers who just want deterministic control over gas.Change
Added an optional
gas?: biginttowriteContractanddeployContract's options. When provided:eth_estimateGasis not called at allsignTransaction) and external-wallet (eth_sendTransaction) send pathsBoth functions already funnel through a shared
_sendTransaction→sendWithEncodedDatahelper, so this was a single plumbing point for both.Testing
Added 4 cases to
tests/contracts-actions.test.ts, reusing the existingsetupWriteContractHarness:eth_estimateGasentirely and is used as-is2_639_994nvalue from #402's own numbers (1_319_997 * 20_000bps / 10_000)deployContractthe same way aswriteContractFull suite: 87 passed.
eslint: clean on all changed files.Summary by CodeRabbit
New Features
Tests