feat: add functions to free slots of expired memberships, withdraw available balance

Also adds some natspec and renames functions

contracts/Membership.sol

@@ -13,6 +13,7 @@ error TokenMismatch();
 error InvalidRateLimit();
 error ExceedMaxRateLimitPerEpoch();
 error NotInGracePeriod(uint256 membershipMapIdx);
+error NotExpired(uint256 membershipMapIdx);
 error NotHolder(uint256 membershipMapIdx);
 
 error InsufficientBalance();
@@ -24,13 +25,23 @@ contract Membership {
     // TODO START - add owned setters to all these variables
 
     IPriceCalculator public priceCalculator;
+
+    /// @notice Maximum total rate limit of all memberships in the tree
     uint32 public maxTotalRateLimitPerEpoch; // TO-ASK: what's the theoretical maximum rate limit per epoch we could set? uint32 accepts a max of 4292967295
+
+    /// @notice Maximum rate limit of one membership
     uint16 public maxRateLimitPerMembership; // TO-ASK: what's the theoretical maximum rate limit per epoch a single membership can have? this accepts 65535
+
+    /// @notice Minimum rate limit of one membership
     uint16 public minRateLimitPerMembership; // TO-ASK: what's the theoretical largest minimum rate limit per epoch a single membership can have? this accepts a minimum from 0 to 65535
 
     // TO-ASK: what happens with existing memberships if
     // the expiration term and grace period are updated?
+
+    /// @notice Membership expiration term
     uint256 public expirationTerm;
+
+    /// @notice Membership grace period
     uint256 public gracePeriod;
 
     // TODO END
@@ -41,24 +52,22 @@ contract Membership {
     // mapping and also remove the token setter and the `token`
     // attribute from the MembershipDetails
 
-    // holder ->  token -> balance
+    /// @notice balances available to withdraw
-    mapping(address => mapping(address => uint)) public expiredBalances;
+    mapping(address => mapping(address => uint)) public balancesToWithdraw; // holder ->  token -> balance
-
-    enum MembershipStatus {
-        // TODO use in getter to determine state of membership?
-        NonExistent,
-        Active,
-        GracePeriod,
-        Expired,
-        ErasedAwaitsWithdrawal,
-        Erased
-    }
 
+    /// @notice Total rate limit of all memberships in the tree
     uint public totalRateLimitPerEpoch;
 
+    /// @notice List of registered memberships
     mapping(uint256 => MembershipDetails) public memberships;
-    uint256 public head = 0;
+
-    uint256 public tail = 0;
+    /// @dev Oldest membership
+    uint256 private head = 0;
+
+    /// @dev Newest membership
+    uint256 private tail = 0;
+
+    /// @dev Autoincrementing ID for memberships
     uint256 private nextID = 0;
 
     // TODO: associate membership details with commitment
@@ -96,18 +105,18 @@ contract Membership {
         gracePeriod = _gracePeriod;
     }
 
-    function registerMembership(
+    function _addMembership(
         address _sender,
         uint256[] memory commitments,
         uint16 _rateLimit
     ) internal {
         // TODO: for each commitment
         (address token, uint256 amount) = priceCalculator.calculate(_rateLimit);
-        acquireRateLimit(_sender, commitments, _rateLimit, token, amount);
+        _setupMembershipDetails(_sender, commitments, _rateLimit, token, amount);
-        transferFees(_sender, token, amount * _rateLimit * commitments.length);
+        _transferFees(_sender, token, amount * _rateLimit * commitments.length);
     }
 
-    function transferFees(address _from, address _token, uint256 _amount) internal {
+    function _transferFees(address _from, address _token, uint256 _amount) internal {
         if (_token == address(0)) {
             if (msg.value != _amount) revert IncorrectAmount();
         } else {
@@ -116,7 +125,7 @@ contract Membership {
         }
     }
 
-    function acquireRateLimit(
+    function _setupMembershipDetails(
         address _sender,
         uint256[] memory _commitments,
         uint16 _rateLimit,
@@ -142,18 +151,23 @@ contract Membership {
                 // Deduct the expired membership rate limit
                 totalRateLimitPerEpoch -= oldestMembershipDetails.rateLimit;
 
-                // Remove the expired membership
+                // Remove expired membership references
-                uint256 nextOld = oldestMembershipDetails.next;
+                uint256 detailsNext = oldestMembershipDetails.next;
-                if (nextOld != 0) memberships[nextOld].prev = 0;
+                uint256 detailsPrev = oldestMembershipDetails.prev;
-
+                if (detailsPrev != 0) {
-                if (tail == head) {
+                    memberships[detailsPrev].next = detailsNext;
-                    // TODO: test this
+                } else {
-                    tail = 0;
+                    head = detailsNext;
+                }
+
+                if (detailsNext != 0) {
+                    memberships[detailsNext].prev = detailsPrev;
+                } else {
+                    tail = detailsPrev;
                 }
-                head = nextOld;
 
                 // Move balance from expired membership to holder balance
-                expiredBalances[oldestMembershipDetails.holder][
+                balancesToWithdraw[oldestMembershipDetails.holder][
                     oldestMembershipDetails.token
                 ] += oldestMembershipDetails.amount;
 
@@ -190,7 +204,7 @@ contract Membership {
         tail = nextID;
     }
 
-    function extendMembership(address _sender, uint256[] calldata membershipMapIdx) public {
+    function _extendMembership(address _sender, uint256[] calldata membershipMapIdx) public {
         for (uint256 i = 0; i < membershipMapIdx.length; i++) {
             uint256 idx = membershipMapIdx[i];
 
@@ -247,34 +261,67 @@ contract Membership {
         return _isGracePeriod(expirationDate);
     }
 
-    function freeExpiredMemberships(uint256[] calldata expiredMemberships) public {
+    function eraseExpiredMemberships(uint256[] calldata expiredMembershipsIdx) public {
-        // TODO: user can pass a list of expired memberships and free them
         // Might be useful because then offchain the user can determine which
         // expired memberships slots are available, and proceed to free them.
         // This might be cheaper than the `while` loop used when registering
         // memberships, although easily solved by having a function that receives
         // the list of memberships to free, and the information for the new
         // membership to register
-    }
 
-    // TODO: expire owned memberships?
+        for (uint256 i = 0; i < expiredMembershipsIdx.length; i++) {
+            uint256 idx = expiredMembershipsIdx[i];
+            MembershipDetails memory mdetails = memberships[idx];
 
-    function withdraw(address token) public {
+            if (!_isExpired(mdetails.expirationDate)) revert NotExpired(idx);
-        // TODO: getSender()  
-        uint256 amount = expiredBalances[msg.sender][token];
-        require(amount > 0, "Insufficient balance");
 
-        expiredBalances[msg.sender][token] = 0;
+            // TODO: this code is repeated in other places, maybe it
-        if (token == address(0)) {
+            // makes sense to extract to an internal function?
-            // ETH
+
-            (bool success, ) = msg.sender.call{value: amount}("");
+            // Move balance from expired membership to holder balance
-            require(success, "eth transfer failed");
+            balancesToWithdraw[mdetails.holder][mdetails.token] += mdetails.amount;
-        } else {
+
-            IERC20(token).safeTransfer(msg.sender, amount);
+            // Deduct the expired membership rate limit
+            totalRateLimitPerEpoch -= mdetails.rateLimit;
+
+            // Remove current membership references
+            if (mdetails.prev != 0) {
+                memberships[mdetails.prev].next = mdetails.next;
+            } else {
+                head = mdetails.next;
+            }
+
+            if (mdetails.next != 0) {
+                memberships[mdetails.next].prev = mdetails.prev;
+            } else {
+                tail = mdetails.prev;
+            }
+
+            delete memberships[idx];
         }
     }
 
-    function getOldestMembership() public view returns (MembershipDetails memory) {
+    // TODO: withdraw grace period or expired memberships
+    //       should be similar to previous function except that
+    //       it will check if the membership is in grace period
+    //       or is expired, and also if it's owned by whoever calls
+    //       the function.
+
+    function _withdraw(address _sender, address token) internal {
+        uint256 amount = balancesToWithdraw[_sender][token];
+        require(amount > 0, "Insufficient balance");
+
+        balancesToWithdraw[_sender][token] = 0;
+        if (token == address(0)) {
+            // ETH
+            (bool success, ) = _sender.call{value: amount}("");
+            require(success, "eth transfer failed");
+        } else {
+            IERC20(token).safeTransfer(_sender, amount);
+        }
+    }
+
+    function oldestMembership() public view returns (MembershipDetails memory) {
         return memberships[head];
     }
 }