test: btt tests for swap exact amount in #149

0xteddybear · 2024-07-09T20:23:53Z

the user not having enough tokenIn would revert on _pullUnderlying, perhaps it's better test modularization to not test that flow here, but with _pullUnderlying? tree has less branches this way, but it's less descriptive of domain cases and isn't consistent with what I just did on exitPool

do you think it's worthwhile to test the cases where

tokenIn.balanceOf(pool) == 0?
tokenOut.balanceOf(pool) == 0? combined with tokenAmountIn == 0 it could cause a division by zero in calcOutGivenIn

closes BAL-142

linear · 2024-07-09T20:37:57Z

BAL-142

0xteddybear · 2024-07-09T21:50:05Z

test/unit/BPool/BPool_SwapExactAmountIn.tree

+│   └── it should revert
+├── when returned tokenOut is less than minAmountOut
+│   └── it should revert
+└── when preconditions are met


ooops, realized I'm not triggering BPool_SpotPriceBeforeAboveTokenRatio. Looking into it

the reason behind that check and the MAX_{IN,OUT}_RATIO ones sure looks like could be fun properties 👀

i think the rationale is avoiding fat fingers, meaning buying more than 50% of the available tokens, or selling more than 50% of the pool balance

I tried to find parameters to trigger this particular error and wasn't able to: https://www.desmos.com/calculator/onhfxbfljk

@drgorillamd you think it would be possible to formally prove this is unreachable?

I did notice that there is one testcase for swapExactAmountOut, will look into it when testing that method

to be clear, we're not planning on removing the check even if we find it unreachable, but we can then skip the test, or add a comment in the test explaining the current behaviour

I tried to find parameters to trigger this particular error and wasn't able to: https://www.desmos.com/calculator/onhfxbfljk

@drgorillamd you think it would be possible to formally prove this is unreachable?

I did notice that there is one testcase for swapExactAmountOut, will look into it when testing that method

Quickly tried with Halmos but there are some loops hiding in bnum, so I'll try with Kontrol instead (Echidna seemds to find some cases tho, for small token amount in esp, will double-check tomorrow along Kontrol tho)

wei3erHase · 2024-07-09T23:35:03Z

test/unit/BPool/BPool_SwapExactAmountIn.tree

+├── when spot price before swap exceeds maxPrice
+│   └── it should revert
+├── when spot price after swap exceeds maxPrice
+│   └── it should revert


isn't there an intermediary step? if spot price before exceeds spot price after? 🤔

i do think this error is unreachable, but perhaps with smock we can have a chance of getting to it

i think we should make the spot price check after the returned tokenOut (to keep consistency with order of reverts)

wei3erHase · 2024-07-09T23:43:52Z

mhhh for the zero variable hence division by zero, if they happen in the tested method i'd include them, if they happen in an underlying one i'd leave them for the specific test

wei3erHase · 2024-07-10T09:40:24Z

test/unit/BPool/BPool_SwapExactAmountIn.tree

+│   └── it should revert
+├── when token out is not bound
+│   └── it should revert
+├── when token in exceeds max allowed ratio


*token amount in

wei3erHase · 2024-07-10T09:40:58Z

test/unit/BPool/BPool_SwapExactAmountIn.tree

+│   └── it should revert
+├── when spot price after swap exceeds maxPrice
+│   └── it should revert
+├── when returned tokenOut is less than minAmountOut


when calculated token amount out is less than min

wei3erHase · 2024-07-10T22:16:37Z

test/unit/BPool/BPool_SwapExactAmountIn.tree

+├── when spot price after swap exceeds maxPrice
+│   └── it should revert
+├── when spot price before swap exceeds token ratio after swap
+│   └── it should revert


missing BPool_SpotPriceAfterBelowSpotPriceBefore revert (also unreachable but perhaps mockable)

mathematically it seems unreachable, and in order to mock it I should:

make calcSpotPrice virtual

manually override it in the mocking for BPool to force an external call

insert a mock for the call with the second set of parameters to return a value lower than for the first one. Thankfully the parameters are different in each invocation, so a regular mockCall can take care of it and we wouldn't have to make a custom override counting calls.

Adding a skipped test to be explicit about it

wei3erHase

looking good, let's see how it goes after merging #148

0xteddybear · 2024-07-11T13:59:53Z

test/unit/BPool/BPool_SwapExactAmountIn.t.sol

