feat: stateful tests now cover all base operations

* handle a couple of edge cases that occurred during stateful testing by either being more restrictive on the fuzzing space or adding some assumptions.

* significantly improved documentation of tests.

* "Loss" error was turned into a dev reason to save bytecode space and use a better explaination.

* added an hypothesis profile which helps when debugging tests. (documented in the README.md)

contracts/main/CurveTwocryptoOptimized.vy

@@ -982,7 +982,8 @@ def tweak_price(
         #         ensure new virtual_price is not less than old virtual_price,
         #                                        else the pool suffers a loss.
         if self.future_A_gamma_time < block.timestamp:
-            assert virtual_price > old_virtual_price, "Loss"
+            # this usually reverts when withdrawing a very small amount of LP tokens
+            assert virtual_price > old_virtual_price # dev: virtual price decreased
 
         # -------------------------- Cache last_xcp --------------------------
 

tests/unitary/pool/stateful/README.md

@@ -15,6 +15,12 @@ python -m pytest --hypothesis-show-statistics --hypothesis-verbosity=verbose -s
 
 `--hypothesis-show-statistics` while not being necessary for showing notes, can be helpful to have some statistics of what happens in the test.
 
+---
+If you see a test reverting but not stopping it's because it is in the shrinking phase! Sometime shrinking can take a lot of time without leading to significant results. If you want to skip the shrinking phase and get straight to the error you can do so by enabling the `no-shrink` profile defined in `strategies.py`. It sufficies to add this line to your test:
+```python
+settings.load_profile("no-shrink")
+```
+
 
 #### Before writing/updating stateful tests
 Read the docs multiple times through your stateful testing journey. Not only the stateful testing section but the whole hypothesis docs. Stateful testing might look like an isolated part of hypothesis, but the reality is that it is built on top of `SearchStrategies` and requires a very deep understanding of how hypothesis works. If you are wondering why one hypothesis functionality is useful, you're probably not capable of writing good tests (yet).

tests/unitary/pool/stateful/stateful_base.py

@@ -1,7 +1,8 @@
+from math import log, log10
 from typing import List
 
 import boa
-from hypothesis import event, note
+from hypothesis import assume, event, note
 from hypothesis.stateful import (
     RuleBasedStateMachine,
     initialize,
@@ -19,15 +20,19 @@
 
 
 class StatefulBase(RuleBasedStateMachine):
-    # try bigger amounts than 30 and e11 for low
     @initialize(
         pool=pool_strategy(),
-        # TODO deposits can be as low as 1e11, but small deposits breaks swaps
-        # I should do stateful testing only with deposit withdrawal
         amount=integers(min_value=int(1e20), max_value=int(1e30)),
         user=address,
     )
     def initialize_pool(self, pool, amount, user):
+        """Initialize the state machine with a pool and some
+        initial liquidity.
+
+        Prefer to use this method instead of the `__init__` method
+        when initializing the state machine.
+        """
+
         # cahing the pool generated by the strategy
         self.pool = pool
 
@@ -84,7 +89,8 @@ def get_balanced_deposit_amounts(self, amount: int):
 
     def add_liquidity(self, amounts: List[int], user: str):
         """Wrapper around the `add_liquidity` method of the pool.
-        Always prefer this instead of calling the pool method directly.
+        Always prefer this instead of calling the pool method directly
+        when constructing rules.
 
         Args:
             amounts (List[int]): amounts of tokens to be deposited
@@ -107,7 +113,6 @@ def add_liquidity(self, amounts: List[int], user: str):
         # store the amount of lp tokens before the deposit
         lp_tokens = self.pool.balanceOf(user)
 
-        # TODO stricter since no slippage
         self.pool.add_liquidity(amounts, 0, sender=user)
 
         # find the increase in lp tokens
@@ -127,7 +132,8 @@ def add_liquidity(self, amounts: List[int], user: str):
 
     def exchange(self, dx: int, i: int, user: str):
         """Wrapper around the `exchange` method of the pool.
-        Always prefer this instead of calling the pool method directly.
+        Always prefer this instead of calling the pool method directly
+        when constructing rules.
 
         Args:
             dx (int): amount in
@@ -138,33 +144,52 @@ def exchange(self, dx: int, i: int, user: str):
         j = 1 - i
 
         mint_for_testing(self.coins[i], user, dx)
-        self.coins[i].approve(self.pool.address, dx, sender=user)
+        self.coins[i].approve(self.pool, dx, sender=user)
 
+        # store the balances of the user before the swap
         delta_balance_i = self.coins[i].balanceOf(user)
         delta_balance_j = self.coins[j].balanceOf(user)
 
+        # this is a bit convoluted because we want this function
+        # to continue in two scenarios:
+        # 1. the function didn't revert (except block)
+        # 2. the function reverted because of an unsafe value
+        # of y (try block + boa.reverts)
         try:
-            expected_dy = self.pool.get_dy(i, j, dx)
-
-            actual_dy = self.pool.exchange(i, j, dx, expected_dy, sender=user)
-        except Exception:
-            # TODO test with different amounts if it fails,
-            # check that we can swap back the other way
+            with boa.reverts(dev="unsafe value for y"):
+                expected_dy = self.pool.get_dy(i, j, dx)
+            # if we end up here something went wrong
             event(
-                "exchange failed... Should report more details about imbalance"
+                "newton_y broke with {:.1f} equilibrium".format(
+                    self.equilibrium
+                )
             )
+            assert (
+                log10(self.equilibrium) > 19
+            ), "pool is not sufficiently imbalanced to justify a revert"
+            # this invariant should hold even in case of a revert
             self.can_always_withdraw(imbalanced_operations_allowed=True)
             return
+        except ValueError as e:
+            assert str(e) == "Did not revert"
+            # if we didn't revert we can continue
+
+        actual_dy = self.pool.exchange(i, j, dx, expected_dy, sender=user)
 
+        # compute the change in balances
         delta_balance_i = self.coins[i].balanceOf(user) - delta_balance_i
         delta_balance_j = self.coins[j].balanceOf(user) - delta_balance_j
 
-        assert -delta_balance_i == dx
-        assert delta_balance_j == expected_dy == actual_dy
+        assert -delta_balance_i == dx, "didn't swap right amount of token x"
+        assert (
+            delta_balance_j == expected_dy == actual_dy
+        ), "didn't receive the right amount of token y"
 
+        # update the internal balances of the test for the invariants
         self.balances[i] -= delta_balance_i
         self.balances[j] -= delta_balance_j
 
+        # update the profit made by the pool
         self.xcp_profit = self.pool.xcp_profit()
 
         note(
@@ -173,14 +198,30 @@ def exchange(self, dx: int, i: int, user: str):
             )
         )
 
-    def remove_liquidity(self, amount, user):
+    def remove_liquidity(self, amount: int, user: str):
+        """Wrapper around the `remove_liquidity` method of the pool.
+        Always prefer this instead of calling the pool method directly
+        when constructing rules.
+
+        Args:
+            amount (int): the amount of lp tokens to withdraw
+            user (str): the address of the withdrawer
+        """
+        # store the balances of the user before the withdrawal
         amounts = [c.balanceOf(user) for c in self.coins]
-        # TODO use reported balances
+
+        # withdraw the liquidity
         self.pool.remove_liquidity(amount, [0] * 2, sender=user)
+
+        # compute the change in balances
         amounts = [
             (c.balanceOf(user) - a) for c, a in zip(self.coins, amounts)
         ]
+
+        # total apply should have decreased by the amount of liquidity
+        # withdrawn
         self.total_supply -= amount
+        # update the internal balances of the test for the invariants
         self.balances = [b - a for a, b in zip(amounts, self.balances)]
 
         # we don't want to keep track of users with low liquidity because
@@ -194,65 +235,108 @@ def remove_liquidity(self, amount, user):
             self.virtual_price = 1e18
 
     def remove_liquidity_one_coin(
-        self, percentage: float, coin_idx: int, user
+        self, percentage: float, coin_idx: int, user: str
     ):
-        print(
-            "balance of fee receiver {:.2e}".format(
-                self.pool.balanceOf(self.fee_receiver)
-            )
-        )
-        # upkeeping prepartion logic
+        """Wrapper around the `remove_liquidity_one_coin` method of the pool.
+        Always prefer this instead of calling the pool method directly
+        when constructing rules.
+
+        Args:
+            percentage (float): percentage of liquidity to withdraw
+            from the user balance
+            coin_idx (int): index of the coin to withdraw
+            user (str): address of the withdrawer
+        """
+        # when the fee receiver is the lp owner we can't compute the
+        # balances in the invariants correctly. (This should never
+        # be the case in production anyway).
+        assume(user != self.fee_receiver)
+
+        # store balances of the fee receiver before the removal
         admin_balances_pre = [
             c.balanceOf(self.fee_receiver) for c in self.coins
         ]
-        lp_balances_pre = self.coins[coin_idx].balanceOf(user)
+        # store the balance of the user before the removal
+        user_balances_pre = self.coins[coin_idx].balanceOf(user)
+
         pool_is_ramping = (
             self.pool.future_A_gamma_time() > boa.env.evm.patch.timestamp
         )
-        # end of upkeeping prepartion logic
 
+        # lp tokens before the removal
         lp_tokens_balance_pre = self.pool.balanceOf(user)
+
         if percentage == 1.0:
             # this corrects floating point errors that can lead to
             # withdrawing more than the user has
             lp_tokens_to_withdraw = lp_tokens_balance_pre
         else:
             lp_tokens_to_withdraw = int(lp_tokens_balance_pre * percentage)
-        # reported_withdrawn_token_amount =
-        self.pool.remove_liquidity_one_coin(
-            lp_tokens_to_withdraw,
-            coin_idx,
-            0,  # no slippage checks
-            sender=user,
+
+        # this is a bit convoluted because we want this function
+        # to continue in two scenarios:
+        # 1. the function didn't revert (except block)
+        # 2. the function reverted because the virtual price
+        # decreased (try block + boa.reverts)
+        try:
+            with boa.reverts(dev="virtual price decreased"):
+                self.pool.remove_liquidity_one_coin(
+                    lp_tokens_to_withdraw,
+                    coin_idx,
+                    0,  # no slippage checks
+                    sender=user,
+                )
+            # if we end up here something went wrong (sometimes it's ok)
+
+            # we only allow small amounts to make the balance decrease
+            # because of rounding errors
+            assert (
+                lp_tokens_to_withdraw < 1e15
+            ), "virtual price decreased but but the amount was too high"
+            event(
+                "unsuccessfull removal of liquidity because of "
+                "loss (this should not happen too often)"
+            )
+            return
+        except ValueError as e:
+            assert str(e) == "Did not revert"
+            # if the function didn't revert we can continue
+            if lp_tokens_to_withdraw < 1e15:
+                # useful to compare how often this happens compared to failures
+                event("successful removal of liquidity with low amounts")
+
+        # compute the change in balances
+        user_balances_post = abs(
+            user_balances_pre - self.coins[coin_idx].balanceOf(user)
         )
 
-        actual_withdrawn_token_amount = lp_balances_pre - self.coins[
-            coin_idx
-        ].balanceOf(user)
-        self.balances[coin_idx] -= actual_withdrawn_token_amount
+        # update internal balances
+        self.balances[coin_idx] -= user_balances_post
+        # total supply should decrease by the amount of tokens withdrawn
         self.total_supply -= lp_tokens_to_withdraw
 
-        logs = self.pool.get_logs()
-        for log in logs:
-            print(log)
-
         # we don't want to keep track of users with low liquidity because
         # it would approximate to 0 tokens and break the test.
         if self.pool.balanceOf(user) <= 1e0:
             self.depositors.remove(user)
 
-        # upkeeping logic
+        # invarinant upkeeping logic:
+        # imbalanced removals can trigger a claim of admin fees
 
+        # store the balances of the fee receiver after the removal
         new_xcp_profit_a = self.pool.xcp_profit_a()
+        # store the balances of the fee receiver before the removal
         old_xcp_profit_a = self.xcp_profit_a
 
-        # check if the admin fees were claimed
+        # check if the admin fees were claimed (not always the case)
         if new_xcp_profit_a > old_xcp_profit_a:
             event("admin fees claim was detected")
             note("claiming admin fees during removal")
-            # if the admin fees were claimed we have to update the balances
+            # if the admin fees were claimed we have to update xcp
             self.xcp_profit_a = new_xcp_profit_a
 
+            # store the balances of the fee receiver after the removal
+            # (should be higher than before the removal)
             admin_balances_post = [
                 c.balanceOf(self.fee_receiver) for c in self.coins
             ]
@@ -264,28 +348,52 @@ def remove_liquidity_one_coin(
                         claimed_amount, i
                     )
                 )
-                assert claimed_amount > 0  # check if non zero amounts of claim
+                assert (
+                    claimed_amount > 0
+                ), f"the admin fees collected should be positive for coin {i}"
                 # TODO this can probably be refactored to a helper function
                 # (useful for future tests)
-                assert not pool_is_ramping  # cannot claim while ramping
+                assert not pool_is_ramping, "claim admin fees while ramping"
 
-                # update self.balances
+                # deduce the claimed amount from the pool balances
                 self.balances[i] -= claimed_amount
 
+        # update test-tracked xcp profit
         self.xcp_profit = self.pool.xcp_profit()
 
     def report_equilibrium(self):
+        """Helper function to report the current equilibrium of the pool.
+        This is useful to see how the pool is doing in terms of
+        imbalances.
+
+        This is useful to see if a revert because of "unsafe values" in
+        the math contract could be justified by the pool being too imbalanced.
+
+        We compute the equilibrium as (x * price_x + y * price_y) / D
+        which is like approximating that the pool behaves as a constant
+        sum AMM.
+        """
+        # we calculate the equilibrium of the pool
         old_equilibrium = self.equilibrium
+
         self.equilibrium = (
-            self.coins[0].balanceOf(self.pool)
+            self.coins[0].balanceOf(
+                self.pool
+            )  # price_x is always 1 by construction.
             + self.coins[1].balanceOf(self.pool) * self.pool.price_scale()
         ) / self.pool.D()
 
+        # we compute the percentage change from the old equilibrium
+        # to have a sense of how much an operation changed the pool
         percentage_change = (
             (self.equilibrium - old_equilibrium) / old_equilibrium * 100
         )
+
+        # we report equilibrium as log to make it easier to read
         note(
-            "pool balance (center is at 5e17) {:.2e} ".format(self.equilibrium)
+            "pool balance (center is at 40.75) {:.2f} ".format(
+                log(self.equilibrium)
+            )
             + "percentage change from old equilibrium: {:.4%}".format(
                 percentage_change
             )
@@ -368,12 +476,6 @@ def balances(self):
         balances = [self.pool.balances(i) for i in range(2)]
         balance_of = [c.balanceOf(self.pool) for c in self.coins]
         for i in range(2):
-            print(
-                "{} \n{} \n{}".format(
-                    self.balances[i], balances[i], balance_of[i]
-                )
-            )
-            print("discrepancy {:.2e}".format(self.balances[i] - balances[i]))
             assert self.balances[i] == balances[i]
             assert self.balances[i] == balance_of[i]
 
@@ -422,5 +524,3 @@ def up_only_profit(self):
 
 
 TestBase = StatefulBase.TestCase
-
-# TODO make sure that xcp goes down when claiming admin fees