fix(cli): handle zero-baseFee chains and raise default gasLimit#860
Open
ryan-kinsey wants to merge 1 commit into
Open
fix(cli): handle zero-baseFee chains and raise default gasLimit#860ryan-kinsey wants to merge 1 commit into
ryan-kinsey wants to merge 1 commit into
Conversation
…flows
Two latent bugs in cli/src/evm/signer.ts surfaced during a Mainnet
NTT v2.0.0 deployment on Monad <-> BSC.
1. Default catch-all gasLimit was 500_000n. NttWithExecutor.transfer
bundles escrow + Wormhole publish + Executor relay instructions in
a single tx, using ~1.2-1.5M gas on mainnet EVM chains, so token
transfers reverted out-of-gas. Raised default to 3_000_000n;
opts.maxGasLimit override is preserved.
2. Chains with baseFeePerGas = 0 (BSC post-Lorentz, April 2025) cause
ethers' getFeeData() to return maxFeePerGas = 1 wei. The BSC
private-tx-service then rejects with:
[private transaction service] require GasPrice=50000000, Provide=1
The fix detects this shape by content (maxFeePerGas <
EIP1559_FEE_SANITY_FLOOR), not by chain name, and falls back to a
legacy (type 0) tx with a 1 gwei gasPrice floor (20x BSC's 0.05
gwei minimum). Forward-compatible with any future chain exhibiting
the same shape.
Refactor:
- Hoist DEFAULT_GAS_LIMIT, LEGACY_FALLBACK_GAS_PRICE, and
EIP1559_FEE_SANITY_FLOOR to module scope.
- Extract buildGasOpts() helper, typed Partial<TransactionRequest>
(replaces a previous gasOpts: any).
- Wrap getFeeData() in try/catch so a transient RPC failure no longer
fails the whole sign() call -- falls back to literal defaults.
- Remove stale "// TODO: DIFF STARTS/ENDS HERE" markers.
Tests: 10 new bun:test cases in cli/src/__tests__/evm-signer.test.ts
cover buildGasOpts (pure) and EvmNativeSigner.sign mocked end-to-end:
default 3M, opts.maxGasLimit override, Mantle/ArbitrumSepolia
branches, BSC legacy fallback with both floor-clamp and above-floor
preservation, Celo skipping getFeeData(), and getFeeData() throwing.
Follow-ups (out of scope for this PR):
- Re-enable the disabled estimateGas + buffer block (signer.ts:218)
-- the static 3M default is symptom suppression for the underlying
issue: the signer doesn't estimate.
- Plumb opts.maxGasLimit through cli/src/signers/getSigner.ts:96 (it
currently passes only { debug: true }, so the override is dead from
the CLI surface).
- Delete this whole wrapper in favour of upstream
@wormhole-foundation/sdk-evm signer post wormhole-sdk-ts PR wormhole-foundation#583;
the file header has carried this TODO for 22 months.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two latent bugs in
cli/src/evm/signer.ts(EvmNativeSigner.sign) reproduced during a Mainnet NTT v2.0.0 deployment on Monad ↔ BSC.gasLimitwas too small forNttWithExecutor.transfer. The flow bundles token escrow + Wormhole message publish + Executor relay instructions in a single transaction and realistically uses ~1.2–1.5M gas on mainnet EVM chains. The previous 500_000 default caused silent out-of-gas reverts on every token transfer.baseFeePerGas = 0(BSC post-Lorentz, April 2025). Ethers'getFeeData()returnsmaxFeePerGas = 1 weiand the BSC private-tx-service rejects with:Fix
Default gasLimit: 500_000n → 3_000_000n
Catch-all branch only. The
Mantle(2.6T) andArbitrumSepolia(4M) special cases are preserved. Theopts.maxGasLimitoverride is preserved; callers passing an explicit value continue to get exactly that value.Content-based legacy-tx fallback (not chain-name)
Instead of hard-coding
chain === "Bsc", the broken EIP-1559 shape is detected by content:This auto-covers any future chain (or buggy provider response) exhibiting the same shape — fixed-fee chains, Linea-style quirks, future zero-baseFee L2s, etc. The 1 gwei floor is 20× BSC's 0.05 gwei minimum and remains inexpensive everywhere it applies.
Other changes in this diff
DEFAULT_GAS_LIMIT,LEGACY_FALLBACK_GAS_PRICE,EIP1559_FEE_SANITY_FLOORto module scope (matches the existing convention used elsewhere in the CLI).buildGasOpts(gasLimit, feeData): Partial<TransactionRequest>helper — typed (the previousgasOptswas implicitlyany) and unit-testable in isolation.getFeeData()intry/catch. A transient RPC failure no longer fails the wholesign()call; it falls back to the literal defaults already defined in the function.// TODO: DIFF STARTS HERE / DIFF ENDS HEREmarkers (they wrapped the previous local divergence and no longer match the current shape).1000_000_000n→1_000_000_000n).Repro (without this PR)
Symptoms:
gasUsed == 500000,status == 0, no logs (out-of-gas).ntt pushlimit application,set-outbound-limit,transfer-ownership, etc.) fail at submission with the BSC private-tx-service error above.Test plan
New file:
cli/src/__tests__/evm-signer.test.ts(10 cases,bun:test).buildGasOpts(pure):maxFeePerGas≥ sanity floor — preserves all three fee fields,typeundefined.maxFeePerGas < 50_000_000n—type: 0,maxFeePerGas/maxPriorityFeePerGasabsent.gasPriceup to1_000_000_000n.gasPricewhen above the floor.maxFeePerGas == floorstays on EIP-1559 path).EvmNativeSigner.sign(mocked end-to-end):gasLimit = 3_000_000n, EIP-1559 fields, notype.opts.maxGasLimit = 42n→ capturedgasLimit === 42n.MantleandArbitrumSepoliakeep their specializedgasLimitregardless of override.maxFeePerGas: 1n) →type: 0,gasPrice: 1_000_000_000n, no 1559 fields.CeloskipsgetFeeData()and uses fallback defaults.getFeeData()throwing falls back to literal defaults instead of propagating.Local validation:
bun run typecheck:cli→ exit 0cd cli && bun test src/ __tests__/→ 166 pass, 2 skip, 0 fail across 15 filesprettier --check src/evm/signer.ts src/__tests__/evm-signer.test.ts→ cleanAlternatives considered
estimateGas + 20% buffer— arguably the root-cause fix. The block is already written and commented out insigner.ts(lines ~218–229) since 2024-06-27. Reviving it would remove the need for a staticgasLimitdefault entirely. Out of scope here because of the larger blast radius; left as a follow-up.NttWithExecutor's owngasLimitOverridesmap (evm/ts/src/nttWithExecutor.ts:80) — those values describe the relayer's quoted gas (paid to the executor), which is orthogonal to the source-chain tx'sgasLimit. Not applicable.chain === "Bsc"check — initial draft. Rejected because chain-name policy in a generic signer doesn't generalize; any other zero-baseFee chain (or buggy provider response) silently regresses. Content-based detection is one fewer code path per future chain.NTT_DEFAULT_GAS_LIMIT) — added complexity for a knob no current caller exercises (cli/src/signers/getSigner.ts:96passes only{ debug: true }). The existingopts.maxGasLimitplumbing is the right place; threading it through belongs in a follow-up.Backwards compatibility
bin: nttonly — nomain/exportsincli/package.json, soEvmNativeSigner/getEvmSignerare not part of any external API surface.opts.maxGasLimitcontinue to get exactly that value.type: 0instead oftype: 2. Anything outside this repo parsingeffectiveGasPrice/maxFeePerGasfrom BSC receipts produced by the CLI would see legacy-shaped receipts; no such consumer exists incli/.Follow-ups (intentionally out of scope)
estimateGas + 20% buffer(cli/src/evm/signer.ts:218); the static 3M default is symptom suppression and the underlying issue is that the signer doesn't estimate.opts.maxGasLimitthroughcli/src/signers/getSigner.ts:96(it currently passes only{ debug: true }, so the override has no CLI surface). Renaming todefaultGasLimitor making it a true upper-bound ceiling would also be useful.MAX_GAS_PRICEceiling so a malicious or misconfigured RPC returning an absurdgasPricecannot cause silent operator overpayment.cli/src/evm/signer.ts:9, this whole local copy of the SDK signer can likely be replaced by upstream@wormhole-foundation/sdk-evmafter wormhole-sdk-ts PR CLI: Installation of ntt cli fails #583. Worth a separate cleanup pass.Files in this PR
cli/src/evm/signer.ts— refactor + fix.cli/src/__tests__/evm-signer.test.ts— new, 10 cases.Diff stat: 2 files changed, 270 insertions(+), 20 deletions(-).