-
Notifications
You must be signed in to change notification settings - Fork 83
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
chore(ethereum-storage): add getter for eip-1559 support #1466
chore(ethereum-storage): add getter for eip-1559 support #1466
Conversation
WalkthroughThe changes introduce a new method named Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EthereumTransactionSubmitter
User->>EthereumTransactionSubmitter: Call supportsEip1559()
EthereumTransactionSubmitter-->>User: Return enableEip1559 status
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 (
|
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- packages/ethereum-storage/src/ethereum-tx-submitter.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/ethereum-storage/src/ethereum-tx-submitter.ts (1)
78-80
: Summary: Changes meet PR objectives with minor suggestions for improvement.The addition of the
supportsEip1559()
method successfully addresses the PR's main objective of providing a getter for EIP-1559 support. The implementation is correct, efficient, and consistent with the existing code structure.To further enhance the code quality:
- Consider adding JSDoc comments for better documentation.
- Implement unit tests to ensure the new method's reliability.
Overall, this change improves the
EthereumTransactionSubmitter
class by allowing users to easily check EIP-1559 support without unnecessary RPC calls, which is particularly useful for startup health checks as mentioned in the PR description.
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: ASSERTIVE
📒 Files selected for processing (1)
- packages/ethereum-storage/src/ethereum-tx-submitter.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/ethereum-storage/src/ethereum-tx-submitter.ts (1)
78-81
: LGTM: New method successfully implements the getter for EIP-1559 support.The
supportsEip1559()
method correctly returns theenableEip1559
property, providing an efficient way to check EIP-1559 support without making unnecessary RPC calls. This implementation aligns well with the PR objectives.
Description of the changes
When using the
EthereumTransactionSubmitter
as a standalone class, it is helpful to know whether it supports EIP-1559, for instance, during a service startup's health checks to ensure the underlying RPC provides the feature.Current solutions
Currently, the only two options are the following:
Current solution 1
This does not test the actual support of EIP-1559 for the
EthereumTransactionSubmitter
class:isEip1559Supported()
can returnfalse
during the initialization phase withEthereumTransactionSubmitter.initialize()
because the RPC times out, for instance ;true
when we test it by calling againisEip1559Supported()
.In this case we're stuck with a
EthereumTransactionSubmitter
that doesn't support EIP-1559 even thought the RPC supports it.Current solution 2
This works but it wastes some RPC calls:
StorageFeeCollector
.Proposed change
Since we already have that information in the class, I propose to expose it.
Summary by CodeRabbit