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

Evaluate 4337 Module Changes For EntryPoint 0.7.0 #215

Closed
nlordell opened this issue Jan 19, 2024 · 3 comments
Closed

Evaluate 4337 Module Changes For EntryPoint 0.7.0 #215

nlordell opened this issue Jan 19, 2024 · 3 comments
Assignees

Comments

@nlordell
Copy link
Collaborator

nlordell commented Jan 19, 2024

Evaluate the required Safe 4337 Module contract changes required for the EntryPoint version 0.7. This should use the “candidate” EntryPoint version that was submitted for audit.

The expected outcome of this issue is:

  1. A detailed task summary of required code changes
  2. List of additional supporting tasks required for the new module version related to a new release
  3. Optional: Draft PR containing code and test changes
@nlordell nlordell changed the title Update 4337 Module For EntryPoint 0.7.0 Evaluate 4337 Module Changes For EntryPoint 0.7.0 Jan 19, 2024
@mmv08
Copy link
Member

mmv08 commented Jan 25, 2024

There are no migration docs except the release notes, and the quality feels subpar, to be honest. It feels strange to be pushed to update, given that the documentation is insufficient. But anyway, I went through the release notes' changes one by one.

  1. Simulation functions removed from deployed EntryPoint. - Irrelevant for accounts
  2. Added delegateAndRevert() helper in EntryPoint to accommodate nodes without state overrides in eth_call and eth_estimateGas - Irrelevant for accounts
  3. Added ERC-165 "supportsInterface" to the EntryPoint. - We could add an erc-165 check during module deployment, but I don't think it's necessary and the contract should be tested anyway.
  4. Added a sample TokenPaymaster. - This should be relevant if we decide to go for a paymaster contract.
  5. Added a 10% penalty charge for unused execution gas limit. - Should be relevant for our SDKs. It's interesting that they charge the penalty on all unused gas; I'd expect a certain threshold after which the penalty applies because gas calculations can sometimes be complicated.
  6. Removed paymaster’s second call to postOp. - Irrelevant for us atm
  7. Added validatePaymasterUserOp and postOp gas limits. - Basically, paymaster and account gas limits are separate now. So far the biggest change for us.
    image
  8. Added “gasPrice” parameter to paymaster’s postOp - Irrelevant for us atm
  9. Separated offchain UserOperation and packed UserOperation onchain struct. - Not sure I fully understand this change, this seems to be related to 7)
  10. Added a new optional account contract api method executeUserOp(). - This matches our method name but with different parameters (this one gets the hash and full user operation struct). I don't see much value in adding it because we do not access any of the struct values.
  11. There are many new storage access rules during the validation step; so far, none of them seem to affect us, but they still have to be tested with the production bundler.

So, that makes the action items:

@nlordell
Copy link
Collaborator Author

nlordell commented Jan 25, 2024

add an erc-165 interface check during module deployment for the entrypoint address?

Is this required by the IAccount interface?

support the new executeUserOp method. We could include it for better developer tooling compatibility.

What does this entail? Just changing the signature of the existing executeUserOp function?

@mmv08
Copy link
Member

mmv08 commented Jan 26, 2024

Is this required by the IAccount interface?

No, it's not about the account, it's about the entrypoint contract.

What does this entail? Just changing the signature of the existing executeUserOp function?

I would not change it but add a new method, I have a gut feeling the new method would use more gas due to more complicated calldata encoding.

@mmv08 mmv08 closed this as completed Jan 26, 2024
nlordell added a commit that referenced this issue Feb 12, 2024
**This PR**:
- Addresses the M-01 from the audit by updating the ERC-4337 contracts
to the latest ones from the development branch (presumably, they will be
released as version 0.7.0). The most notable change is the new
`PackedUserOperation` struct with the new packed gas limit field
`accountGasLimits` that consists of two `uint128` packed values
`(validationGasLimit, callGasLimit)`
- Implements #225

Forked contracts repo with build artifacts:
https://github.com/5afe/account-abstraction
More about 0.7.0 changes:
#215

**Issues/concerns**:
- Using unaudited non-release candidate contract versions
- The reference bundler is still in development, thus e2e tests are
failing (Confirmed with ERC4337 team)

---------

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
nlordell added a commit that referenced this issue Feb 23, 2024
**This PR**:
- Addresses the M-01 from the audit by updating the ERC-4337 contracts
to the latest ones from the development branch (presumably, they will be
released as version 0.7.0). The most notable change is the new
`PackedUserOperation` struct with the new packed gas limit field
`accountGasLimits` that consists of two `uint128` packed values
`(validationGasLimit, callGasLimit)`
- Implements #225

Forked contracts repo with build artifacts:
https://github.com/5afe/account-abstraction
More about 0.7.0 changes:
#215

**Issues/concerns**:
- Using unaudited non-release candidate contract versions
- The reference bundler is still in development, thus e2e tests are
failing (Confirmed with ERC4337 team)

---------

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
nlordell added a commit that referenced this issue Mar 5, 2024
**This PR**:
- Addresses the M-01 from the audit by updating the ERC-4337 contracts
to the latest ones from the development branch (presumably, they will be
released as version 0.7.0). The most notable change is the new
`PackedUserOperation` struct with the new packed gas limit field
`accountGasLimits` that consists of two `uint128` packed values
`(validationGasLimit, callGasLimit)`
- Implements #225

Forked contracts repo with build artifacts:
https://github.com/5afe/account-abstraction
More about 0.7.0 changes:
#215

**Issues/concerns**:
- Using unaudited non-release candidate contract versions
- The reference bundler is still in development, thus e2e tests are
failing (Confirmed with ERC4337 team)

---------

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
nlordell added a commit that referenced this issue Mar 5, 2024
**This PR**:
- Addresses the M-01 from the audit by updating the ERC-4337 contracts
to the latest ones from the development branch (presumably, they will be
released as version 0.7.0). The most notable change is the new
`PackedUserOperation` struct with the new packed gas limit field
`accountGasLimits` that consists of two `uint128` packed values
`(validationGasLimit, callGasLimit)`
- Implements #225

Forked contracts repo with build artifacts:
https://github.com/5afe/account-abstraction
More about 0.7.0 changes:
#215

**Issues/concerns**:
- Using unaudited non-release candidate contract versions
- The reference bundler is still in development, thus e2e tests are
failing (Confirmed with ERC4337 team)

---------

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
nlordell added a commit that referenced this issue Mar 5, 2024
**This PR**:
- Addresses the M-01 from the audit by updating the ERC-4337 contracts
to the latest ones from the development branch (presumably, they will be
released as version 0.7.0). The most notable change is the new
`PackedUserOperation` struct with the new packed gas limit field
`accountGasLimits` that consists of two `uint128` packed values
`(validationGasLimit, callGasLimit)`
- Implements #225

Forked contracts repo with build artifacts:
https://github.com/5afe/account-abstraction
More about 0.7.0 changes:
#215

**Issues/concerns**:
- Using unaudited non-release candidate contract versions
- The reference bundler is still in development, thus e2e tests are
failing (Confirmed with ERC4337 team)

---------

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
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

No branches or pull requests

2 participants