add guardian tests

contracts/smart-account/modules/AccountRecoveryModule.sol

@@ -288,14 +288,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
         if (guardian == bytes32(0)) revert ZeroGuardian();
         if (_guardians[guardian][msg.sender].validUntil != 0)
             revert GuardianAlreadySet(guardian, msg.sender);
-
         (validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(
             validUntil,
             validAfter
         );
-
-        // TODO:
-        // make a test case that it fails if validAfter + securityDelay together overflow uint48
         _guardians[guardian][msg.sender] = TimeFrame(validUntil, validAfter);
         ++_smartAccountSettings[msg.sender].guardiansCount;
         emit GuardianAdded(
@@ -306,7 +302,7 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
     }
 
     // natspec
-    // same as adding guardian, but also makes the old one active only until the new one is active
+    // deletes guardian and adds the new one, however it will still be valid not earlier than now+securityDelay
     function replaceGuardian(
         bytes32 guardian,
         bytes32 newGuardian,
@@ -319,6 +315,8 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
         if (guardian == bytes32(0)) revert ZeroGuardian();
         if (newGuardian == bytes32(0)) revert ZeroGuardian();
 
+        // remove guardian
+
         (validUntil, validAfter) = _checkAndAdjustValidUntilValidAfter(
             validUntil,
             validAfter
@@ -329,7 +327,10 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
             validUntil == 0 ? type(uint48).max : validUntil,
             validAfter
         );
-        ++_smartAccountSettings[msg.sender].guardiansCount;
+
+        // don't increment guardiansCount as we haven't decremented it when deleting previous one
+        // ++_smartAccountSettings[msg.sender].guardiansCount;
+
         emit GuardianAdded(
             msg.sender,
             newGuardian,
@@ -338,16 +339,6 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
                 validAfter
             )
         );
-
-        // make the previous stay valid until the new one becomes valid
-        // if the new one becomes valid earlier, than old one validUntil, change the validUntil for the old one
-        // to the validAfter of the new one. So two are never valid at the same time
-        uint48 oldGuardianValidUntil = _guardians[guardian][msg.sender]
-            .validUntil;
-        _guardians[guardian][msg.sender].validUntil = (oldGuardianValidUntil <
-            validAfter)
-            ? oldGuardianValidUntil
-            : validAfter;
     }
 
     // natspec
@@ -380,6 +371,9 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
     // validUntil's and validAfter's for several guardians works correctly
     // @note if validAfter is less then now + securityDelay, it is set to now + securityDelay
     // as for security reasons new guardian is only active after securityDelay
