Add fallback for gas fee estimation in zkSync #8637
Conversation
The getZkGasFees function now falls back to standard EVM gas estimation methods when the zks_estimateFee RPC method is unavailable. This improves compatibility with zkSync chains that do not support zks_estimateFee and ensures transactions can still be processed. Corresponding tests have been added to verify the fallback logic.
🦋 Changeset detectedLatest commit: 9522486 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a fallback path to Changes
Sequence DiagramsequenceDiagram
participant Test as Test/Caller
participant getZkGasFees as getZkGasFees()
participant ZkSyncRPC as zkSync RPC (zks_estimateFee)
participant EVM as EVM Estimation (estimateGas)
participant GasData as Gas Data Modules
Test->>getZkGasFees: call(transaction, from)
getZkGasFees->>ZkSyncRPC: request zks_estimateFee
alt zkSync RPC responds
ZkSyncRPC-->>getZkGasFees: zk gas + fees
getZkGasFees-->>Test: return zkSync-formatted fees
else RPC fails/error
getZkGasFees->>EVM: dynamically import & call estimateGas
getZkGasFees->>GasData: dynamically import fee data modules
EVM-->>getZkGasFees: gas estimate
GasData-->>getZkGasFees: fee data
getZkGasFees-->>getZkGasFees: compute fallback fees (gasPerPubdata=100000n)
getZkGasFees-->>Test: return fallback fees
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.ts (2)
206-206: Consider logging or categorizing the caught error for debugging.The empty catch swallows all errors, including potential network issues or malformed responses. While the fallback behavior is correct, silently failing without any logging could make debugging difficult in production.
💡 Optional improvement to add minimal error context
- } catch { + } catch (error) { // Fallback to standard EVM methods if zks_estimateFee is not available + // Intentionally catching all errors - zks_estimateFee may not be supported
213-220: Pre-defined gas values get doubled, which may be unexpected.When the user specifies
gasbut notmaxFeePerGas, the function enters the estimation block. The fallback path reuses the user's gas value but still multiplies it by 2 (line 220). This is consistent with the zkSync path behavior but could surprise callers who expect their explicit gas value to be preserved.Consider documenting this behavior or only applying the multiplier when gas was actually estimated:
♻️ Proposed fix to preserve user-specified gas
const [estimatedGas, gasOverrides] = await Promise.all([ gas === undefined ? estimateGas({ transaction, from }) : Promise.resolve(gas), getDefaultGasOverrides(transaction.client, transaction.chain), ]); - gas = estimatedGas * 2n; // overestimating similar to zkSync estimation + // Only apply multiplier if we actually estimated gas + gas = gas === undefined ? estimatedGas * 2n : estimatedGas;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.tspackages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each TypeScript file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes in TypeScript
Avoidanyandunknownin TypeScript unless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.) in TypeScript
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity and testability
Re-use shared types from@/typesor local types.ts barrel exports
Prefer type aliases over interface except for nominal shapes
Avoid any and unknown unless unavoidable; narrow generics whenever possible
Choose composition over inheritance; leverage utility types (Partial, Pick, etc.)
Comment only ambiguous logic in TypeScript files; avoid restating TypeScript types and signatures in prose
Files:
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.tspackages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Place tests alongside code:foo.ts↔foo.test.tsin the same directory
Use real function invocations with stub data in tests; avoid brittle mocks
Use Mock Service Worker (MSW) for fetch/HTTP call interception in tests
Keep tests deterministic and side-effect free
Use predefined test accounts fromtest/src/test-wallets.tsin tests
UseFORKED_ETHEREUM_CHAINfor mainnet interactions andANVIL_CHAINfor isolated tests
**/*.test.{ts,tsx}: Co-locate tests with source files using the pattern foo.ts ↔ foo.test.ts
Use real function invocations with stub data in tests; avoid brittle mocks
For network interactions in tests, use Mock Service Worker (MSW) to intercept fetch/HTTP calls, mocking only scenarios that are hard to reproduce
Keep tests deterministic and side-effect free; Vitest is pre-configured
Files:
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.ts
packages/thirdweb/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
packages/thirdweb/src/**/*.{ts,tsx}: Comment only ambiguous logic in SDK code; avoid restating TypeScript in prose
Load heavy dependencies inside async paths to keep initial bundle lean (e.g.const { jsPDF } = await import("jspdf");)Lazy-load heavy dependencies inside async paths to keep the initial bundle lean (e.g., const { jsPDF } = await import('jspdf');)
Files:
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.tspackages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Biome governs formatting and linting; its rules live in biome.json. Run
pnpm fix&pnpm lintbefore committing, ensure there are no linting errors
Files:
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.tspackages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lazy-import optional features; avoid top-level side-effects
Files:
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.tspackages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.ts
🧬 Code graph analysis (2)
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.ts (2)
packages/thirdweb/src/exports/thirdweb.ts (1)
prepareTransaction(181-181)packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.ts (1)
getZkGasFees(164-244)
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.ts (1)
packages/thirdweb/src/gas/fee-data.ts (1)
getDefaultGasOverrides(127-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Lint Packages
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.ts (2)
221-235: LGTM!The fallback gas fee handling correctly accommodates both EIP-1559 and legacy fee structures from
getDefaultGasOverrides. The 2x multiplier maintains consistency with the zkSync path for fast inclusion.
183-243: Good implementation of fallback mechanism with lazy imports.The fallback logic correctly handles the case when
zks_estimateFeeis unavailable by falling back to standard EVM estimation methods. The dynamic imports (import("../estimate-gas.js")andimport("../../../gas/fee-data.js")) follow the coding guidelines for lazy-loading heavy dependencies in async paths.packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.ts (1)
8-14: LGTM!The new imports for
defineChainandgetZkGasFeesare appropriate for testing the fallback mechanism.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.changeset/spotty-experts-rest.md:
- Line 5: Update the changeset sentence "newer zkos chains no longer support
zks\_ rpc endpoints, fallback to evm std" to a clear, correctly spelled
sentence: capitalize and correct "zkos" to "zkSync", remove the unnecessary
backslash in "zks\_", and replace "evm std" with "the EVM standard" (e.g.,
"Newer zkSync chains no longer support zks RPC endpoints; fall back to the EVM
standard."). Also add a short clarifying phrase if helpful (for example, what
component falls back or why) to make the intent explicit.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/spotty-experts-rest.mdpackages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/thirdweb/src/transaction/actions/zksync/send-eip712-transaction.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Lint Packages
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "thirdweb": patch | ||
| --- | ||
|
|
||
| newer zkos chains no longer support zks\_ rpc endpoints, fallback to evm std |
There was a problem hiding this comment.
Fix typos and improve clarity in the changeset description.
The description contains several issues:
- "zkos" should be "zkSync"
- Escaped underscore in "zks_" may be unnecessary
- "evm std" should be "EVM standard"
- Missing article and could be more descriptive
📝 Proposed fix for clarity
-newer zkos chains no longer support zks\_ rpc endpoints, fallback to evm std
+Newer zkSync chains no longer support zks_ RPC endpoints; fall back to the EVM standard📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| newer zkos chains no longer support zks\_ rpc endpoints, fallback to evm std | |
| Newer zkSync chains no longer support zks_ RPC endpoints; fall back to the EVM standard |
🤖 Prompt for AI Agents
In @.changeset/spotty-experts-rest.md at line 5, Update the changeset sentence
"newer zkos chains no longer support zks\_ rpc endpoints, fallback to evm std"
to a clear, correctly spelled sentence: capitalize and correct "zkos" to
"zkSync", remove the unnecessary backslash in "zks\_", and replace "evm std"
with "the EVM standard" (e.g., "Newer zkSync chains no longer support zks RPC
endpoints; fall back to the EVM standard."). Also add a short clarifying phrase
if helpful (for example, what component falls back or why) to make the intent
explicit.
size-limit report 📦
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8637 +/- ##
==========================================
+ Coverage 52.97% 53.06% +0.08%
==========================================
Files 932 932
Lines 62270 62298 +28
Branches 4096 4105 +9
==========================================
+ Hits 32987 33057 +70
+ Misses 29184 29142 -42
Partials 99 99
🚀 New features to boost your workflow:
|
The getZkGasFees function now falls back to standard EVM gas estimation methods when the zks_estimateFee RPC method is unavailable. This improves compatibility with zkSync chains that do not support zks_estimateFee and ensures transactions can still be processed. Corresponding tests have been added to verify the fallback logic.
PR-Codex overview
This PR focuses on enhancing the
sendEip712Transactionfunctionality to support fallback mechanisms for gas fee estimation on zkSync chains that do not supportzks_estimateFee, ensuring robust transaction processing.Detailed summary
thirdwebpackage version in.changeset/spotty-experts-rest.md.defineChainimport insend-eip712-transaction.test.ts.send-eip712-transaction.tsto implement a try-catch for fee estimation, falling back to standard methods ifzks_estimateFeefails.Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.