Add test cases for setTransactionInitiator #82
Conversation
Add test cases for setTransactionInitiator
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
| // 0- verify that existing tests have failed | ||
| // (Expect 15 tests to fail under this condition.) | ||
|
|
||
| // // 1- verify that setting the transaction initiator flag to true works as expected. | ||
| // vm.startPrank(killSwitchAuthorizer); | ||
| // emailRecoveryModule.setTransactionInitiator(address(this), true); | ||
| // vm.stopPrank(); | ||
|
|
||
| // // 2- confirm that the functionality works correctly after 6 months has passed. | ||
| // vm.warp(block.timestamp + 15_768_000); // 6 months | ||
|
|
||
| // // 3- test the scenario where the transaction initiator flag is initially set to true, then changed to false. | ||
| // // (Expect 15 tests to fail under this condition.) | ||
| // vm.startPrank(killSwitchAuthorizer); | ||
| // emailRecoveryModule.setTransactionInitiator(address(this), false); | ||
| // vm.stopPrank(); | ||
|
|
||
| // // 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. | ||
| // vm.startPrank(killSwitchAuthorizer); | ||
| // emailRecoveryModule.setTransactionInitiator(address(this), true); | ||
| // emailRecoveryModule.setTransactionInitiator(address(this), false); | ||
| // vm.stopPrank(); | ||
| // vm.warp(block.timestamp + 15_768_000); // 6 months | ||
|
|
||
| // // 5- verify that only the killSwitchAuthorizer can set the transaction initiator | ||
| // emailRecoveryModule.setTransactionInitiator(address(this), true); | ||
|
|
||
| // // 6- confirm that it does not work before 6 months when the transaction initiator flag is not true | ||
| // // (Expect 15 tests to fail under this condition.) | ||
| // vm.warp(block.timestamp + 7_884_000); // 3 months | ||
|
|
There was a problem hiding this comment.
This should be removed, we should not rely on any commented out code
| @@ -120,6 +120,38 @@ abstract contract OwnableValidatorRecovery_EmailRecoveryModule_Base is BaseTest | |||
| ); | |||
| emailRecoveryModule = EmailRecoveryModule(emailRecoveryModuleAddress); | |||
|
|
|||
| // 0- verify that existing tests have failed | |||
There was a problem hiding this comment.
Can you set up the tests so that they all pass by default. So we need to call setTransactionInitiator in test setup. Then then whenever we need to test the correct revert behaviour, we call setTransactionInitiator as needed. Does that make sense?
There was a problem hiding this comment.
@JohnGuilding I understand, I removed these commented out lines except for 1
| address owner = vm.addr(2); | ||
| address approvedAccount = address(0x2); |
There was a problem hiding this comment.
The following addresses are set in base test, could you use different ones to these:
zkEmailDeployer = vm.addr(1);
killSwitchAuthorizer = vm.addr(2);
Also you may as well use killSwitchAuthorizer instead of defining a new var called owner if that makes sense
There was a problem hiding this comment.
@JohnGuilding I intentionally set the owner to the same address as the killSwitchAuthorizer because I thought "owner" is an easier name to understand
Can I use "owner" as another name for killSwitchAuthorizer?
| address owner = vm.addr(2); | ||
| address nonOwner = address(0x2); |
There was a problem hiding this comment.
These are the same address?
There was a problem hiding this comment.
@JohnGuilding
No, I think vm.addr(2) is the second address of addresses which is generated by default seed, but address(0x2) is just an example address not practical use
owner 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF
nonOwner 0x0000000000000000000000000000000000000002
There was a problem hiding this comment.
Ah cool didn't know that. Happy to leave as is
foundry.toml
Outdated
| @@ -17,6 +17,8 @@ ignored_warnings_from = [ | |||
| threads = 1 | |||
| gas_limit = "18446744073709551615" | |||
| memory_limit = 1844674407370955161 | |||
| allow_internal_expect_revert = true | |||
There was a problem hiding this comment.
Instead of this, could you try the same as this commit from Bence? 463801f
There was a problem hiding this comment.
@JohnGuilding Yes I did it at ede3392
JohnGuilding
merged commit c42aac8
into
feat/virtual-functions-for-clave-feature
This PR adds test cases to the following PR:
#81
The tests cover the setTransactionInitiator function as comprehensively as possible, following the comments in this review:
#81 (review)
I removed the onlyWhenActive modifier from acceptGuardian and processRecovery in EmailRecoveryModule because it was applied twice. Other than that, I didn't change the implementation.
However, if you find any issues or areas for improvement, especially regarding test readability, please let me know.