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

Safe Recovery Initial Setup #383

Closed
wants to merge 1 commit into from
Closed

Safe Recovery Initial Setup #383

wants to merge 1 commit into from

Conversation

remedcu
Copy link
Member

@remedcu remedcu commented Apr 16, 2024

This PR adds an empty NPM package for the Recovery Module, including:

  • CI configuration
  • Default Hardhat template
  • hardhat-deploy configuration
  • Linting

Inspired from #297, this PR fixes #350

@remedcu remedcu self-assigned this Apr 16, 2024
@remedcu remedcu requested a review from a team as a code owner April 16, 2024 07:53
@remedcu remedcu requested review from nlordell, akshay-ap and mmv08 and removed request for a team April 16, 2024 07:53
Copy link
Member

@mmv08 mmv08 left a comment

Choose a reason for hiding this comment

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

Did we decide that we need a new module instead of choosing plenty of existing options? I cannot see any decision made in #348

@remedcu
Copy link
Member Author

remedcu commented Apr 16, 2024

Did we decide that we need a new module instead of choosing plenty of existing options? I cannot see any decision made in #348

Even though not as the first milestone, we want to support the 4337 flow sooner or later, which I believe the existing options do not support.

CC @nlordell

@mmv08
Copy link
Member

mmv08 commented Apr 16, 2024

Did we decide that we need a new module instead of choosing plenty of existing options? I cannot see any decision made in #348

Even though not as the first milestone, we want to support the 4337 flow sooner or later, which I believe the existing options do not support.

CC @nlordell

I took a look at candide contracts and they seem to fit our requirements, except for the 4337. Theoretically, we may just inherit our contract from it and add a validateUserOp function - was that option explored?

cc @Sednaoui

@Sednaoui
Copy link

Did we decide that we need a new module instead of choosing plenty of existing options? I cannot see any decision made in #348

Even though not as the first milestone, we want to support the 4337 flow sooner or later, which I believe the existing options do not support.
CC @nlordell

I took a look at candide contracts and they seem to fit our requirements, except for the 4337. Theoretically, we may just inherit our contract from it and add a validateUserOp function - was that option explored?

cc @Sednaoui

I am not sure I understood the 4337 requirements needed for the Recovery Module. Can you please further clarify? From our knowledge:

  • Candide recovery module works for any safe, including ones with the 4337Module.
  • When the owner interacts with the recovery module, if the owner is a 4337 account, then the account normally validatesUserOp before they can call the recovery module. The recovery module only checks that the call is coming from the account itself.
  • When a guardian interacts with the module, they interact with it just as any other contract on chain.

Having validateUserOp can work for Safe{Core} Protocol because they do have signature validators, if the goal for example for guardians to initiate and finalize the recovery using the owner's account (owner pays the gas and is the sender), then the recovery module can act both as a plugin and a signature validator. That is not possible with just 4337 Safe Module, so I don't understand the reasoning to include validateUserOp in the recovery module.

@nlordell
Copy link
Collaborator

For some additional context, we are considering adding "signature validator" support to the 4337 module to allow modules to be executed over 4337 (which is currently not possible with Safe and the 4337 module).

@Sednaoui
Copy link

For some additional context, we are considering adding "signature validator" support to the 4337 module to allow modules to be executed over 4337 (which is currently not possible with Safe and the 4337 module).

Got it -- yes in that case the support for signature validators are quite beneficial. We can add support for it in the social recovery module if this is planned

@remedcu remedcu added the wontfix This will not be worked on label Apr 22, 2024
@remedcu
Copy link
Member Author

remedcu commented Apr 22, 2024

Based on internal discussion, we have decided we will be exploring currently available module options instead of creating a new one from scratch. So, this PR will be considered as "won't implement".

@remedcu remedcu closed this Apr 22, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2024
@nlordell nlordell deleted the safe-recovery-setup branch May 13, 2024 11:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup NPM Package For Recovery Module
4 participants