Add functions which returns the user state is in module enabled or recovery is activated. #28
Conversation
1. Add isActivated function to EmailRecoveryManager 2. Add isAuthorizedToBeRecovered to EmailRecoveryModule and UniversalEmailRecoveryModule 3. Update unit tests in some actions.
| * @return true if the module is authorized, false otherwise | ||
| */ | ||
| function isAuthorizedToBeRecovered(address account) external view returns (bool) { | ||
| return validatorCount[account] > 0; |
There was a problem hiding this comment.
Should I use return authorized[account] as same as the src/modules/EmailRecoveryModule.sol ?
There was a problem hiding this comment.
See comment on modified solution. Let me know what you think
There was a problem hiding this comment.
This looks good. Although I think I've thought of a better way of doing this. Wdyt of this idea:
Instead of re-adding the isAuthorizedToBeRecovered logic, we remove that function and related state from each recovery module, and just use the isActivated function as you use it here? There is no need to write a separate function on the modules if you can use isActivated and you can use state from the manager, as the manager is now part of the module
If you agree with this change, could you also re-add the logic for calling isAuthorizedToBeRecovered in the manager too but using isActiviated instead? Like this:
if (!isActivated()) {
revert RecoveryModuleNotAuthorized();
}It isn't strictly related to the isActiviated change, but will actually be useful for the native safe module PR (it was my mistake to remove it in hindsight). This will involve:
- Adding the equivalent of this logic, but using
!isActivated()instead ofisAuthorizedToBeRecoveredback toacceptGuardianandprocessRecovery:
if (!isActivated()) {
revert RecoveryModuleNotAuthorized();
}- Add the tests back for this
- Also add a similar check in
completeRecovery: - Write a test for new check in
completeRecovery - Remove all changes related to
isAuthorizedToBeRecoveredand associated state in the modules that were added in this PR
src/EmailRecoveryManager.sol
Outdated
| /** | ||
| * @notice Checks if the recovery is activated for a given account | ||
| * @param recoveredAccount The address of the recovered account for which the activation status is being checked | ||
| * @return bool True if the recovery request is activated, false otherwise | ||
| */ | ||
| function isActivated(address recoveredAccount) public view override returns (bool) { | ||
| return guardianConfigs[recoveredAccount].threshold > 0; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
| /** | |
| * @notice Checks if the recovery is activated for a given account | |
| * @param recoveredAccount The address of the recovered account for which the activation status is being checked | |
| * @return bool True if the recovery request is activated, false otherwise | |
| */ | |
| function isActivated(address recoveredAccount) public view override returns (bool) { | |
| return guardianConfigs[recoveredAccount].threshold > 0; | |
| } | |
| /** | |
| * @notice Checks if the recovery is activated for a given account | |
| * @param account The address of the account for which the activation status is being checked | |
| * @return bool True if the recovery request is activated, false otherwise | |
| */ | |
| function isActivated(address account) public view override returns (bool) { | |
| return guardianConfigs[account].threshold > 0; | |
| } |
There was a problem hiding this comment.
Nitpick - added suggestion for consistent naming with other functions in this file
There was a problem hiding this comment.
Nitpick - why do we need assertions for isActivated in these tests? threshold state isn't modified in this function
| * @return true if the module is authorized, false otherwise | ||
| */ | ||
| function isAuthorizedToBeRecovered(address account) external view returns (bool) { | ||
| return validatorCount[account] > 0; |
There was a problem hiding this comment.
See comment on modified solution. Let me know what you think
|
LGTM @wshino. Pushed some minor changes:
|
This PR is for adding the functions which returns user state is in module enabled or recovery process is activated.
Currently for rhinestone, they removed the branch named feature/4337-compliance in rhinestonewtf/safe7579.I used forking repo in zkemail instead, I hope this will be fixed soon.
I'm going to create a PR to them.
Also regarding ether-email-auth, at now we need to use commit hash or branch name to use the latest EmailAccountRecovery.sol. This will be fixed soon as we will use npm package.