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

Add functions which returns the user state is in module enabled or recovery is activated. #28

Merged
merged 10 commits into from
Aug 6, 2024
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@
"dependencies": {
"@matterlabs": "github:matter-labs/era-contracts",
"@openzeppelin/contracts-upgradeable": "v5.0.1",
"@rhinestone/modulekit": "github:rhinestonewtf/modulekit#test/safe-latest",
"@rhinestone/modulekit": "github:rhinestonewtf/modulekit",
"@zk-email/contracts": "v6.0.3",
"email-wallet-sdk": "github:zkemail/email-wallet-sdk",
"erc7579-implementation": "github:erc7579/erc7579-implementation",
"ether-email-auth": "github:zkemail/ether-email-auth",
"ether-email-auth": "github:zkemail/ether-email-auth#711c7892c244b210febe9d6f8cc28eddbce5fbdc",
"solidity-stringutils": "github:Arachnid/solidity-stringutils"
},
"files": [
Expand Down
144 changes: 83 additions & 61 deletions pnpm-lock.yaml

Large diffs are not rendered by default.

10 changes: 10 additions & 0 deletions src/EmailRecoveryManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,16 @@ abstract contract EmailRecoveryManager is
return recoveryRequests[account];
}

/**
* @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;
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/**
* @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;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick - added suggestion for consistent naming with other functions in this file

/**
* @notice Returns a two-dimensional array of strings representing the subject templates for an
* acceptance by a new guardian.
Expand Down
1 change: 1 addition & 0 deletions src/interfaces/IEmailRecoveryModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
pragma solidity ^0.8.25;

interface IEmailRecoveryModule {
function isAuthorizedToBeRecovered(address account) external view returns (bool);
function canStartRecoveryRequest(address smartAccount) external view returns (bool);
}
1 change: 1 addition & 0 deletions src/interfaces/IUniversalEmailRecoveryModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
pragma solidity ^0.8.25;

interface IUniversalEmailRecoveryModule {
function isAuthorizedToBeRecovered(address account) external returns (bool);
function canStartRecoveryRequest(
address account,
address validator
Expand Down
19 changes: 17 additions & 2 deletions src/modules/EmailRecoveryModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,17 @@ contract EmailRecoveryModule is EmailRecoveryManager, ERC7579ExecutorBase, IEmai
*/
bytes4 public immutable selector;

/**
* Mapping of Account address to authorized validator
*/
mapping(address account => bool isAuthorized) internal authorized;

event RecoveryExecuted(address indexed account, address indexed validator);

error InvalidSelector(bytes4 selector);
error InvalidOnInstallData();
error InvalidValidator(address validator);

constructor(
address verifier,
address dkimRegistry,
Expand Down Expand Up @@ -97,7 +102,7 @@ contract EmailRecoveryModule is EmailRecoveryManager, ERC7579ExecutorBase, IEmai
) {
revert InvalidValidator(validator);
}

authorized[msg.sender] = true;
configureRecovery(guardians, weights, threshold, delay, expiry);
}

Expand All @@ -106,6 +111,7 @@ contract EmailRecoveryModule is EmailRecoveryManager, ERC7579ExecutorBase, IEmai
* @dev the data parameter is not used
*/
function onUninstall(bytes calldata /* data */ ) external {
authorized[msg.sender] = false;
deInitRecoveryModule();
}

Expand All @@ -118,6 +124,15 @@ contract EmailRecoveryModule is EmailRecoveryManager, ERC7579ExecutorBase, IEmai
return getGuardianConfig(account).threshold != 0;
}

/**
* Check if the recovery module is authorized to recover the account
* @param account The smart account to check
* @return true if the module is authorized, false otherwise
*/
function isAuthorizedToBeRecovered(address account) external view returns (bool) {
return authorized[account];
}

/**
* Check if a recovery request can be initiated based on guardian acceptance
* @param account The smart account to check
Expand Down
9 changes: 9 additions & 0 deletions src/modules/UniversalEmailRecoveryModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,15 @@ contract UniversalEmailRecoveryModule is
return getGuardianConfig(account).threshold != 0;
}

/**
* Check if the recovery module is authorized to recover the account
* @param account The smart account to check
* @return true if the module is authorized, false otherwise
*/
function isAuthorizedToBeRecovered(address account) external view returns (bool) {
return validatorCount[account] > 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I use return authorized[account] as same as the src/modules/EmailRecoveryModule.sol ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment on modified solution. Let me know what you think

}

