Apply suggestions from code review

Co-authored-by: Derek Pierre <[email protected]>
nucypher · Jun 7, 2024 · 37033a8 · 37033a8
1 parent 4d94e32
commit 37033a8
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 21 deletions.
diff --git a/contracts/contracts/coordination/BqETHSubscription.sol b/contracts/contracts/coordination/BqETHSubscription.sol
@@ -46,8 +46,9 @@ contract BqETHSubscription is IFeeModel {
      * @notice Emitted when a subscription is paid
      * @param subscriber The address of the subscriber
      * @param amount The amount paid
+     * @param endOfSubscription End timestamp of subscription
      */
-    event SubscriptionPaid(address indexed subscriber, uint256 amount);
+    event SubscriptionPaid(address indexed subscriber, uint256 amount, uint32 endOfSubscription);
 
     /**
      * @notice Sets the coordinator and fee token contracts
@@ -117,7 +118,7 @@ contract BqETHSubscription is IFeeModel {
     function initialize(IEncryptionAuthorizer _accessController) external {
         require(
             address(accessController) == address(0) && address(_accessController) != address(0),
-            "Access controller cannot be the zero address"
+            "Access controller not already set and parameter cannot be the zero address"
         );
         accessController = _accessController;
         activeRitualId = INACTIVE_RITUAL_ID;
@@ -130,7 +131,7 @@ contract BqETHSubscription is IFeeModel {
     /**
      * @notice Pays for a subscription
      */
-    function paySubscriptionFor() external {
+    function payForSubscription() external {
         // require(endOfSubscription == 0, "Subscription already payed");
         uint256 amount = packageFees();
         if (endOfSubscription == 0) {
@@ -139,15 +140,15 @@ contract BqETHSubscription is IFeeModel {
         endOfSubscription += uint32(maxDuration);
 
         feeToken.safeTransferFrom(msg.sender, address(this), amount);
-        emit SubscriptionPaid(msg.sender, amount);
+        emit SubscriptionPaid(msg.sender, amount, endOfSubscription);
     }
 
     /**
      * @notice Withdraws the contract balance to the beneficiary
      * @param amount The amount to withdraw
      */
     function withdrawToBeneficiary(uint256 amount) external onlyBeneficiary {
-        require(amount <= feeToken.balanceOf(address(this)), "Insufficient available amount");
+        require(amount <= feeToken.balanceOf(address(this)), "Insufficient balance available");
         feeToken.safeTransfer(beneficiary, amount);
         emit WithdrawalToBeneficiary(beneficiary, amount);
     }
@@ -159,7 +160,7 @@ contract BqETHSubscription is IFeeModel {
         uint32 duration
     ) external override onlyCoordinator {
         require(initiator == adopter, "Only adopter can initiate ritual");
-        require(endOfSubscription != 0, "Subscription has to be payed first");
+        require(endOfSubscription != 0, "Subscription has to be paid first");
         require(
             endOfSubscription + yellowPeriodDuration + redPeriodDuration >=
                 block.timestamp + duration &&
@@ -178,7 +179,7 @@ contract BqETHSubscription is IFeeModel {
                 state == Coordinator.RitualState.DKG_INVALID ||
                     state == Coordinator.RitualState.DKG_TIMEOUT ||
                     state == Coordinator.RitualState.EXPIRED, // TODO check if it's ok
-                "Only failed rituals allowed to be reinitiate"
+                "Only failed rituals allowed to be reinitiated"
             );
         }
         activeRitualId = ritualId;

diff --git a/tests/test_bqeth_subscription.py b/tests/test_bqeth_subscription.py
@@ -109,26 +109,34 @@ def test_pay_subscription(erc20, subscription, adopter, chain):
     balance_before = erc20.balanceOf(adopter)
     assert subscription.packageFees() == FEE
 
-    tx = subscription.paySubscriptionFor(sender=adopter)
+    tx = subscription.payForSubscription(sender=adopter)
     timestamp = chain.pending_timestamp - 1
     assert subscription.endOfSubscription() == timestamp + MAX_DURATION
     balance_after = erc20.balanceOf(adopter)
     assert balance_after + FEE == balance_before
     assert erc20.balanceOf(subscription.address) == FEE
 
     events = subscription.SubscriptionPaid.from_receipt(tx)
-    assert events == [subscription.SubscriptionPaid(subscriber=adopter, amount=FEE)]
+    assert events == [
+        subscription.SubscriptionPaid(
+            subscriber=adopter, amount=FEE, endOfSubscription=timestamp + MAX_DURATION
+        )
+    ]
 
     # Top up
     balance_before = erc20.balanceOf(adopter)
-    tx = subscription.paySubscriptionFor(sender=adopter)
+    tx = subscription.payForSubscription(sender=adopter)
     assert subscription.endOfSubscription() == timestamp + 2 * MAX_DURATION
     balance_after = erc20.balanceOf(adopter)
     assert balance_after + FEE == balance_before
     assert erc20.balanceOf(subscription.address) == 2 * FEE
 
     events = subscription.SubscriptionPaid.from_receipt(tx)
-    assert events == [subscription.SubscriptionPaid(subscriber=adopter, amount=FEE)]
+    assert events == [
+        subscription.SubscriptionPaid(
+            subscriber=adopter, amount=FEE, endOfSubscription=timestamp + 2 * MAX_DURATION
+        )
+    ]
 
 
 def test_withdraw(erc20, subscription, adopter, treasury):
@@ -137,12 +145,12 @@ def test_withdraw(erc20, subscription, adopter, treasury):
     with ape.reverts("Only the beneficiary can call this method"):
         subscription.withdrawToBeneficiary(1, sender=adopter)
 
-    with ape.reverts("Insufficient available amount"):
+    with ape.reverts("Insufficient balance available"):
         subscription.withdrawToBeneficiary(1, sender=treasury)
 
-    subscription.paySubscriptionFor(sender=adopter)
+    subscription.payForSubscription(sender=adopter)
 
-    with ape.reverts("Insufficient available amount"):
+    with ape.reverts("Insufficient balance available"):
         subscription.withdrawToBeneficiary(FEE + 1, sender=treasury)
 
     tx = subscription.withdrawToBeneficiary(FEE, sender=treasury)
@@ -167,13 +175,13 @@ def test_process_ritual_payment(
         coordinator.processRitualPayment(
             treasury, ritual_id, number_of_providers, DURATION, sender=treasury
         )
-    with ape.reverts("Subscription has to be payed first"):
+    with ape.reverts("Subscription has to be paid first"):
         coordinator.processRitualPayment(
             adopter, ritual_id, number_of_providers, DURATION, sender=treasury
         )
 
     erc20.approve(subscription.address, 10 * FEE, sender=adopter)
-    subscription.paySubscriptionFor(sender=adopter)
+    subscription.payForSubscription(sender=adopter)
 
     with ape.reverts("Ritual parameters exceed available in package"):
         coordinator.processRitualPayment(
@@ -272,7 +280,7 @@ def test_process_ritual_extending(
         )
 
     erc20.approve(subscription.address, 10 * FEE, sender=adopter)
-    subscription.paySubscriptionFor(sender=adopter)
+    subscription.payForSubscription(sender=adopter)
     coordinator.setRitual(
         ritual_id, RitualState.ACTIVE, 0, global_allow_list.address, sender=treasury
     )
@@ -346,7 +354,7 @@ def test_before_set_authorization(
         global_allow_list.authorize(0, [creator.address], sender=adopter)
 
     erc20.approve(subscription.address, 10 * FEE, sender=adopter)
-    subscription.paySubscriptionFor(sender=adopter)
+    subscription.payForSubscription(sender=adopter)
     coordinator.setRitual(
         ritual_id, RitualState.ACTIVE, 0, global_allow_list.address, sender=treasury
     )
@@ -364,7 +372,7 @@ def test_before_set_authorization(
     with ape.reverts("Subscription has expired"):
         global_allow_list.authorize(ritual_id, [creator.address], sender=adopter)
 
-    subscription.paySubscriptionFor(sender=adopter)
+    subscription.payForSubscription(sender=adopter)
     global_allow_list.authorize(ritual_id, [creator.address], sender=adopter)
 
 
@@ -389,7 +397,7 @@ def test_before_is_authorized(
         global_allow_list.isAuthorized(0, bytes(signature), bytes(data))
 
     erc20.approve(subscription.address, 10 * FEE, sender=adopter)
-    subscription.paySubscriptionFor(sender=adopter)
+    subscription.payForSubscription(sender=adopter)
     coordinator.setRitual(
         ritual_id, RitualState.ACTIVE, 0, global_allow_list.address, sender=treasury
     )
@@ -406,5 +414,5 @@ def test_before_is_authorized(
     with ape.reverts("Yellow period has expired"):
         global_allow_list.isAuthorized(ritual_id, bytes(signature), bytes(data))
 
-    subscription.paySubscriptionFor(sender=adopter)
+    subscription.payForSubscription(sender=adopter)
     assert global_allow_list.isAuthorized(ritual_id, bytes(signature), bytes(data))