wevm / viem

TypeScript Interface for Ethereum
https://viem.sh
Other
2.45k stars 737 forks source link

fix: default nonce manager resets previous nonce #2623

Open holic opened 1 month ago

holic commented 1 month ago

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

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

changeset-bot[bot] commented 1 month ago

⚠️ 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

vercel[bot] commented 1 month ago

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

A member of the Team first needs to authorize it.

holic commented 1 month ago

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.

holic commented 1 month ago

Added a test to demonstrate the expected behavior.