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

single auth tuple for UserOperation #232

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Conversation

drortirosh
Copy link
Contributor

No description provided.

Copy link
Contributor

@forshtat forshtat left a comment

Choose a reason for hiding this comment

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

I have serious doubts that this change will be beneficial in the long term.

@@ -8,4 +9,5 @@ export interface UserOperation extends OperationBase {
nonce: BigNumberish

preVerificationGas: BigNumberish
eip7712auth?: EIP7702Authorization

This comment was marked as resolved.

Base automatically changed from AA-453-eip-7702-support to master December 22, 2024 12:17
@@ -230,7 +230,7 @@ export class BundleManager implements IBundleManager {
const authorizationList: AuthorizationList = eip7702Tuples.map(it => {
const res = {
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion,@typescript-eslint/no-base-to-string
chainId: `0x${parseInt(it.chainId.toString()).toString(16)}` as PrefixedHexString,
chainId: `0x${parseInt(it.chainId.toString()).toString(16)}`.replace(/0x0*/, '0x') as PrefixedHexString,
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, this is getting out of hand. Please create a getHexChainId() function that would handle this bullshit.

if (!mergeOk) {
debug('unable to add bundle as it relies on an EIP-7702 tuple that conflicts with other UserOperations')
continue
const authorizationList = getAuthorizationList(entry.userOp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you inline this method? And any way, the mergeEip7702Authorizations is still there. I don't see a point in this change.

Copy link
Contributor Author

@drortirosh drortirosh Jan 6, 2025

Choose a reason for hiding this comment

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

getAuthorizationList is for simpler support for erc4337 and rip7562: it accepts a Base class, and parse it as either UserOp or AATX. also, it always returns a list, and empty one of no tuple exists. This saves checking (and type casts) on each use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

restored (I didn't "inline" it. maybe I originally created my pr before you extracted it..)

@@ -463,17 +473,18 @@ export class BundleManager implements IBundleManager {
* @return {boolean} - Returns `true` if the authorizations were successfully merged, otherwise `false`.
*/
mergeEip7702Authorizations (entry: MempoolEntry, authList: EIP7702Authorization[]): boolean {
for (const eip7702Authorization of entry.userOp.authorizationList ?? []) {
// TODO: need to replace
Copy link
Contributor

Choose a reason for hiding this comment

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

A new TODO for no apparent reason.

Comment on lines 485 to 487
// if (existingAuthorization == null && entry.userOp.authorizationList != null) {
// authList.push(...getAuthorizationList(entry.userOp))
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented out code

Comment on lines 241 to 250
if (authorizationList.length > 0) {
// relevant only for RIP-7562...
requireCond(authorizationList.length === 1, 'Only one authorization is supported', ValidationErrors.InvalidFields)

const currentChainId = BigNumber.from((this.entryPoint.provider as any)._network.chainId)
const authChainId = BigNumber.from(authorizationList[0].chainId)
requireCond(authChainId.eq(BigNumber.from(0)) ||
authChainId.eq(currentChainId), 'Invalid chainId in authorization', ValidationErrors.InvalidFields)
requireCond(getEip7702AuthorizationSigner(authorizationList[0]).toLowerCase() === userOp.sender.toLowerCase(), 'Authorization signer is not sender', ValidationErrors.InvalidFields)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract to a separate function.

packages/validation-manager/src/ValidationManager.ts Outdated Show resolved Hide resolved
packages/validation-manager/src/ValidationManager.ts Outdated Show resolved Hide resolved
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