+    // overriding `calcSpotPrice` in a mock:
+    // P_{sb} = \frac{\frac{b_i}{w_i}}{\frac{b_o}{w_o}}
+    // P_{sa} = \frac{\frac{b_i + a_i}{w_i}}{\frac{b_o - a_o}{w_o}}
+    // ...and both a_i (amount in) and a_o (amount out) are uints


@drgorillamd another proving candidate?

0xteddybear · 2024-07-11T14:01:40Z

test/unit/BPool.t.sol

@@ -818,230 +818,6 @@ contract BPool_Unit_GetSpotPriceSansFee is BasePoolTest {
  }
 }

-contract BPool_Unit_SwapExactAmountIn is SwapExactAmountInUtils {


btw I plan on taking care of the BCoWPool.verify tests, which rely on SwapExactAmountInUtils next, so we can get rid of the abstract contract as well

actually i saw today that many of the Utils.sol methods are unused, so we should make a cleaning after getting the final coverage

making a list in BAL-155

test/unit/BPool/BPool_SwapExactAmountIn.t.sol

wei3erHase · 2024-07-11T16:29:29Z

test/unit/BPool/BPool_SwapExactAmountIn.t.sol

+  function test_RevertWhen_SpotPriceBeforeSwapExceedsTokenRatioAfterSwap() external {
    // it should revert
-    vm.expectRevert(IBPool.BPool_TokenAmountOutBelowMinOut.selector);
-    bPool.swapExactAmountIn(tokenIn, tokenAmountIn, tokenOut, expectedAmountOut + 1, spotPriceAfterSwap);
+    vm.skip(true);
  }


this one appears to be reachable in current tests

wei3erHase · 2024-07-11T16:32:12Z

test/unit/BPool.t.sol

@@ -818,195 +818,6 @@ contract BPool_Unit_GetSpotPriceSansFee is BasePoolTest {
  }
 }

-contract BPool_Unit_ExitPool is BasePoolTest {


i think this is not the test you should be erasing in this PR

~~ 😨 ~~ hold on, I'm not seeing this in the diff 👀

ahh perhaps was on the "merging dev" commit

wei3erHase · 2024-07-11T16:40:57Z

test/unit/BPool/BPool_SwapExactAmountIn.t.sol

+    // it should revert
+    vm.skip(true);


have a set of values that triggers this :)

Suggested change

// it should revert

vm.skip(true);

uint256 tokenAmountIn_ = 30e18;

uint256 balanceTokenIn_ = 36830000000000000000000000000000;

uint256 weightTokenIn_ = 1e18;

uint256 balanceTokenOut_ = 18100000000000000000000000000000;

uint256 weightTokenOut_ = 1e18;

uint256 swapFee_ = 0.019e18;

vm.mockCall(tokenIn, abi.encodePacked(IERC20.balanceOf.selector), abi.encode(uint256(balanceTokenIn_)));

vm.mockCall(tokenOut, abi.encodePacked(IERC20.balanceOf.selector), abi.encode(uint256(balanceTokenOut_)));

bPool.set__records(tokenIn, IBPool.Record({bound: true, index: 0, denorm: weightTokenIn_}));

bPool.set__records(tokenOut, IBPool.Record({bound: true, index: 1, denorm: weightTokenOut_}));

bPool.set__swapFee(swapFee_);

// it should revert

vm.expectRevert(IBPool.BPool_SpotPriceBeforeAboveTokenRatio.selector);

bPool.swapExactAmountIn(tokenIn, tokenAmountIn_, tokenOut, 0, type(uint256).max);

how did you generate this? a fuzz run?

i used the fuzzer of legacy swapExactAmountIn tests to generate a counterproof, to hit the specific revert a combination of testFail and vm.expectRevert can be used

wei3erHase

rocking it pal! 👏

0xteddybear added 2 commits July 9, 2024 17:21

test: btt tests for bpool.swapExactAmountIn

5321bbf

chore: delete preexisting unit tests

432f63a

0xteddybear marked this pull request as ready for review July 9, 2024 20:37

0xteddybear requested a review from wei3erHase July 9, 2024 20:39

0xteddybear commented Jul 9, 2024

View reviewed changes

wei3erHase reviewed Jul 9, 2024

View reviewed changes

wei3erHase reviewed Jul 10, 2024

View reviewed changes

0xteddybear added 2 commits July 10, 2024 16:18

test: small renames from feedback

e66ed9f

test: be explicit about untestable code

1acff62

wei3erHase reviewed Jul 10, 2024

