added unit tests for transaction initiater changing

[aalimsahin]

WHAT

added the 'transactionInitiator' change to the EmailRecoveryModule contract
added unit tests for changes.

TEST

To test the changes, please uncomment the commented lines in the test/integration/OwnableValidatorRecovery/EmailRecoveryModule/EmailRecoveryModuleBase.t.sol (lines 123-149) and test/unit/modules/EmailRecoveryModule/EmailRecoveryModuleBase.t.sol (lines 78-102) files. Don't forget to comment out the previous one each time.

0- No changes were made to the old existing tests. We can see the failed tests along with the changes made to the Email Recovery Module contract. (Expect 15 tests to fail under this condition.)

1- After deploying the EmailRecoveryModule, assign the address that will call the acceptGuardian and processRecovery functions as the transactionInitiator. (All tests must be passed successfully)

2- Verify that everyone can initiate transactions after 6 months, even if no transactionInitiator is assigned. (All tests must be passed successfully)

3- Verify that the setTransactionInitiator function works as expected. (Expect 15 tests to fail under this condition.)

4- Validate the behavior when the transaction initiator flag is set to true first, then changed to false, and the contract is working after a waiting period of 6 months. (All tests must be passed successfully)

5- Verify that only the killSwitchAuthorizer can set the transaction initiator. (The setup phase of the tests must be failed.)

-- New --

6- Verify that it does not work before 6 months when the transaction initiator flag is not true. (Expect 15 tests to fail under this condition.)

[openzeppelin-code]

added unit tests for transaction initiater changing

Generated at commit: cc0df30a50a57ecf6c13fc6fc07b3a8f30d84ae0

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	0 0 0 3 17 20
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

---

For more details view the full report in OpenZeppelin Code Inspector

[JohnGuilding]

Thanks for putting this together. The code looks the same but noticed two inconsistencies:

1. override multiple inheritance
2. deploymentTimestamp in constructor

I am concerned basing tests on commenting in/out code is error prone. Since this is going to production imminently, I'm in favour of building a higher level of confidence with the tests cases - Wdyt?

I wrote a non-exhuastive list of test cases that I was envisioning. Thinking here is we would cover every case and all tests would pass:

Unit

Constructor

1. Modified constructor test to assert deploymentTimestamp was set correctly

setTransactionInitiator

1. Reverts if not owner
2. Sets non-zero address to true
3. Sets non-zero address to false after setting it to true (could be combined with 2.)
4. Sets zero address to true
5. Sets zero address to false after setting it to true (could be combined with 4.)

acceptGuardian

1. Reverts when transaction initiator is not set
2. Fails when exactly 6 * 30 days - 1 seconds has passed
3. Succeeds when transaction initiator is set
4. Succeeds when exactly 6 * 30 days has passed
5. Succeeds when exactly 6 * 30 days + 1 seconds has passed
6. Succeeds when zero address for transaction initiator is set to true

processRecovery

1. Reverts when transaction initiator is not set
2. Fails when exactly 6 * 30 days - 1 seconds has passed
3. Succeeds when transaction initiator is set
4. Succeeds when exactly 6 * 30 days has passed
5. Succeeds when exactly 6 * 30 days + 1 seconds has passed
6. Succeeds when zero address for transaction initiator is set to true

Integration

1. Unapproved account cannot recover account (call initial acceptGuardian)
2. Can recover account with unapproved account(s) after 6 months (full recovery flow)
3. Can acceptGuardian or processRecovery with approved account -> permission is revoked for that account -> cannot processRecovery or acceptGuardian again
4. Can acceptGuardian or processRecovery with approved account -> permission is revoked for that account -> cannot processRecovery or acceptGuardian again -> permission is given to new account and recovery can continue
5. Any others you can think of

[JohnGuilding]

Was there a reason onlyWhenActive was added here? onlyWhenActive will be called twice so can be safely removed here and for processRecovery. Unless you wanted to execute it twice but always show the KillSwitchEnabled first if that error occurrs at the same time as "Only allowed accounts can call this function"

[JohnGuilding]

Hmm do you know why this contract only requires override(EmailRecoveryManager), but the production module in clave-email-recovery requires override(EmailAccountRecovery, EmailRecoveryManager)?

[JohnGuilding]

Thanks Alim. Resolved by #82