-
Notifications
You must be signed in to change notification settings - Fork 105
feat: add gasFeeCeiling to transaction overrides and handling logic #905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughA new optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant Worker
participant Blockchain
User->>API: Submit transaction with txOverrides (may include gasFeeCeiling)
API->>Worker: Queue transaction (includes gasFeeCeiling in overrides)
Worker->>Worker: Estimate gas cost
alt Estimated cost > gasFeeCeiling
Worker->>Worker: Delay job (moveToDelayed, throw DelayedError)
else Estimated cost <= gasFeeCeiling
Worker->>Blockchain: Send transaction
Blockchain-->>Worker: Transaction result
end
Worker-->>API: Update transaction status
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/server/schemas/tx-overrides.ts(1 hunks)src/server/utils/transaction-overrides.ts(1 hunks)src/shared/db/transactions/queue-tx.ts(1 hunks)src/shared/utils/transaction/types.ts(1 hunks)src/worker/tasks/send-transaction-worker.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/worker/tasks/send-transaction-worker.ts (1)
Learnt from: d4mr
PR: thirdweb-dev/engine#897
File: src/server/routes/backend-wallet/transfer.ts:15-0
Timestamp: 2025-06-09T23:37:07.373Z
Learning: The `getChecksumAddress` function in this codebase safely handles undefined/null values by accepting `val?: string | Address | null` and returning `Address | undefined`. It returns `undefined` when passed falsy values, so calling it on potentially undefined values like `accountFactoryAddress` does not cause runtime errors.
🧬 Code Graph Analysis (3)
src/server/utils/transaction-overrides.ts (1)
src/shared/utils/primitive-types.ts (1)
maybeBigInt(5-5)
src/server/schemas/tx-overrides.ts (1)
src/server/schemas/number.ts (1)
WeiAmountStringSchema(8-11)
src/worker/tasks/send-transaction-worker.ts (1)
src/shared/utils/error.ts (1)
wrapError(5-6)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
src/shared/db/transactions/queue-tx.ts (1)
26-26: LGTM! Clean interface addition.The addition of the optional
gasFeeCeilingproperty is consistent with other gas-related properties in the interface and aligns with the PR's objective to add gas fee ceiling support.src/server/utils/transaction-overrides.ts (1)
23-23: LGTM! Consistent parsing implementation.The
gasFeeCeilingparsing follows the established pattern usingmaybeBigInt, which safely handles the string-to-BigInt conversion and is consistent with other gas-related properties.src/shared/utils/transaction/types.ts (1)
48-48: LGTM! Proper type definition.The optional
gasFeeCeilingproperty withbiginttype is consistent with the parser output and follows the same pattern as other gas-related properties in the overrides object.src/server/schemas/tx-overrides.ts (1)
27-31: LGTM! Well-documented schema addition.The
gasFeeCeilingschema is properly defined usingWeiAmountStringSchemaand includes an excellent description that clearly explains the delay behavior and clarifies that it doesn't affect actual gas fee calculation.src/worker/tasks/send-transaction-worker.ts (4)
307-327: LGTM! Correct userOp gas fee ceiling implementation.The gasFeeCeiling logic for user operations correctly:
- Calculates total estimated cost using
maxFeePerGas * (callGasLimit + preVerificationGas + verificationGasLimit)- Delays the job for 5 minutes when estimated cost exceeds the ceiling
- Provides detailed logging with all relevant gas parameters
- Uses proper DelayedError handling for BullMQ
331-333: LGTM! Improved maxFeePerGas logic clarity.The refactoring improves readability by storing the override in a local variable and using clearer conditional logic. The comparison
unsignedUserOp.maxFeePerGas > overrideMaxFeePerGascorrectly delays when onchain fees exceed the override.
213-215: LGTM! Improved error message formatting.The multiline template literal formatting improves readability while maintaining the same error message content. The use of
wrapErrorensures consistent error handling.Also applies to: 238-240, 356-358, 383-385
550-552: LGTM! Cleaner transaction sending code.The single-line await expression is more concise while maintaining the same functionality.
| const estimatedCost = | ||
| unsignedUserOp.maxFeePerGas * | ||
| (unsignedUserOp.callGasLimit + | ||
| unsignedUserOp.preVerificationGas + | ||
| unsignedUserOp.verificationGasLimit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we do this elsewhere? might be useful to abstract into some helper and explain this formula
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't do it anywhere else, if/when we do I'll extract it out
Example log:
PR-Codex overview
This PR introduces a new optional parameter
gasFeeCeilingin various transaction-related files, enhancing transaction fee management by allowing jobs to be delayed if the estimated costs exceed this ceiling.Detailed summary
gasFeeCeilingas an optional parameter in:src/shared/db/transactions/queue-tx.tssrc/shared/utils/transaction/types.tsgasFeeCeilingin:src/server/utils/transaction-overrides.tssrc/server/schemas/tx-overrides.tssrc/worker/tasks/send-transaction-worker.ts(multiple instances)send-transaction-worker.ts.Summary by CodeRabbit
New Features
Refactor