Feat/guardian module #1
Conversation
Created GuardianRecoveryValidator structure to start development
implemented proposeRecoveryKey and addRecoveryKey with tests.
| // If the guardian exist this method stops | ||
| for (uint i = 0; i < guardians.length; i++) { | ||
| if (guardians[i].addr == newGuardian) { | ||
| return; |
There was a problem hiding this comment.
Maybe we should revert in this case
| Guardian[] storage guardians = accountGuardians[msg.sender]; | ||
|
|
||
| // If the guardian exist this method stops | ||
| for (uint i = 0; i < guardians.length; i++) { |
There was a problem hiding this comment.
Please replace all uint variable types for uint256
| bool isReady; | ||
| } | ||
|
|
||
| mapping(address account => Guardian[]) accountGuardians; |
There was a problem hiding this comment.
Perhaps an approach of:
struct GuardianStatus { bool proposed; bool ready }
mapping(address account => mapping (address guardian => GuardianStatus)) guardians;Might be more gas efficient.
There was a problem hiding this comment.
If we do that we cannot remove all the guardians for an account or list all the guardians of an account. Not sure how bad is that.
| bool retValue = !guardians[i].isReady; | ||
| guardians[i].isReady = true; | ||
| return retValue; |
There was a problem hiding this comment.
We should revert if the guardian is already confirmed.
There was a problem hiding this comment.
They do the return false things in other modules. That's why I did it in this way.
There was a problem hiding this comment.
Yes but that's usually done when the functions are meant to be called by other contracts. In this case, we don't want the user wasting gas in case they're already guardians.
When a user tries to remove a guardian that does not exist we revert the tx.
Using uint256 instead of uint everywhere.
Description
Additional context