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

[Protocol Manager] Evaluate 4337 Compatible Validator Implementation #125

Closed
rmeissner opened this issue Oct 27, 2023 · 2 comments
Closed
Assignees

Comments

@rmeissner
Copy link
Member

rmeissner commented Oct 27, 2023

Evaluate necessary implementation changes to make the Manager 4337 compatible

See #60 (comment)

Expected outcome from this task would be to have:

  • A PoC implementation (not necessarily a merged PR)
  • Documented learnings on remaining work and considerations required for full support
@nlordell
Copy link
Collaborator

nlordell commented Nov 9, 2023

A working PoC implementation is provided in #131. Some notable changes to the Manager that were required for the implementation to work:

  1. The storage mappings in the manager must use the account address as the inner most key (as suggested in [Protocol Manager] Evaluate Protocol support for ERC4337 #60 (comment)). This was implemented with (AFAICT) no downsides in Refactor Storage Access to be 4337 Compatible #132 and can be merged.
  2. An alternative path is required for the 4337 module to transfer the prefund to the entrypoint. In the PoC this is the transferPrefund method. Notably, it cannot check that the plugin is enabled and not flagged. Note that this also means that currently the permission bits ARE NOT CHECKED. I created another issue [Protocol Specs] More Flexible Plugin Permissions safe-core-protocol-specs#65 where with a case for changing the way permissions are stored to instead be on the account's enabled plugin and not tied to the registry.
  3. Requires a staked factory in order to support the current validateUserOp implementation with initCode (without initCode, the current PoC works)

Remaining things to evaluate for a full implementation:

  • Evaluate if the plugin + function handler module is the best approach, support could be added directly to the Manager for example
  • Make sure all security guarantees are upheld by the plugin + function handler module; in particular, that it isn't possible to trick the module into transferring the prefund to the entrypoint outside of the expected flow (i.e. entrypoint calls validateUserOp on the account).
  • Clean up hacked-on transferPrefund escape hatch which is required (documentation, evaluate if it is the best/most sound approach, tests, etc.). I think in general that the flow feels a bit ad-hoc and not super well designed. As detailed in the spec, the manager needs some level of first class support for 4337, so it would make sense to evaluate what the best way to add this first class support is (either move validateUserOp implementation into the manager, or keep the current transferPrefund escape hatch, or add first class support for "validators" - which would work with validateUserIntent which is an EIP that is currently being worked on).
  • Tests, tests, tests

@nlordell
Copy link
Collaborator

Closing as we have reached the intended outcome and documented the findings from implementing a PoC ERC-4337 module with the Safe{Core} Protocol.

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

3 participants