fix: split errors of _eraseMembershipLazily

waku-org · Oct 7, 2024 · 2a2c75f · 2a2c75f
1 parent b78a4c0
commit 2a2c75f
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 6 deletions.
diff --git a/src/Membership.sol b/src/Membership.sol
@@ -19,8 +19,11 @@ error CannotExtendActiveMembership(uint256 idCommitment);
 // The sender is not the holder of this membership
 error NonHolderCannotExtend(uint256 idCommitment);
 
-// This membership cannot be erased
-error CannotEraseMembership(uint256 idCommitment);
+// The membership is still active
+error CannotEraseActiveMembership(uint256 idCommitment);
+
+// The sender is not the holder of this membership
+error NonHolderCannotErase(uint256 idCommitment);
 
 // This membership does not exist
 error MembershipDoesNotExist(uint256 idCommitment);
@@ -264,8 +267,10 @@ abstract contract MembershipUpgradeable is Initializable {
             _isInPeriod(membership.gracePeriodStartTimestamp, membership.gracePeriodDuration);
         bool isHolder = (membership.holder == _sender);
 
-        if (!membershipExpired && !(membershipIsInGracePeriod && isHolder)) {
-            revert CannotEraseMembership(_idCommitment);
+        if (!membershipExpired && !membershipIsInGracePeriod) {
+            revert CannotEraseActiveMembership(_idCommitment);
+        } else if (membershipIsInGracePeriod && !isHolder) {
+            revert NonHolderCannotErase(_idCommitment);
         }
 
         // Move deposit balance from the membership to be erased to holder deposit balance

diff --git a/test/WakuRlnV2.t.sol b/test/WakuRlnV2.t.sol
@@ -378,7 +378,7 @@ contract WakuRlnV2Test is Test {
         commitmentsToErase[1] = 2;
         commitmentsToErase[2] = 5; // This one is still active
         token.approve(address(w), priceB);
-        vm.expectRevert(abi.encodeWithSelector(CannotEraseMembership.selector, 5));
+        vm.expectRevert(abi.encodeWithSelector(CannotEraseActiveMembership.selector, 5));
         w.register(6, 60, commitmentsToErase);
 
         // Attempt to erase 3 memberships that can be erased
@@ -463,14 +463,35 @@ contract WakuRlnV2Test is Test {
         (, uint256 price) = w.priceCalculator().calculate(20);
         uint32 index;
         uint256[] memory commitmentsToErase = new uint256[](idCommitmentsLength);
+        uint256 time = block.timestamp;
         for (uint256 i = 1; i <= idCommitmentsLength; i++) {
             token.approve(address(w), price);
             w.register(i, 20, noCommitments);
             (,,,,, index,,) = w.memberships(i);
             assertEq(index, w.nextFreeIndex() - 1);
             commitmentsToErase[i - 1] = i;
+            time += 100;
+            vm.warp(time);
+        }
+
+        // None of the commitments can be deleted because they're still active
+        uint256[] memory singleCommitmentToErase = new uint256[](1);
+        for (uint256 i = 1; i <= idCommitmentsLength; i++) {
+            singleCommitmentToErase[0] = i;
+            vm.expectRevert(abi.encodeWithSelector(CannotEraseActiveMembership.selector, i));
+            w.eraseMemberships(singleCommitmentToErase);
         }
 
+        // Fastfwd to commitment grace period, and try to erase it without being the owner
+        (,, uint256 gracePeriodStartTimestamp,,,,,) = w.memberships(1);
+        vm.warp(gracePeriodStartTimestamp);
+        assertTrue(w.isInGracePeriod(1));
+        singleCommitmentToErase[0] = 1;
+        address randomAddress = vm.addr(block.timestamp);
+        vm.prank(randomAddress);
+        vm.expectRevert(abi.encodeWithSelector(NonHolderCannotErase.selector, 1));
+        w.eraseMemberships(singleCommitmentToErase);
+
         // time travel to the moment we can erase all expired memberships
         uint256 membershipExpirationTimestamp = w.membershipExpirationTimestamp(idCommitmentsLength);
         vm.warp(membershipExpirationTimestamp);
@@ -562,7 +583,7 @@ contract WakuRlnV2Test is Test {
         vm.warp(gracePeriodStartTimestamp - 1);
         commitmentsToErase[0] = idCommitment;
         commitmentsToErase[1] = idCommitment + 4;
-        vm.expectRevert(abi.encodeWithSelector(CannotEraseMembership.selector, idCommitment + 4));
+        vm.expectRevert(abi.encodeWithSelector(CannotEraseActiveMembership.selector, idCommitment + 4));
         w.eraseMemberships(commitmentsToErase);
     }