+    // validAfter is always gte now+securityDelay
+    // and validUntil is always gte validAfter
+    // thus we do not need to check than validUntil is gte now
     function _checkAndAdjustValidUntilValidAfter(
         uint48 validUntil,
         uint48 validAfter
@@ -393,7 +387,6 @@ contract AccountRecoveryModule is BaseAuthorizationModule {
             : validAfter;
         if (validUntil < validAfter)
             revert InvalidTimeFrame(validUntil, validAfter);
-        if (validUntil < block.timestamp) revert ExpiredValidUntil(validUntil);
         return (validUntil, validAfter);
     }
 

test/module/AccountRecovery.Module.specs.ts

@@ -2054,20 +2054,17 @@ describe("Account Recovery Module: ", async () => {
   });
 
   describe("addGuardian", async () => {
-    let newGuardian: String;
+    let newGuardian: string;
 
-    before (async () => {
-      const {
-        controlMessage,
-      } = await setupTests();
+    before(async () => {
+      const { controlMessage } = await setupTests();
 
       const messageHash = ethers.utils.id(controlMessage);
       const messageHashBytes = ethers.utils.arrayify(messageHash);
 
       newGuardian = ethers.utils.keccak256(
         await eve.signMessage(messageHashBytes)
       );
-
     });
 
     it("Can add a guardian and proper event is emitted", async () => {
@@ -2134,98 +2131,137 @@ describe("Account Recovery Module: ", async () => {
       expect(guardiansAfter).to.equal(guardiansBefore + 1);
     });
 
-
     it("Should revert if zero guardian is provided", async () => {
-      const {
-        userSA,
-        accountRecoveryModule,
-      } = await setupTests();
+      const { accountRecoveryModule } = await setupTests();
 
-      //assign empty bytes32
+      // assign empty bytes32
       const zeroGuardian = ethers.utils.hexZeroPad("0x", 32);
-      const guardiansCountBefore = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount;
-
-      await expect(accountRecoveryModule.addGuardian(
-        zeroGuardian,
-        16741936496,
-        0,
-      )).to.be.revertedWith("ZeroGuardian");
-
-      const guardiansCountAfter = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount;
+      const guardiansCountBefore = (
+        await accountRecoveryModule.getSmartAccountSettings(deployer.address)
+      ).guardiansCount;
+
+      await expect(
+        accountRecoveryModule.addGuardian(zeroGuardian, 16741936496, 0)
+      ).to.be.revertedWith("ZeroGuardian");
+
+      const guardiansCountAfter = (
+        await accountRecoveryModule.getSmartAccountSettings(deployer.address)
+      ).guardiansCount;
       expect(guardiansCountAfter).to.equal(guardiansCountBefore);
     });
 
     it("Should revert if such a guardian has already been set", async () => {
-      const {
-        accountRecoveryModule,
-        guardians
-      } = await setupTests();
+      const { accountRecoveryModule, guardians } = await setupTests();
 
       const guardian = guardians[1];
 
-      //add guardian first
-      await accountRecoveryModule.addGuardian(
-        guardian,
-        15555934444,
-        0,
-      );
+      // add guardian first
+      await accountRecoveryModule.addGuardian(guardian, 15555934444, 0);
 
-      const guardiansCountBefore = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount;
+      const guardiansCountBefore = (
+        await accountRecoveryModule.getSmartAccountSettings(deployer.address)
+      ).guardiansCount;
 
-      await expect(accountRecoveryModule.addGuardian(
-        guardian,
-        15555934444,
-        0,
-      )).to.be.revertedWith("GuardianAlreadySet")
-        .withArgs(
-          guardian,
-          deployer.address
-        );
-
-      const guardiansCountAfter = (await accountRecoveryModule.getSmartAccountSettings(deployer.address)).guardiansCount;
+      await expect(accountRecoveryModule.addGuardian(guardian, 15555934444, 0))
+        .to.be.revertedWith("GuardianAlreadySet")
+        .withArgs(guardian, deployer.address);
+
+      const guardiansCountAfter = (
+        await accountRecoveryModule.getSmartAccountSettings(deployer.address)
+      ).guardiansCount;
       expect(guardiansCountAfter).to.equal(guardiansCountBefore);
     });
 
     it("Should set validUntil to uint48.max if validUntil = 0 is provided", async () => {
-      const {
-        accountRecoveryModule,
-      } = await setupTests();
+      const { accountRecoveryModule } = await setupTests();
 
       const validUntil = 0;
       const validAfter = 0;
 
-      //add guardian first
+      // add guardian first
       await accountRecoveryModule.addGuardian(
         newGuardian,
         validUntil,
-        validAfter,
+        validAfter
       );
 
-      const newGuardianTimeFrame = await accountRecoveryModule.getGuardianParams(
+      const newGuardianTimeFrame =
+        await accountRecoveryModule.getGuardianParams(
+          newGuardian,
+          deployer.address
+        );
+
+      expect(newGuardianTimeFrame.validUntil).to.equal(2 ** 48 - 1);
+    });
+
+    it("Should set validAfter as guardian.timeframe.validAfter if it is bigger than now+securityDelay", async () => {
+      const { accountRecoveryModule, defaultSecurityDelay } = await setupTests();
+      await accountRecoveryModule.setSecurityDelay(defaultSecurityDelay);
+
+      const nowPlusSecurityDelay = (
+        await ethers.provider.getBlock("latest")
+      ).timestamp + defaultSecurityDelay;
+
+      const validAfter = nowPlusSecurityDelay + 1000;
+
+      await accountRecoveryModule.addGuardian(newGuardian, 15555934444, validAfter);
+
+      const guardianDetails = await accountRecoveryModule.getGuardianParams(
         newGuardian,
         deployer.address
       );
 
-      expect(newGuardianTimeFrame.validUntil).to.equal(2**48-1);
+      expect(guardianDetails.validAfter).to.equal(validAfter);
     });
 
-
-    /*
-    it("Should set validAfter as guardian.timeframe.validAfter if it is bigger than now+securityDelay", async () => {});
+    it("Should set now+securityDelay if validAfter is less than it", async () => {
+      const { accountRecoveryModule, defaultSecurityDelay } = await setupTests();
+      await accountRecoveryModule.setSecurityDelay(defaultSecurityDelay);
+
+      const nowPlusSecurityDelay = (
+        await ethers.provider.getBlock("latest")
+      ).timestamp + defaultSecurityDelay;
+
+      const validAfter = nowPlusSecurityDelay - 1000;
+
+      await accountRecoveryModule.addGuardian(newGuardian, 15555934444, validAfter);
 
-    it("Should set now+securityDelay if validAfter is less than it", async () => {});
+      const guardianDetails = await accountRecoveryModule.getGuardianParams(
+        newGuardian,
+        deployer.address
+      );
 
-    it("Should revert if validUntil is less than resulting validAfter", async () => {});
+      expect(guardianDetails.validAfter).to.be.gte(nowPlusSecurityDelay);
+    });
 
-    it("Should revert if validUntil < now", async () => {});
-    */
+    it("Should revert if validUntil is less than resulting validAfter", async () => {
+      const { accountRecoveryModule, defaultSecurityDelay } = await setupTests();
 
-    // it("_________", async () => {});
+      const nowPlusSecurityDelay = (
+        await ethers.provider.getBlock("latest")
+      ).timestamp + defaultSecurityDelay;
+
+      const validAfter = nowPlusSecurityDelay + 1000;
+      const validUntil = nowPlusSecurityDelay - 1000;
+
+      await expect(accountRecoveryModule.addGuardian(newGuardian, validUntil, validAfter)).to.be.revertedWith("InvalidTimeFrame")
+        .withArgs(validUntil, validAfter);
+    });
   });
 
   /*
   describe("replaceGuardian", async () => {
-    it("_________", async () => {});
+
+    it("Can replace guardian, old is deleted, new is active, guardians count has not changed and events are emitted", async () => {});
+
+    it("reverts if guardian has not been set", async () => {});
+
+    it("reverts if guardian to replace and new guardian are identical", async () => {});
+
+    it("reverts if guardian to replace is zero", async () => {});
+
+    it("reverts if new guardian is zero", async () => {});
+
   });
 
   describe("removeGuardian", async () => {