/**
* Check if a recovery request can be initiated based on guardian acceptance
* @param account The smart account to check
Expand Down
6 changes: 6 additions & 0 deletions test/unit/EmailRecoveryManager/configureRecovery.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase {
vm.startPrank(accountAddress);
emailRecoveryModule.workaround_validatorsPush(accountAddress, validatorAddress);

bool isActivated = emailRecoveryModule.isActivated(accountAddress);
assertFalse(isActivated);

vm.expectEmit();
emit IEmailRecoveryManager.RecoveryConfigured(
instance.account, guardians.length, totalWeight, threshold
Expand All @@ -66,6 +69,9 @@ contract EmailRecoveryManager_configureRecovery_Test is UnitBase {
emailRecoveryModule.getGuardian(accountAddress, guardians[0]);
assertEq(uint256(guardian.status), uint256(GuardianStatus.REQUESTED));
assertEq(guardian.weight, guardianWeights[0]);

isActivated = emailRecoveryModule.isActivated(accountAddress);
assertTrue(isActivated);
}

function test_ConfigureRecovery_RevertWhen_ZeroGuardians() public {
Expand Down
6 changes: 6 additions & 0 deletions test/unit/EmailRecoveryManager/deInitRecoveryModule.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ contract EmailRecoveryManager_deInitRecoveryModule_Test is UnitBase {
acceptGuardian(accountSalt1);
acceptGuardian(accountSalt2);

bool isActivated = emailRecoveryModule.isActivated(accountAddress);
assertTrue(isActivated);

vm.prank(accountAddress);
vm.expectEmit();
emit IEmailRecoveryManager.RecoveryDeInitialized(accountAddress);
Expand Down Expand Up @@ -66,5 +69,8 @@ contract EmailRecoveryManager_deInitRecoveryModule_Test is UnitBase {
assertEq(guardianConfig.totalWeight, 0);
assertEq(guardianConfig.acceptedWeight, 0);
assertEq(guardianConfig.threshold, 0);

isActivated = emailRecoveryModule.isActivated(accountAddress);
assertFalse(isActivated);
}
}
6 changes: 6 additions & 0 deletions test/unit/EmailRecoveryManager/updateRecoveryConfig.t.sol
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick - why do we need assertions for isActivated in these tests? threshold state isn't modified in this function

Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ contract EmailRecoveryManager_updateRecoveryConfig_Test is UnitBase {
recoveryConfig = emailRecoveryModule.getRecoveryConfig(accountAddress);
assertEq(recoveryConfig.delay, newDelay);
assertEq(recoveryConfig.expiry, newExpiry);

bool isActivated = emailRecoveryModule.isActivated(accountAddress);
assertTrue(isActivated);
}

function test_UpdateRecoveryConfig_Succeeds() public {
Expand All @@ -117,5 +120,8 @@ contract EmailRecoveryManager_updateRecoveryConfig_Test is UnitBase {
recoveryConfig = emailRecoveryModule.getRecoveryConfig(accountAddress);
assertEq(recoveryConfig.delay, newDelay);
assertEq(recoveryConfig.expiry, newExpiry);

bool isActivated = emailRecoveryModule.isActivated(accountAddress);
assertTrue(isActivated);
}
}
3 changes: 3 additions & 0 deletions test/unit/modules/EmailRecoveryModule/onInstall.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,8 @@ contract EmailRecoveryModule_onInstall_Test is EmailRecoveryModuleBase {

bool isInitialized = emailRecoveryModule.isInitialized(accountAddress);
assertTrue(isInitialized);

bool isAuthorizedToBeRecovered = emailRecoveryModule.isAuthorizedToBeRecovered(accountAddress);
assertTrue(isAuthorizedToBeRecovered);
}
}
3 changes: 3 additions & 0 deletions test/unit/modules/EmailRecoveryModule/onUninstall.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,8 @@ contract EmailRecoveryModule_onUninstall_Test is EmailRecoveryModuleBase {

bool isInitialized = emailRecoveryModule.isInitialized(accountAddress);
assertFalse(isInitialized);

bool isAuthorizedToBeRecovered = emailRecoveryModule.isAuthorizedToBeRecovered(accountAddress);
assertFalse(isAuthorizedToBeRecovered);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,8 @@ contract UniversalEmailRecoveryModule_onInstall_Test is UnitBase {
bytes4[] memory allowedSelectors = emailRecoveryModule.getAllowedSelectors(accountAddress);
assertEq(allowedValidators.length, 1);
assertEq(allowedSelectors.length, 1);

bool isAuthorizedToBeRecovered = emailRecoveryModule.isAuthorizedToBeRecovered(accountAddress);
assertTrue(isAuthorizedToBeRecovered);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,8 @@ contract UniversalEmailRecoveryModule_onUninstall_Test is UnitBase {
allowedSelectors = emailRecoveryModule.getAllowedSelectors(accountAddress);
assertEq(allowedValidators.length, 0);
assertEq(allowedSelectors.length, 0);

bool isAuthorizedToBeRecovered = emailRecoveryModule.isAuthorizedToBeRecovered(accountAddress);
assertFalse(isAuthorizedToBeRecovered);
}
}