filmakarov commented 9 months ago

Summary

changeGuardianParams issue

Fixed issue with 'changeGuardianParams' could be added by SA to also add guardians leading to inconsistency of guardiansCount.

Recoveries limitation

Added recoveriesLeft counter. Now, only user-defined # of recoveries can happen. After this, no userOps can be validated, so the user may reset the module and set new guardians. To achieve this, it is required to change the flow of how recovery request is executed:

Was: EP => SA.execute => recoverer.RecoveryMethod
Now: EP => SA.execute => AccRecoveryModule.executeRecovery => SA.execTransactionFromModuleReturnData => recoverer.RecoveryMethod Due to introducing AccRecoveryModule.executeRecovery method, we have a single point of handling the execution of the recovery requests. Here the module:
Checks if the recovery has been properly executed
Emits event
Clears the request
Decrements recoveriesLeft

Also, the module now checks that the recovery request uses executeRecovery. Requests that do not use executeRecovery are not allowed.

Reset Module

resetModule method was introduced. It still requires to provide guardians from off-chain. Otherwise, it would require storing them in a mapping, which is ugly. In any case, all the guardians are available through events.

Tests

The according changes were made to the test suite.

Related Issue: #SMA-28

Change Type

[x] Refactor
[x] New Feature
[x] Breaking Change

Checklist

[x] My code follows this project's style guidelines
[x] I've reviewed my own code
[x] I've added comments for any hard-to-understand areas
[ ] I've updated the documentation if necessary
[x] My changes generate no new warnings
[x] I've added tests that prove my fix is effective or my feature works
[x] All unit tests pass locally with my changes
[x] Any dependent changes have been merged and published

linear[bot] commented 9 months ago

SMA-28 Account Recovery Kawach

livingrockrises commented 9 months ago

what is defined as recoverer.RecoveryMethod?

filmakarov commented 9 months ago

do not merge please, may need slight improvement before merging as per Zellic notes

livingrockrises commented 8 months ago

do not merge please, may need slight improvement before merging as per Zellic notes

let us know here once all improvements are pushed.

filmakarov commented 8 months ago

do not merge please, may need slight improvement before merging as per Zellic notes

let us know here once all improvements are pushed.

all improvements are pushed, waiting for the confirmation from Zellic that all good