View reviewed changes

0xteddybear added 2 commits July 11, 2024 10:28

chore: merge dev

86f1950

test: adding skipped test for unreachable condition

d111e8b

0xteddybear commented Jul 11, 2024

View reviewed changes

wei3erHase reviewed Jul 11, 2024

View reviewed changes

test/unit/BPool/BPool_SwapExactAmountIn.t.sol Show resolved Hide resolved

wei3erHase reviewed Jul 11, 2024

View reviewed changes

0xteddybear mentioned this pull request Jul 11, 2024

test: btt bpool swap exact amount out #153

Merged

test: code wasnt so unreachable after all

9f074dc

0xteddybear mentioned this pull request Jul 11, 2024

feat: adding bcowpool verify btt tests #155

Merged

0xteddybear requested a review from wei3erHase July 11, 2024 23:03

wei3erHase approved these changes Jul 12, 2024

View reviewed changes

wei3erHase merged commit 06efb54 into dev Jul 12, 2024
4 checks passed

wei3erHase deleted the test/btt-bpool-swapExactAmountIn branch July 12, 2024 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: btt tests for swap exact amount in #149

test: btt tests for swap exact amount in #149

0xteddybear commented Jul 9, 2024 •

edited

Loading

linear bot commented Jul 9, 2024

0xteddybear Jul 9, 2024

0xteddybear Jul 9, 2024

wei3erHase Jul 9, 2024 •

edited

Loading

0xteddybear Jul 10, 2024

wei3erHase Jul 10, 2024

simon-something Jul 10, 2024

wei3erHase Jul 9, 2024

wei3erHase Jul 10, 2024

wei3erHase commented Jul 9, 2024 •

edited

Loading

wei3erHase Jul 10, 2024

wei3erHase Jul 10, 2024 •

edited

Loading

wei3erHase Jul 10, 2024

0xteddybear Jul 11, 2024

wei3erHase left a comment

0xteddybear Jul 11, 2024

0xteddybear Jul 11, 2024

wei3erHase Jul 11, 2024

0xteddybear Jul 11, 2024

wei3erHase Jul 11, 2024

wei3erHase Jul 11, 2024

0xteddybear Jul 11, 2024 •

edited

Loading

wei3erHase Jul 12, 2024

wei3erHase Jul 11, 2024 •

edited

Loading

0xteddybear Jul 11, 2024

wei3erHase Jul 12, 2024

wei3erHase left a comment

-    // it should revert
-    vm.skip(true);
+    uint256 tokenAmountIn_ = 30e18;
+    uint256 balanceTokenIn_ = 36830000000000000000000000000000;
+    uint256 weightTokenIn_ = 1e18;
+    uint256 balanceTokenOut_ = 18100000000000000000000000000000;
+    uint256 weightTokenOut_ = 1e18;
+    uint256 swapFee_ = 0.019e18;
+    vm.mockCall(tokenIn, abi.encodePacked(IERC20.balanceOf.selector), abi.encode(uint256(balanceTokenIn_)));
+    vm.mockCall(tokenOut, abi.encodePacked(IERC20.balanceOf.selector), abi.encode(uint256(balanceTokenOut_)));
+    bPool.set__records(tokenIn, IBPool.Record({bound: true, index: 0, denorm: weightTokenIn_}));
+    bPool.set__records(tokenOut, IBPool.Record({bound: true, index: 1, denorm: weightTokenOut_}));
+    bPool.set__swapFee(swapFee_);
+    // it should revert
+    vm.expectRevert(IBPool.BPool_SpotPriceBeforeAboveTokenRatio.selector);
+    bPool.swapExactAmountIn(tokenIn, tokenAmountIn_, tokenOut, 0, type(uint256).max);

test: btt tests for swap exact amount in #149

test: btt tests for swap exact amount in #149

Conversation

0xteddybear commented Jul 9, 2024 • edited Loading

linear bot commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wei3erHase Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wei3erHase commented Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

wei3erHase Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wei3erHase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xteddybear Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wei3erHase Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wei3erHase left a comment

Choose a reason for hiding this comment

0xteddybear commented Jul 9, 2024 •

edited

Loading

wei3erHase Jul 9, 2024 •

edited

Loading

wei3erHase commented Jul 9, 2024 •

edited

Loading

wei3erHase Jul 10, 2024 •

edited

Loading

0xteddybear Jul 11, 2024 •

edited

Loading

wei3erHase Jul 11, 2024 •

edited

Loading