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.

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

fix: default nonce manager resets previous nonce #2623

Closed
wants to merge 2 commits into from

Conversation

holic
Copy link
Contributor

@holic holic commented Aug 16, 2024

I am using the default nonce manager to trigger a bunch of transactions in a queue to ensure they land in the mempool in order. If any transaction fails to submit to the mempool, I call nonceManager.reset().

For example:

tx:1 -> nonce:1
tx:2 -> nonce:2
tx:3 -> nonce:3

If tx:1 fails, I call nonceManager.reset() and retry it. However, the logic that looks up the nonce after reset also takes into account the previously cached nonce in the nonce manager:

const nonce = await source.get({ address, chainId, client })
const previousNonce = nonceMap.get(key) ?? 0
if (previousNonce > 0 && nonce <= previousNonce)
return previousNonce + 1
nonceMap.delete(key)
return nonce

Since the queue is blocked on tx:1, our nonce at the source should still be nonce:1. But because of the logic above, the nonce returned will actually be nonce:4 (because of our previously queued txs).

IMO .reset should reset the state of the nonce manager back to the source, including the cached nonce value.


PR-Codex overview

The focus of this PR is to add tests for the nonceManager reset functionality and ensure correct nonce handling during transactions.

Detailed summary

  • Added a test for nonceManager reset functionality
  • Implemented handling of nonce increment and reset during transactions

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

changeset-bot bot commented Aug 16, 2024

⚠️ No Changeset found

Latest commit: aec5183

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Aug 16, 2024

@holic is attempting to deploy a commit to the Wevm Team on Vercel.

A member of the Team first needs to authorize it.

@holic
Copy link
Contributor Author

holic commented Aug 16, 2024

This change does work for my context (nonce is reset and I no longer get "nonce too high" errors), but I just noticed this line:

} finally {
this.reset({ address, chainId })
}

I think this means this change needs a deeper refactor to make sense? Will play with this more locally and report back with an alternative.

@holic holic marked this pull request as draft August 16, 2024 12:55
@holic
Copy link
Contributor Author

holic commented Aug 17, 2024

Added a test to demonstrate the expected behavior.

@jxom jxom force-pushed the main branch 3 times, most recently from b56e162 to ad2831b Compare September 7, 2024 02:46
@jxom
Copy link
Member

jxom commented Oct 14, 2024

closing for now, will reopen when we visit in near future

@jxom jxom closed this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants