feat(transactor): Cleanup retry policy and handle error cases

[calbera]

Summary by CodeRabbit

• New Features
  • Added health check interval configuration for Ethereum client connections.
  • Introduced a new retry policy for transactions, including exponential backoff strategy for retries.
• Bug Fixes
  • Fixed a bug in job manager to handle cases when jitter is nil before calculating task retry sleep duration.
  • Enhanced transaction sending and tracking with improved retry mechanism and error handling.
• Refactor
  • Updated health check client creation to support custom health check intervals.
  • Refined transaction retry logic, moving from a default policy to more flexible policy implementations.

[coderabbitai]

Warning

Rate Limit Exceeded

@calbera has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 12 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits

Files that changed from the base of the PR and between 7f562ad and a1d1c18.

Walkthrough

The recent changes aim to fortify transaction handling and health checking in an Ethereum client app. By introducing a new retry policy, customizing health check intervals, and refining error handling, the system gains resilience and adaptability to network fluctuations and operational needs.

Changes

Files	Change Summary
client/eth/connection_pool.go	Added HealthCheckInterval to ConnectionPoolConfig and updated NewHealthCheckedClient calls.
core/transactor/sender/retry.go core/transactor/sender/sender.go	Introduced RetryPolicy interface, updated retry logic in Sender, and enhanced retry mechanism.

🐇💻
Changes afoot in the code we weave,
Making our Ethereum client believe,
In retries and checks, more robust each day,
A hop, skip, and jump, we're on our way!
🌟🚀

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[coderabbitai]

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between ab5568c and 00e0c10.

Files selected for processing (6)

• baseapp/job_manager.go (1 hunks)
• client/eth/connection_pool.go (2 hunks)
• client/eth/health_client.go (1 hunks)
• core/transactor/sender/retry.go (1 hunks)
• core/transactor/sender/sender.go (3 hunks)
• core/transactor/transactor.go (3 hunks)

Additional comments: 9

client/eth/health_client.go (1)

• 22-27: The addition of healthCheckInterval parameter in NewHealthCheckedClient function enhances flexibility in setting health check intervals. This is a positive change that aligns with the PR's objectives to improve the robustness of Ethereum client connections.

core/transactor/sender/retry.go (1)

• 47-48: Handling of nil jitter by assigning a new big.Int value is a good safety measure to avoid potential nil pointer dereferences. This aligns with the PR's objective of code cleanup and optimization.

client/eth/connection_pool.go (1)

• 31-42: The addition of HealthCheckInterval to ConnectionPoolConfig and its initialization in DefaultConnectPoolConfig function are positive changes that enhance the flexibility and configurability of health check intervals for Ethereum client connections. This aligns with the PR's objectives to improve connection stability.

core/transactor/sender/sender.go (3)

• 37-40: Initialization of Sender with an updated retry policy using NewExponentialRetryPolicy is a significant enhancement that aligns with the PR's objectives to improve the retry mechanism. This change introduces a more sophisticated and flexible retry strategy.
• 52-54: The delegation of retry logic to retryTxWithPolicy in the SendTransaction method is a good architectural decision that centralizes and simplifies the retry mechanism. This enhances maintainability and aligns with the PR's objectives.
• 92-96: The modifications in the OnError method to handle specific error cases, such as nonce too low, and delegate retrying transactions to retryTxWithPolicy enhance the error handling mechanism. This is in line with the PR's objectives to improve transaction management.

core/transactor/transactor.go (2)

• 86-90: The modification to SubscribeTxResults method to take only a tracker.Subscriber as an argument simplifies the interface and enhances usability. This change is a positive improvement towards making the system more robust.
• 159-161: Enhanced logging in the sendAndTrack method to include message IDs when an error occurs provides better context on errors. This improvement aligns with the PR's objectives to refine logging for better error management.

baseapp/job_manager.go (1)

• 257-259: Handling of nil jitter by assigning a new big.Int value before calculating the sleep duration for retrying a task is a good safety measure. This change aligns with the PR's objectives to optimize code and handle edge cases effectively.

[coderabbitai]

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 00e0c10 and 03a483e.

Files selected for processing (3)

• client/eth/connection_pool.go (2 hunks)
• core/transactor/sender/retry.go (1 hunks)
• core/transactor/sender/sender.go (3 hunks)

Files skipped from review as they are similar to previous changes (2)

• client/eth/connection_pool.go
• core/transactor/sender/retry.go

Additional comments: 4

core/transactor/sender/sender.go (4)

• 37-40: The initialization of Sender with a NewExponentialRetryPolicy in the New function is a positive change, aligning with the PR's objective to enhance retry mechanisms. This ensures a more sophisticated approach to retrying transactions.
• 52-54: The error handling in SendTransaction now properly logs the failure and delegates retry logic to retryTxWithPolicy. This separation of concerns improves maintainability and aligns with the PR's goal for improved error handling. However, ensure that retryTxWithPolicy is designed to handle all possible errors appropriately, including those that should not trigger a retry.
• 98-114: The retryTx method's approach to retrying transactions with new gas and nonce values is clear and follows best practices by logging the retry attempt and handling errors appropriately. However, ensure that the new gas price calculation aligns with the network's current conditions to avoid underpricing or overpricing transactions.
• 128-147: The handleNonceTooLow method correctly identifies and handles the ErrNonceTooLow error by building and assigning a new transaction. This is a crucial step in ensuring that transactions do not fail due to nonce issues. However, ensure that the new transaction's parameters (gas price, nonce, etc.) are appropriately set to reflect the current network conditions and the intended transaction behavior.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 03a483e and 7f562ad.

Files selected for processing (2)

• core/transactor/factory/factory.go (1 hunks)
• core/transactor/sender/sender.go (3 hunks)

Additional comments: 8

core/transactor/factory/factory.go (1)

• 115-115: The change to use ethClient.EstimateGas for determining the gas limit dynamically is a positive improvement for transaction efficiency and cost-effectiveness. However, ensure that ethereum.CallMsg(*txReq) correctly represents the intended transaction, as inaccuracies in the CallMsg could lead to incorrect gas estimations.

core/transactor/sender/sender.go (7)

• 37-40: Initialization of the Sender struct with default and new retry policies in the New function is clear and follows the PR objectives of enhancing retry mechanisms. Using NewExponentialRetryPolicy() for the retry policy is a good choice for more sophisticated retry logic.
• 52-54: Logging the error when a transaction fails to send is crucial for debugging. However, ensure that sensitive information is not being logged, especially in production environments, to avoid potential security risks.
• 78-78: The OnStale method's implementation to retry transactions aligns with the objective of improving transaction management. It's important to monitor the impact of this change on system performance, especially in high-traffic scenarios.
• 85-95: The OnError method's enhancements for handling errors and retrying transactions are well-implemented. However, consider adding more detailed logging for each step of the error handling process to improve traceability and debugging.
• 97-111: The retryTxWithPolicy method introduces a loop for retrying transactions based on the retry policy. Ensure that there are safeguards in place to prevent infinite loops or excessive retries, which could lead to resource exhaustion.
• 113-130: The retryTx method's logic for replacing and resending transactions is crucial for the retry mechanism. Ensure that the new gas and nonce values are calculated correctly to avoid transaction failures. Additionally, consider handling potential errors from SendTransaction recursively called at the end of retryTx.
• 132-154: Handling the ErrNonceTooLow error by building and replacing the transaction in handleNonceTooLow is a good strategy. Ensure that the new transaction parameters are correctly set to prevent the same error from recurring.

[calbera]

Why is the string.Contains() for error equality checking necessary?

[itsdevbear]

Overall looks good to me utACK