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

feat: add a nonce key override to support parallel nonces #462

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

moldy530
Copy link
Collaborator

@moldy530 moldy530 commented Feb 15, 2024

Pull Request Checklist


PR-Codex overview

Focus of the PR

This PR focuses on allowing the use of parallel nonces for user operations in smart contracts.

Detailed summary

  • Added a nonceKey parameter to the buildUserOperation function in buildUserOperation.ts.
  • Updated the getNonce function in smartContractAccount.ts to accept a nonceKey parameter.

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

@@ -54,6 +54,7 @@ export type UserOperationOverrides = Partial<{
| UserOperationStruct["verificationGasLimit"]
| Percentage;
paymasterAndData: UserOperationStruct["paymasterAndData"];
nonceKey: bigint;
Copy link
Contributor

Choose a reason for hiding this comment

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

so by differentiating nonceKey, you can send parallel nonces. But our bundler supports only up to 4 for unstaked accounts. (Unstaked senders are only allowed to have 4 UOs in the mempool. This is already possible today with parallel nonces).

  1. we should put doc explaining about these nonce keys, their usage, context, etc.
  2. this is an easy quick solution, but I think not really falls into "override". Would be better to have our sendUO api to support parallel nonces more natively.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right now our bundler doesn't really support parallel nonces (it does but users don't get the full benefit since a bundle will only contain 1 UO from a given sender). I think once our bundler supports it more fully then we can add much better docs.

  1. yea I can update the UserOperationOverride docs for this override.
  2. I disagree about it not being a override, but agree there's probably a better way to do it. It's an override because other methods use sendUserOperation under the hood. Take a look at the plugin gen'd methods for example. Those all call send UO under the hood. This overrides the default behaviour of buildUserOperation, and such falls in the mental model of an override. There is a world where getNonce is actually a middleware instead, and then users provide custom nonce middleware (like ethers-rs / alloy). But until we understand how users plan to use this (ie. is baking this into the entryPoint.getNonce() method not enough), I don't think we should over optimize

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added a comment here since the docs for UserOperationOverrides just link you to this part of the code anyways

Copy link
Contributor

Choose a reason for hiding this comment

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

for now not blocking.

Copy link
Contributor

Choose a reason for hiding this comment

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

@moldy530 parallel nonces means there are multiple UOs in the bundler mempool, different from batching which is just one UO. My understanding is that our bundler does support multiple parallel nonces from a single sender, up to 4 for unstaked users. Am I understanding wrong? cc: @dancoombs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Our bundler does support submitting parallel nonces and it won't reject the UOs, but it will still only include one UO per bundle (at least was my understanding). That's what I was trying to get at in the comment that it might not be "fully supported". The full benefit of the parallel nonces is when all of your UOs can be included in the bundle

@moldy530 moldy530 force-pushed the moldy/nonce-key-ovd branch from f3dd86d to 2b544ce Compare February 15, 2024 23:55
Copy link
Contributor

@denniswon denniswon left a comment

Choose a reason for hiding this comment

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

LGTM. Later we could add a doc or guide for using the parallel nonces once Alchemy rundler supports it. I thought we already did and support sending multiple UOs to the mempool.

@denniswon denniswon merged commit d48c586 into main Feb 19, 2024
2 checks passed
@denniswon denniswon deleted the moldy/nonce-key-ovd branch February 19, 2024 19:20
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