Skip to content
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.

Sign up for GitHub

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

Jump to bottom

WhaleBasedAccountManager with Retry Mechanism #278

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

WhaleBasedAccountManager with Retry Mechanism #278

wants to merge 6 commits into from

Conversation

nadeemb53
Copy link
Contributor

@nadeemb53 nadeemb53 commented Nov 4, 2024 •
edited
Loading

This PR closes #215, which reported intermittent failures in the coordinator's integration tests due to nonce-related and gas price errors. The errors included "Replacement transaction underpriced" and "Nonce too low," originating from the WhaleBasedAccountManager

Checklist

  • I wrote new tests for my new core changes.
  • I have successfully ran tests, style checker and build against my new changes locally.
  • I have informed the team of any breaking changes if there are any.

Note: Changes are not tested

@thedarkjester
Copy link
Collaborator

It looks like the coordinator tests are failing now, please have a look and adjust accordingly.

see: https://github.com/Consensys/linea-monorepo/actions/runs/11687214785/job/32693999670?pr=278

@nadeemb53
Copy link
Contributor Author

It looks like the coordinator tests are failing now, please have a look and adjust accordingly.

see: https://github.com/Consensys/linea-monorepo/actions/runs/11687214785/job/32693999670?pr=278

I've added a fix in this commit.

Please note that I haven’t yet been able to run the coordinator tests locally. I’ve raised an issue regarding this setup difficulty here.

@jonesho
Copy link
Contributor

jonesho commented Nov 19, 2024 •
edited
Loading

@nadeemb53 The coordinator test is failed again and should be easily fixed by replacing sendWithRetry() return type with EthSendTransaction, btw for running the unit tests locally, you could open the repo in your IDE (preferably IntelliJ) and then run ./gradlew -V coordinator:app:buildNeeded, just like what we did in this github workflow file. Similarily, in the same file, you could also see how we run coordinator integration tests.

jonesho
jonesho reviewed Nov 19, 2024
View reviewed changes
coordinator/ethereum/test-utils/src/main/kotlin/net/consensys/zkevm/ethereum/AccountManager.kt
result.forEach { (account, transferTx) ->

result.forEach { pair ->
val (account, transferTx) = pair
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be simplified as:

result.forEach { (account, transferTx) ->

@jonesho
Copy link
Contributor

jonesho commented Nov 19, 2024

@nadeemb53 FYI, we have a class called AsyncRetryer which could potentially be used for this case, the caveat is that it assumes the action return type is async and needs to pass in an vertx instance.

@nadeemb53 nadeemb53 closed this Dec 12, 2024
@nadeemb53 nadeemb53 reopened this Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonesho jonesho jonesho left review comments

@VGau VGau Awaiting requested review from VGau

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Coordinator Integration Test - Account Manager helper
3 participants
@nadeemb53 @thedarkjester @jonesho