From 66a1a6c48ce10d8e6e7d7608bdd283edd69cd698 Mon Sep 17 00:00:00 2001
From: Richard Ramos <info@richardramos.me>
Date: Mon, 7 Oct 2024 16:13:15 -0400
Subject: [PATCH] code review

---
 src/Membership.sol   | 14 +++++++-------
 src/WakuRlnV2.sol    |  1 -
 test/WakuRlnV2.t.sol | 10 +++++-----
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/src/Membership.sol b/src/Membership.sol
index 614253f..27fc29f 100644
--- a/src/Membership.sol
+++ b/src/Membership.sol
@@ -11,13 +11,13 @@ error InvalidMembershipRateLimit();
 
 // Cannot acquire the rate limit for a new membership due to exceeding the expected limits
 // even after attempting to erase expired memberships
-error ExceededMaxTotalRateLimit();
+error CannotExceedMaxTotalRateLimit();
 
 // This membership is not in its grace period
-error NotInGracePeriod(uint256 idCommitment);
+error CannotExtendActiveMembership(uint256 idCommitment);
 
 // The sender is not the holder of this membership
-error AttemptedExtensionByNonHolder(uint256 idCommitment);
+error NonHolderCannotExtend(uint256 idCommitment);
 
 // This membership cannot be erased
 error CannotEraseMembership(uint256 idCommitment);
@@ -161,7 +161,7 @@ abstract contract MembershipUpgradeable is Initializable {
         gracePeriodDurationForNewMemberships = _gracePeriodDurationForNewMemberships;
     }
 
-    /// @dev acquire a membership and trasnfer the deposit to the contract
+    /// @dev acquire a membership and transfer the deposit to the contract
     /// @param _sender the address of the transaction sender
     /// @param _idCommitment the idCommitment of the new membership
     /// @param _rateLimit the membership rate limit
@@ -184,7 +184,7 @@ abstract contract MembershipUpgradeable is Initializable {
 
         // Determine if we exceed the total rate limit
         if (currentTotalRateLimit > maxTotalRateLimit) {
-            revert ExceededMaxTotalRateLimit();
+            revert CannotExceedMaxTotalRateLimit();
         }
 
         (address token, uint256 depositAmount) = priceCalculator.calculate(_rateLimit);
@@ -235,10 +235,10 @@ abstract contract MembershipUpgradeable is Initializable {
         MembershipInfo storage membership = memberships[_idCommitment];
 
         if (!_isInPeriod(membership.gracePeriodStartTimestamp, membership.gracePeriodDuration)) {
-            revert NotInGracePeriod(_idCommitment);
+            revert CannotExtendActiveMembership(_idCommitment);
         }
 
-        if (_sender != membership.holder) revert AttemptedExtensionByNonHolder(_idCommitment);
+        if (_sender != membership.holder) revert NonHolderCannotExtend(_idCommitment);
 
         // Note: we add the new active period to the end of the ongoing grace period
         uint256 newGracePeriodStartTimestamp =
diff --git a/src/WakuRlnV2.sol b/src/WakuRlnV2.sol
index a4b877b..efaed97 100644
--- a/src/WakuRlnV2.sol
+++ b/src/WakuRlnV2.sol
@@ -57,7 +57,6 @@ contract WakuRlnV2 is Initializable, Ownable2StepUpgradeable, UUPSUpgradeable, M
         _;
     }
 
-
     constructor() {
         _disableInitializers();
     }
diff --git a/test/WakuRlnV2.t.sol b/test/WakuRlnV2.t.sol
index ea92926..1f9e6f5 100644
--- a/test/WakuRlnV2.t.sol
+++ b/test/WakuRlnV2.t.sol
@@ -213,7 +213,7 @@ contract WakuRlnV2Test is Test {
         // Attempt to extend the membership (but it is not owned by us)
         address randomAddress = vm.addr(block.timestamp);
         vm.prank(randomAddress);
-        vm.expectRevert(abi.encodeWithSelector(AttemptedExtensionByNonHolder.selector, commitmentsToExtend[0]));
+        vm.expectRevert(abi.encodeWithSelector(NonHolderCannotExtend.selector, commitmentsToExtend[0]));
         w.extendMemberships(commitmentsToExtend);
 
         // Attempt to extend the membership (but now we are the owner)
@@ -238,7 +238,7 @@ contract WakuRlnV2Test is Test {
         token.approve(address(w), price);
         w.register(idCommitment + 1, membershipRateLimit);
         commitmentsToExtend[0] = idCommitment + 1;
-        vm.expectRevert(abi.encodeWithSelector(NotInGracePeriod.selector, commitmentsToExtend[0]));
+        vm.expectRevert(abi.encodeWithSelector(CannotExtendActiveMembership.selector, commitmentsToExtend[0]));
         w.extendMemberships(commitmentsToExtend);
     }
 
@@ -367,7 +367,7 @@ contract WakuRlnV2Test is Test {
         token.approve(address(w), priceB);
 
         // Should fail. There's not enough free rate limit
-        vm.expectRevert(abi.encodeWithSelector(ExceededMaxTotalRateLimit.selector));
+        vm.expectRevert(abi.encodeWithSelector(CannotExceedMaxTotalRateLimit.selector));
         w.register(6, 60);
 
         // Attempt to erase 3 memberships including one that can't be erased (the last one)
@@ -438,7 +438,7 @@ contract WakuRlnV2Test is Test {
         membershipRateLimit = 2;
         (, price) = w.priceCalculator().calculate(membershipRateLimit);
         token.approve(address(w), price);
-        vm.expectRevert(abi.encodeWithSelector(ExceededMaxTotalRateLimit.selector));
+        vm.expectRevert(abi.encodeWithSelector(CannotExceedMaxTotalRateLimit.selector));
         w.register(3, membershipRateLimit);
 
         // Should register succesfully
@@ -451,7 +451,7 @@ contract WakuRlnV2Test is Test {
         membershipRateLimit = 1;
         (, price) = w.priceCalculator().calculate(membershipRateLimit);
         token.approve(address(w), price);
-        vm.expectRevert(abi.encodeWithSelector(ExceededMaxTotalRateLimit.selector));
+        vm.expectRevert(abi.encodeWithSelector(CannotExceedMaxTotalRateLimit.selector));
         w.register(4, membershipRateLimit);
     }