viem icon indicating copy to clipboard operation
viem copied to clipboard

fix: default nonce manager resets previous nonce

Open frolic opened this issue 1 year ago • 4 comments

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:

https://github.com/wevm/viem/blob/5f350ecf63b2da861c8dcb60e77fbcae3e7192ab/src/utils/nonceManager.ts#L79-L84

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}

frolic avatar Aug 16 '24 12:08 frolic

⚠️ No Changeset found

Latest commit: aec51837d528234a54572a408344caa67e9af6d8

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

changeset-bot[bot] avatar Aug 16 '24 12:08 changeset-bot[bot]

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

A member of the Team first needs to authorize it.

vercel[bot] avatar Aug 16 '24 12:08 vercel[bot]

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: https://github.com/wevm/viem/blob/5f350ecf63b2da861c8dcb60e77fbcae3e7192ab/src/utils/nonceManager.ts#L85-L87

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.

frolic avatar Aug 16 '24 12:08 frolic

Added a test to demonstrate the expected behavior.

frolic avatar Aug 17 '24 12:08 frolic

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

jxom avatar Oct 14 '24 01:10 jxom