feat(ethereum-storage): custom gas limit #1495
Conversation
WalkthroughThe changes in this pull request introduce a new optional property, Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
github-actions
bot
requested review from
aimensahnoun,
MantisClone,
sstefdev and
yomarion
alexandre-abrioux
requested review from
leoslr,
SPeshov,
PaulvanMotman and
kevindavee
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/ethereum-storage/test/ethereum-tx-submitter.test.ts (2)
61-72: LGTM! Consider documenting the gas limit value.The test correctly verifies custom gas limit functionality. However, consider adding a comment explaining why 1000000 was chosen as the test value.
it('can use a custom gas limit', async () => { + // Using 1M gas as a reasonable upper limit for testing + const TEST_GAS_LIMIT = 1000000; const txSubmitterWithGasLimit = new EthereumTransactionSubmitter({ network: 'private', signer, - gasLimit: BigNumber.from(1000000), + gasLimit: BigNumber.from(TEST_GAS_LIMIT), }); const sendTransactionSpy = jest.spyOn(signer, 'sendTransaction'); await txSubmitterWithGasLimit.submit('hash', 1); expect(sendTransactionSpy).toHaveBeenCalledWith( - expect.objectContaining({ gasLimit: BigNumber.from(1000000) }), + expect.objectContaining({ gasLimit: BigNumber.from(TEST_GAS_LIMIT) }), ); });
72-72: Consider adding tests for edge cases.To ensure robust gas limit handling, consider adding tests for:
- Invalid gas limits (negative values, zero)
- Extremely large gas limits
- Gas limit updates after initialization (if supported)
Example test structure:
it('should reject invalid gas limits', async () => { expect(() => new EthereumTransactionSubmitter({ network: 'private', signer, gasLimit: BigNumber.from(0) }) ).toThrow(); }); it('should handle maximum allowed gas limit', async () => { const MAX_GAS = BigNumber.from('15000000'); // Ethereum block gas limit const txSubmitter = new EthereumTransactionSubmitter({ network: 'private', signer, gasLimit: MAX_GAS }); // ... test implementation });packages/ethereum-storage/src/ethereum-tx-submitter.ts (2)
53-53: Consider adding validation for gasLimit.While the implementation is correct, consider adding validation to ensure the gasLimit is a reasonable value (not too low or too high) to prevent potential issues with transaction failures.
constructor({ network, signer, logger, gasPriceMin, gasPriceMax, gasPriceMultiplier, gasLimit, debugProvider, }: SubmitterProps) { + if (gasLimit && gasLimit.lt(BigNumber.from(21000))) { + throw new Error('Gas limit must be at least 21000'); + } // ... existing code ... this.gasLimit = gasLimit; // ... existing code ... }Also applies to: 70-70
Line range hint
25-113: Consider adding monitoring and documentation for gas usage patterns.The addition of custom gas limits is a good security and performance enhancement. Consider:
- Adding metrics/monitoring for gas usage patterns to help users optimize their gas limits
- Documenting recommended gas limits for common operations in the README
- Adding warning logs when gas usage approaches the custom limit
This will help users make informed decisions about setting appropriate gas limits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/ethereum-storage/src/ethereum-tx-submitter.ts(6 hunks)packages/ethereum-storage/test/ethereum-tx-submitter.test.ts(1 hunks)
🔇 Additional comments (4)
packages/ethereum-storage/test/ethereum-tx-submitter.test.ts (1)
53-59: LGTM! Well-structured test for default behavior.
The test effectively verifies that gas limit remains undefined when not explicitly set, following good testing practices with clear setup and assertions.
packages/ethereum-storage/src/ethereum-tx-submitter.ts (3)
25-29: LGTM! Well-documented type definition.
The gasLimit property is properly typed as an optional BigNumber with clear documentation explaining both scenarios when it's set or unset.
44-44: LGTM! Properly encapsulated property.
The gasLimit property is correctly defined as private and readonly, maintaining proper encapsulation.
107-113: Verify gas estimation fallback behavior.
When gasLimit is undefined, the transaction will use the network's default gas estimation. Ensure this is sufficient for all possible transaction scenarios, especially for complex operations.
✅ Verification successful
Based on the search results, I can now generate the final response:
Gas limit handling is properly implemented with fallback behavior
The implementation correctly handles both cases:
- When
gasLimitis undefined, it's omitted from the transaction request, allowing the network's default gas estimation - When specified, the custom
gasLimitis used, as verified in the test cases
The code is safe because:
- The transaction will be automatically estimated by the provider when no gas limit is specified
- Tests confirm both scenarios work: with and without gas limit (
ethereum-storage/test/ethereum-tx-submitter.test.ts) - The contract interaction is a simple hash submission, which has predictable gas costs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for submitHash function implementations to analyze gas requirements
ast-grep --pattern 'function submitHash($$$) {
$$$
}'
# Search for existing gas usage patterns
rg -A 5 'gasLimit'
Length of output: 12857
Description of the changes
In this PR I propose adding a
gasLimitoption to ourEthereumTransactionSubmitterclass. This would allow builders to configure a gas limit for submitted transactions, in order to improve:RequestOpenHashSubmittersmart contract (low probability but could happen during an update for instance), funds on the submission wallet could be drained. This option would prevent that.EthereumTransactionSubmitterwhen using Alchemy. Setting this option would cancel that cost and slightly improve performance (one less network call).Summary by CodeRabbit
New Features
gasLimitproperty for transaction submissions, allowing users to specify a gas limit.Bug Fixes