fix(contract): bug with ordering of operations (#100)

* refactor: remove unnecessary `min` and rename `claim_amount`

* refactor(contracts): ensure cancel_stream reverts when empty/claimed

* refactor(contracts,sdk): allow owner to cancel immediately

NOTE: Needed to remove exception since it's not possible to tell anymore

* refactor(sdk): change name to match contract

* refactor(test): make `MIN_STREAM_LIFE` a fixture parameter

* refactor(test): make `create_stream` fixture and use it more

* refactor(test): add tests for stream cancelation

contracts/StreamManager.vy

@@ -219,17 +219,21 @@ def cancel_stream(
     reason: Bytes[MAX_REASON_SIZE] = b"",
     creator: address = msg.sender,
 ) -> uint256:
-    assert msg.sender == creator or msg.sender == self.owner
-    assert self.streams[creator][stream_id].start_time + MIN_STREAM_LIFE <= block.timestamp
+    if msg.sender == creator:
+        # Creator needs to wait `MIN_STREAM_LIFE` to cancel a stream
+        assert self.streams[creator][stream_id].start_time + MIN_STREAM_LIFE <= block.timestamp
+    else:
+        # Owner can cancel at any time
+        assert msg.sender == self.owner
 
     funded_amount: uint256 = self.streams[creator][stream_id].funded_amount
-    amount_locked: uint256 = funded_amount  - self._amount_unlocked(creator, stream_id)
+    amount_locked: uint256 = funded_amount - self._amount_unlocked(creator, stream_id)
+    assert amount_locked > 0  # NOTE: reverts if stream doesn't exist, or already cancelled
+    self.streams[creator][stream_id].funded_amount = funded_amount - amount_locked
 
     token: ERC20 = self.streams[creator][stream_id].token
     assert token.transfer(creator, amount_locked, default_return_value=True)
 
-    self.streams[creator][stream_id].funded_amount = funded_amount - amount_locked
-
     log StreamCancelled(creator, stream_id, amount_locked, reason)
 
     return funded_amount - amount_locked
@@ -238,17 +242,13 @@ def cancel_stream(
 @external
 def claim(creator: address, stream_id: uint256) -> uint256:
     funded_amount: uint256 = self.streams[creator][stream_id].funded_amount
-    claimed_amount: uint256 = min(
-        self._amount_unlocked(creator, stream_id),
-        funded_amount,
-    )
+    claim_amount: uint256 = self._amount_unlocked(creator, stream_id)
+    self.streams[creator][stream_id].funded_amount = funded_amount - claim_amount
+    self.streams[creator][stream_id].last_pull = block.timestamp
 
     token: ERC20 = self.streams[creator][stream_id].token
-    assert token.transfer(self.owner, claimed_amount, default_return_value=True)
-
-    self.streams[creator][stream_id].funded_amount = funded_amount - claimed_amount
-    self.streams[creator][stream_id].last_pull = block.timestamp
+    assert token.transfer(self.owner, claim_amount, default_return_value=True)
 
-    log Claimed(creator, stream_id, funded_amount == claimed_amount, claimed_amount)
+    log Claimed(creator, stream_id, funded_amount == claim_amount, claim_amount)
 
-    return claimed_amount
+    return claim_amount

sdk/py/apepay/__init__.py

@@ -24,7 +24,6 @@
     FundsNotClaimable,
     MissingCreationReceipt,
     StreamLifeInsufficient,
-    StreamNotCancellable,
     TokenNotAccepted,
     ValidatorFailed,
 )
@@ -422,7 +421,7 @@ def amount_unlocked(self) -> int:
         return self.contract.amount_unlocked(self.creator, self.stream_id)
 
     @property
-    def amount_left(self) -> int:
+    def amount_locked(self) -> int:
         return self.info.funded_amount - self.amount_unlocked
 
     @property
@@ -460,9 +459,6 @@ def is_cancelable(self) -> bool:
 
     @property
     def cancel(self) -> ContractTransactionHandler:
-        if not self.is_cancelable:
-            raise StreamNotCancellable(self.time_left)
-
         return cast(
             ContractTransactionHandler,
             partial(self.contract.cancel_stream, self.stream_id),

sdk/py/apepay/exceptions.py

@@ -14,11 +14,6 @@ def __init__(self):
         super().__init__("Missing creation transaction for stream. Functionality unavailabie.")
 
 
-class StreamNotCancellable(ApePayException):
-    def __init__(self, time_left: timedelta):
-        super().__init__(f"Stream not cancelable yet. Please wait: {time_left}")
-
-
 class FundsNotClaimable(ApePayException):
     def __init__(self):
         super().__init__("Stream has no funds left to claim.")

tests/conftest.py

@@ -1,9 +1,6 @@
 import pytest
 from apepay import StreamManager
 
-ONE_HOUR = 60 * 60
-
-
 @pytest.fixture(scope="session")
 def owner(accounts):
     return accounts[0]
@@ -27,6 +24,20 @@ def tokens(create_token, payer, request):
     return [create_token(payer) for _ in range(int(request.param.split(" ")[0]) + 1)]
 
 
+@pytest.fixture(scope="session")
+def token(tokens):
+    if len(tokens) == 0:
+        pytest.skip("No valid tokens")
+
+    return tokens[0]
+
+
+@pytest.fixture(scope="session")
+def starting_balance(token, payer):
+    # NOTE: All tokens start with the same balance
+    return token.balanceOf(payer)
+
+
 @pytest.fixture(scope="session")
 def create_validator(owner, project):
     def create_validator():
@@ -41,8 +52,13 @@ def validators(create_validator, request):
 
 
 @pytest.fixture(scope="session")
-def stream_manager_contract(owner, project, validators, tokens):
-    return owner.deploy(project.StreamManager, owner, ONE_HOUR, validators, tokens)
+def MIN_STREAM_LIFE():
+    return 60 * 60  # 1 hour in seconds
+
+
+@pytest.fixture(scope="session")
+def stream_manager_contract(owner, project, MIN_STREAM_LIFE, validators, tokens):
+    return owner.deploy(project.StreamManager, owner, MIN_STREAM_LIFE, validators, tokens)
 
 
 @pytest.fixture(scope="session")

tests/test_stream.py

@@ -1,4 +1,5 @@
 from datetime import datetime, timedelta
+import ape
 
 import pytest
 from apepay import exceptions as apepay_exc
@@ -7,7 +8,6 @@
 def test_init(stream_manager, owner, validators, tokens):
     assert stream_manager.MIN_STREAM_LIFE == timedelta(hours=1)
     assert stream_manager.owner == owner
-
     assert stream_manager.validators == validators
 
     for token in tokens:
@@ -16,33 +16,47 @@ def test_init(stream_manager, owner, validators, tokens):
 
 def test_set_validators(stream_manager, owner, create_validator):
     new_validator = create_validator()
-
     assert new_validator not in stream_manager.validators
 
     stream_manager.add_validators(new_validator, sender=owner)
-
     assert new_validator in stream_manager.validators
 
     stream_manager.remove_validators(new_validator, sender=owner)
-
     assert new_validator not in stream_manager.validators
 
 
 def test_add_rm_tokens(stream_manager, owner, tokens, create_token):
     new_token = create_token(owner)
     assert new_token not in tokens
-
     assert not stream_manager.is_accepted(new_token)
 
     stream_manager.add_token(new_token, sender=owner)
-
     assert stream_manager.is_accepted(new_token)
 
     stream_manager.remove_token(new_token, sender=owner)
-
     assert not stream_manager.is_accepted(new_token)
 
 
+@pytest.fixture(scope="session")
+def create_stream(stream_manager, payer, MIN_STREAM_LIFE):
+    def create_stream(token=None, amount_per_second=None, sender=None, allowance=(2**256-1), **extra_args):
+        if amount_per_second is None:
+            # NOTE: Maximum amount we can afford to send (using 1 hr pre-allocation)
+            amount_per_second = token.balanceOf(sender or payer) // MIN_STREAM_LIFE
+
+        if token.allowance(sender or payer, stream_manager.contract) != allowance:
+            token.approve(stream_manager.contract, allowance, sender=sender or payer)
+
+        return stream_manager.create(
+            token,
+            amount_per_second,
+            **extra_args,
+            sender=sender or payer,
+        )
+
+    return create_stream
+
+
 @pytest.mark.parametrize(
     "extra_args",
     [
@@ -58,31 +72,59 @@ def test_add_rm_tokens(stream_manager, owner, tokens, create_token):
         dict(start_time=-1000),
     ],
 )
-def test_create_stream(chain, payer, tokens, stream_manager, extra_args):
-    if len(tokens) == 0:
-        pytest.skip("No valid tokens")
-
-    # NOTE: Maximum amount we can afford to send (using 1 hr pre-allocation)
-    amount_per_second = tokens[0].balanceOf(payer) // int(
-        stream_manager.MIN_STREAM_LIFE.total_seconds()
-    )
-
+def test_create_stream(chain, payer, token, create_stream, MIN_STREAM_LIFE, extra_args):
     with pytest.raises(apepay_exc.StreamLifeInsufficient):
-        stream_manager.create(tokens[0], amount_per_second, sender=payer)
+        create_stream(token, allowance=0, **extra_args)
 
-    tokens[0].approve(stream_manager.contract, 2**256 - 1, sender=payer)
+    amount_per_second = (token.balanceOf(payer) // MIN_STREAM_LIFE)
 
     with pytest.raises(apepay_exc.StreamLifeInsufficient):
-        stream_manager.create(tokens[0], amount_per_second + 1, sender=payer)
+        # NOTE: Performs approval
+        create_stream(token, amount_per_second=amount_per_second + 1, **extra_args)
 
-    stream = stream_manager.create(tokens[0], amount_per_second, **extra_args, sender=payer)
+    stream = create_stream(token, **extra_args)
     start_time = chain.blocks.head.timestamp
 
-    assert stream.token == tokens[0]
+    assert stream.token == token
     assert stream.stream_id == 0
     assert stream.creator == payer
     assert stream.amount_per_second == amount_per_second
     assert stream.reason == extra_args.get("reason", "")
+
     expected = datetime.fromtimestamp(start_time + extra_args.get("start_time", 0))
-    one_second = timedelta(seconds=1)
-    assert stream.start_time - expected <= one_second, "Unexpected start time"
+    assert stream.start_time - expected <= timedelta(seconds=1), "Unexpected start time"
+
+
+@pytest.fixture
+def stream(create_stream, token, payer, MIN_STREAM_LIFE):
+    # NOTE: Use 2 hour stream life
+    amount_per_second = token.balanceOf(payer) // (2 * MIN_STREAM_LIFE)
+    return create_stream(token, amount_per_second=amount_per_second)
+
+
+def test_cancel_stream(chain, token, payer, starting_balance, owner, MIN_STREAM_LIFE, stream):
+    with chain.isolate():
+        # Owner can cancel at any time
+        stream.cancel(b"Because I felt like it", payer, sender=owner)
+        assert token.balanceOf(stream.contract) == stream.amount_unlocked
+        assert token.balanceOf(payer) == starting_balance - stream.amount_unlocked
+        assert not stream.is_active
+
+    with ape.reverts():
+        # Payer has to wait `MIN_STREAM_LIFE`
+        stream.cancel(sender=payer)
+
+    chain.pending_timestamp += MIN_STREAM_LIFE
+
+    with chain.isolate():
+        # Owner can still cancel at any time
+        stream.cancel(b"Because I felt like it", payer, sender=owner)
+        assert token.balanceOf(stream.contract) == stream.amount_unlocked
+        assert token.balanceOf(payer) + stream.amount_unlocked == starting_balance
+        assert not stream.is_active
+
+    # Payer can cancel after `MIN_STREAM_LIFE`
+    stream.cancel(sender=payer)
+    assert token.balanceOf(stream.contract) == stream.amount_unlocked
+    assert token.balanceOf(payer) + stream.amount_unlocked == starting_balance
+    assert not stream.is_active