feat: adding CoW helper MVP #121

wei3erHase · 2024-06-27T16:05:31Z

TODO: Vendor GetTradeableOrder from cow-amm after this PR gets merged: chore: running forge update and fixing OZ related methods cowprotocol/cow-amm#98
Helper MUST be reviewed by CoWSwap SC team

BCoWHelper status:

The BCoWHelper contract is designed to be view-called by solvers in order to fetch the relevant information that is needed to swap in a Pool, given a pricing vectore. The contract implements a CoW library, that supports only 2-token unweighted pools.

Helper inherits ICOWAMMPoolHelper interface from cow-amm
Helper implements GetTradeableOrder from cow-amm
tokens method WILL revert with PoolDoesNotExist if:
- Pool is not a children of the helper’s factory
- Pool is not finalized (this one reverts with BPool_PoolNotFinalized)
- Pool has more than 2 tokens
- Pool tokens have different weights
order method internally calls tokens (same reverting cases)
order returns the commit pre-interaction and the order calculated in GetTradeableOrder library
factory aims to the BCoWFactory and reads the AppData from it

Considerations in limited compatibility:

The unweighted nature of GetTradeableOrder is definite, the calculation doesn’t fall true for pools with different token weights, a new calculation needs to be done in order to support them (perhaps GetTradeableWeightedOrder?).
The 2-token nature, on the other hand, is a workable limitation. The current design of the Helper just reverts if the Pool has more than 2 tokens. When we consider a 3-token pool (unweighted), we can imagine the interaction being: a 3-dimensional pricing vector is sent to the Helper, and the Helper will decide which of these 2 tokens should be traded (and build the GPv2Order + commit pre-interaction). This, at first sight is very difficult to implement, considering:
- How do we create a 3-dimensional pricing vector?
- How does the Helper know which tokens are best to trade?
- How do we input a 3-dimensional array into the library?
There is a workaround to this approach, and is making the helper accept 3-dimensional pricing vectors, but ONLY if 2 dimensions are non-zero. In that case, a X/Y/Z pool can be queried for [X,Y,0], [X,0,Z], and [0,Y,Z] separately, and reuse the 2-dimensional library that CoW provides.
Pool level, there is no 2-token restriction for BCoWPools, Pools with up to 8 tokens can be created, and the BCoW flow supports swaps from any token to any other token in the Pool. When swapping in the Pool, the only relevant tokens are the token IN and OUT, no other token bounded to the pool has any effect on the swap, or in other way: during swaps the Pool behaves always as 2-token. So surplus measurements shouldn’t be affected by supporting multiple-token pools.

Summary:

Helper is currently NOT supporting +2-token pools.
Helper is currently NOT supporting weighted pools.
Helper COULD support +2-token pools (iff the pricing vector has 2 non-zero values)
Helper COULD NOT support weighted pools (requires new calcs).
BCoWPool DOES support +2-token pools.
BCoWPool DOES support weighted pools.

Comments:

Adding a limitation to the BCoWFactory to allow only deployments of 2-token unweighted pools breaks the future compatibility of a newly developed Helper. In the event that the new calcs (to support weighted pools) are written, and some +2-token interaction is designed, the Factory would have to be redeployed in order to remove the check. And since supporting deployments of such Pools don’t cause any security issue, we recommend that the BCoWFactory is deployed as is (supporting +2-token weighted pools), that we keep the incompatibility checks at the Helper level, so that a new Helper can be added later without having to modify the Factory.

into feat/helper-poc

fedgiac

Everything in the PR description is reasonable and the code is a good implementation for the helper (with one caveat, more on this later). The considerations/limitations are as expected. More in detail:

we recommend that the BCoWFactory is deployed as is (supporting +2-token weighted pools), that we keep the incompatibility checks at the Helper level

The helper is indeed disposable and it's good to just deploy it as-is now, then replace it once we build up support for more tokens/weights (which is tricky for the reasons you mentioned).

About the caveat: the sell/buy price is computed with GetTradeableOrder.getTradeableOrder. This library works for a constant-product AMM like it's the case for the Balancer AMM as supported here. Still, the generated trade is (up to rounding) tight, in the sense that decreasing minimally the buy amount should cause the order to be invalid (manifested as an invalid signature, reverting with BPool_TokenAmountOutBelowMinOut). My concern is that, because of rounding, the order returned by GetTradeableOrder.getTradeableOrder might be slightly below the threshold for calcOutGivenIn and cause the helper to return a trade that isn't valid. The forked test right now works, but there could be variations of the input values (balances, weights (even if the same)) that may cause it to fail.
Concretely, this means that the CoW Protocol back-end would generate an invalid order and the AMM might be ignored for the batches where rounding causes the creation of a reverting trade.
A fix for this would be to replicate the GetTradeableOrder logic for the sell amount but use calcOutGivenIn for the sell amount. This should return the same result (up to rounding) but be unaffected by the issue above.

All other review comments are minor.

test/integration/BCoWHelper.t.sol

fedgiac

Under the assumption that #154 is merged, the code looks good for being used by the CoW Protocol back end.

* fix: typos in comments * fix: overwriting sellAmount in order to avoid rounding issues * feat: improving the test case to cover both cases

0xteddybear

looking good, I'd move more checks onto the unit tests and clean up the integrations so the valid scenario they are executing is easier to follow

package.json

0xteddybear · 2024-07-15T20:06:01Z

src/contracts/BCoWHelper.sol

+  using GPv2Order for GPv2Order.Data;
+
+  /// @notice The app data used by this helper's factory.
+  bytes32 public immutable APP_DATA;


this is not in any interface, IIRC we intended to have every public method available in an interface as part of our internal style guide (although it's disabled in the natspec smells config, since we didn't want to deal with some methods on BMath and BNum)

hmmm we could make it internal tbh, this contract is supposed to be called only by the interface related methods, as it's al off-chain helper for solvers

src/contracts/BCoWHelper.sol

0xteddybear · 2024-07-15T22:43:15Z

test/integration/BCoWHelper.t.sol

+  }
+
+  // NOTE: reverting test, weighted pools are not supported
+  function testWeightedOrder() public {


I believe a unit test is enough for this

this one was chosen to maintain the shape in case the feature gets added later, the unit test of the helper reverting is there, and the integration perspective we keep it here, in the future, another helper will not revert on weighted pools, and this test can be reimplemented not to revert

... if this is meant to stay then wdyt about removing the commented code below?

test/integration/BCoWHelper.t.sol

…unit test

wei3erHase · 2024-07-16T20:06:04Z

test/unit/BCoWHelper.tree

+│   ├── it should return a commit pre-interaction
+│   ├── it should return an empty post-interaction
+│   └── it should return a valid signature
+└── given a price vector


cc-ing this to @drgorillamd, i'm breaking standard with given to not-branch but just make a test with a different focus, in this one in particular is fuzzing the balances and price vectors, it could have been done in the same scope as when the pool is supported, but think that separation of affairs is good there, since the code is looking at other things, wdyt?

given ftw :D cc @gas1cent ?

0xteddybear · 2024-07-17T00:11:53Z

test/integration/BCoWHelper.t.sol

+  }
+
+  // NOTE: reverting test, weighted pools are not supported
+  function testWeightedOrder() public {


... if this is meant to stay then wdyt about removing the commented code below?

0xteddybear · 2024-07-17T00:15:58Z

test/integration/BCoWHelper.t.sol

+    _executeHelperOrder(pool);
+
+    uint256 postSpotPrice = pool.getSpotPriceSansFee(address(WETH), address(DAI));
+    assertEq(postSpotPrice, 1_052_631_578_947_368);


I'd do a ranged assert with a computed amount, similar to how you did with order.{buy,sell}Amount below

0xteddybear · 2024-07-17T00:20:57Z

test/manual-smock/MockBCoWHelper.sol

+
+contract MockBCoWHelper is BCoWHelper, Test {
+  // NOTE: manually added methods (internal immutable exposers not supported in smock)
+  function call__APP_DATA() external view returns (bytes32) {


why not make _APP_DATA public?

is not required in the interface that CoWSwap provides ICOWAMMPoolHelper, they only request to expose factory, tokens and order

test/unit/BCoWHelper.t.sol

0xteddybear · 2024-07-17T00:29:20Z

test/unit/BCoWHelper.t.sol

+    assertEq(keccak256(validSig), keccak256(sig));
+  }
+
+  function test_OrderGivenAPriceVector(uint256 priceSkewness, uint256 balanceToken0, uint256 balanceToken1) external {


I don't really follow what are the 5000, 10_000and15_000`, much less why we need them. Do you think it would be clearer if they had names? or some comments?

afaict we will skew the price by a particular factor from current spot price in order to allow for naturally ocurring slippage, is that correct?

making it uint256 base = 1e18 and the bounds between 0.5 and 1.5

0xteddybear · 2024-07-17T00:32:35Z

test/unit/BCoWHelper.t.sol

+    ) = helper.order(address(pool), priceVector);
+
+    // it should return a valid pool order
+    assertEq(order_.receiver, GPv2Order.RECEIVER_SAME_AS_OWNER);


I think it's worth it to precompute some values to make sure we're not passing parameters to the library in the inverse order (or other silly-but-consequential bugs like that). This won't block merging since we're already doing it for the integrations, but is still desireable. (related to https://github.com/defi-wonderland/balancer-v1-amm/pull/121/files#r1678466826)

0xteddybear · 2024-07-17T00:35:37Z

test/unit/BCoWHelper.t.sol

+    // it should return a valid pool order
+    (GPv2Order.Data memory ammOrder,,,) = helper.order(address(pool), prices);
+
+    assertEq(address(ammOrder.buyToken), priceSkewness > 10_000 ? tokens[0] : tokens[1]);


are we sure these are not different branches?

wdym by "not different branches"?
i think is more of a different approach but same branch, test_OrderWhenThePoolIsSupported doesn't check the amounts and just use the default price vector (1:1.05), while test_OrderGivenAPriceVector fuzzes both balances and the skeweness but checks only that the pool can verify it, and that the token being sold is the correct one.

we could entirely fuzz test_OrderWhenThePoolIsSupported, but we'd be adding complexity to that test instead of maintaining the "valid scenario" approach, dunno wdyt of that

I believe that to fully benefit from btt tests we should make the .tree fully match the branches available in the (code|domain). I've checked and they're effectively different branches: node_modules/cow-amm/src/libraries/GetTradeableOrder.sol:56

It was not obvious for me reviewing it, and it could have been easier to see had the information of when token0 reserve * denominator > when token1 reserve * numerator -> buyToken is token1 been in the .tree file. I believe this is a case where you set some 'funny values' to get to that branch. I consider this PR should include:

fully fuzzed tests to report on rounding errors (this one is ok for these purposes, we could also be purists and have one fully-fuzzed for each branch. Definetly not required.) This doesnt fit neatly into the .tree but I can live with that.

'valid scenario' 'funny value'd unit tests that are described in .tree and walk every available branch. In this case I'd consider the library an implementaiton detail and walk their branches as well, as that's critical to ensure this helper works as intended. This includes:

one testcase forcing token0 to be buyToken

one testcase forcing token1 to be buyToken

Some assertions on src/contracts/BCoWHelper.sol:74, since that defines two branches as well. Since they are coupled to the above, I don't know if they'll be their own separate two testcases or extra asserts inside them, do whatever feels right.

tl;dr: fuzzing is ok as is, we should have tests manually walking each branch.

0xteddybear

good stuff!
having btt tests walk all branches is highly desireable, the rest are nitpicks.

test/unit/BCoWHelper.tree

0xteddybear · 2024-07-18T16:07:32Z

test/unit/BCoWHelper.t.sol


    // it should return a valid pool order
    (GPv2Order.Data memory ammOrder,,,) = helper.order(address(pool), prices);

-    assertEq(address(ammOrder.buyToken), priceSkewness > 10_000 ? tokens[0] : tokens[1]);
+    assertEq(address(ammOrder.buyToken), priceSkewness > 1e18 ? tokens[0] : tokens[1]);


use the base variable

0xteddybear · 2024-07-18T16:13:05Z

src/contracts/BCoWHelper.sol

+      callData: abi.encodeWithSelector(IBCoWPool.commit.selector, orderCommitment)
+    });
+
+    return (order_, preInteractions, postInteractions, sig);


from what you laid out here: #156 (comment) I suppose we should skip the explicit return

@wei3erHase 👀

i think is worth calling it "non-obvious return", specially since postInteractions will be empty and we cannot not declare it (compiler warnings)

0xteddybear · 2024-07-18T16:47:45Z

test/unit/BCoWHelper.t.sol

+    // it should return a valid pool order
+    (GPv2Order.Data memory ammOrder,,,) = helper.order(address(pool), prices);
+
+    assertEq(address(ammOrder.buyToken), priceSkewness > 10_000 ? tokens[0] : tokens[1]);


I believe that to fully benefit from btt tests we should make the .tree fully match the branches available in the (code|domain). I've checked and they're effectively different branches: node_modules/cow-amm/src/libraries/GetTradeableOrder.sol:56

It was not obvious for me reviewing it, and it could have been easier to see had the information of when token0 reserve * denominator > when token1 reserve * numerator -> buyToken is token1 been in the .tree file. I believe this is a case where you set some 'funny values' to get to that branch. I consider this PR should include:

fully fuzzed tests to report on rounding errors (this one is ok for these purposes, we could also be purists and have one fully-fuzzed for each branch. Definetly not required.) This doesnt fit neatly into the .tree but I can live with that.

'valid scenario' 'funny value'd unit tests that are described in .tree and walk every available branch. In this case I'd consider the library an implementaiton detail and walk their branches as well, as that's critical to ensure this helper works as intended. This includes:

one testcase forcing token0 to be buyToken

one testcase forcing token1 to be buyToken

Some assertions on src/contracts/BCoWHelper.sol:74, since that defines two branches as well. Since they are coupled to the above, I don't know if they'll be their own separate two testcases or extra asserts inside them, do whatever feels right.

tl;dr: fuzzing is ok as is, we should have tests manually walking each branch.

0xteddybear

having fuzzing cover separate code branches in different tests is a good compromise, however as an improvement I would have preferred:

for the unit tests to include when- branches with 'funny values' that cause the different branches' execution with no fuzzing setup.
the above to include asserts on {buy,sell}Amount (not a hard requirement since they are covered in the integrations)
fuzzing separate from that, in given branches, possibly with less assertions

0xteddybear · 2024-07-22T20:23:24Z

test/unit/BCoWHelper.t.sol


-    balanceToken0 = bound(balanceToken0, 1e18, 1e36);
-    balanceToken1 = bound(balanceToken1, 1e18, 1e36);
+    balanceToken0 = bound(balanceToken0, 1e18, 1e27);


why this change?

avoiding overflows

0xteddybear · 2024-07-22T20:39:52Z

src/contracts/BCoWHelper.sol

+      callData: abi.encodeWithSelector(IBCoWPool.commit.selector, orderCommitment)
+    });
+
+    return (order_, preInteractions, postInteractions, sig);


@wei3erHase 👀

0xteddybear · 2024-07-22T20:51:11Z

test/unit/BCoWHelper.t.sol

+    pool.set__totalWeight(2 * VALID_WEIGHT);
+    pool.set__finalized(true);
+
+    priceVector[0] = 1e18;


I meant something like 3000:1 so it's more evident, like you did with the integration tests

wei3erHase added 28 commits June 27, 2024 15:33

feat: non-weighted tradeable order test

699d9d7

fix: adding GetTradeableOrder library

271e00a

feat: adding weights to the math

48888e3

feat: variable weights

28565f7

feat: adding test for helper

1a44f5e

feat: adding support for weights

8bf22dd

fix: fmt

3683943

fix: sending correct price vector

9422865

fix: making helper return valid signature

be1701d

chore: merge dev

24553ab

feat: deprecated weights in helper

441d117

feat: updated buyAmount as per aaf44a3

5630797

refactor: mv GetTradeableOrder to libraries dir

627d878

fix: update comment on BCoWHelper

cb2fb52

fix: rm unused variable warning

5207c18

feat: adding BTT test for BCoWHelper

6f76f85

fix: linter warnings

de99df4

fix: rm remanent line change

bd64dca

fix: rm unused lines

ac17f69

fix: missing natspec

6a14084

fix: uncommenting assertion in test

96f0302

fix: missing natspec

e8c4d33

fix: safety checks comments

22480c5

feat: vendoring interface from cow-amm

3e5bfea

fix: lint run

68e8368

chore: merge dev

5afd282

chore: pull dev no-rebase

9390e64

fix: gas snapshot

b1ba43a

wei3erHase force-pushed the feat/helper-poc branch from ca413a4 to b1ba43a Compare July 9, 2024 15:11

refactor: reordered helper flow and cleanup

9ec1c34

Merge branch 'dev' of https://github.com/defi-wonderland/balancer-v1-amm

8d72ff8

into feat/helper-poc

wei3erHase marked this pull request as ready for review July 11, 2024 12:17

wei3erHase changed the title ~~[DRAFT] feat: helper PoC~~ feat: adding CoW helper MVP Jul 11, 2024

fedgiac reviewed Jul 11, 2024

View reviewed changes

test/integration/BCoWHelper.t.sol Outdated Show resolved Hide resolved

test/integration/BCoWHelper.t.sol Outdated Show resolved Hide resolved

fix: typos in comments

699996e

wei3erHase mentioned this pull request Jul 11, 2024

fix: overwriting sellAmount in order to avoid rounding issues #154

Merged

fedgiac approved these changes Jul 15, 2024

View reviewed changes

fix: overwriting sellAmount in order to avoid rounding issues (#154)

1bec1fd

* fix: typos in comments * fix: overwriting sellAmount in order to avoid rounding issues * feat: improving the test case to cover both cases

wei3erHase requested a review from 0xteddybear July 15, 2024 14:03

0xteddybear reviewed Jul 15, 2024

View reviewed changes

wei3erHase added 2 commits July 16, 2024 18:36

chore: addressing contract changes from PR comments

1ffda8b

chore: addressing comments in tests

063d3f6

wei3erHase requested a review from 0xteddybear July 16, 2024 16:52

wei3erHase added 4 commits July 16, 2024 18:53

chore: merge dev

471b0e6

fix: adding helper mock

e0de518

fix: addressing comments from PR

d0bba5c

refactor: deprecate fuzzed integration valid order test in favour of …

d8c49e6

…unit test

wei3erHase commented Jul 16, 2024

View reviewed changes

0xteddybear reviewed Jul 17, 2024

View reviewed changes

wei3erHase added 2 commits July 17, 2024 16:10

feat: simplifying helper integration test

abe6f00

feat: improving unit test for valid order

b2056be

wei3erHase requested a review from 0xteddybear July 17, 2024 14:14

feat: adding call tokens expectation

ffb1bc0

0xteddybear reviewed Jul 18, 2024

View reviewed changes

wei3erHase added 2 commits July 22, 2024 11:33

fix: branching different behaviours given skewness

7719c39

fix: adding comments on the skewness sign

7db4e8e

wei3erHase requested a review from 0xteddybear July 22, 2024 13:59

0xteddybear approved these changes Jul 22, 2024

View reviewed changes

wei3erHase merged commit c9a3849 into dev Jul 23, 2024
4 checks passed

wei3erHase deleted the feat/helper-poc branch July 23, 2024 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding CoW helper MVP #121

feat: adding CoW helper MVP #121

wei3erHase commented Jun 27, 2024 •

edited

Loading

fedgiac left a comment

fedgiac left a comment

0xteddybear left a comment

0xteddybear Jul 15, 2024

wei3erHase Jul 16, 2024

0xteddybear Jul 15, 2024

wei3erHase Jul 16, 2024

0xteddybear Jul 17, 2024

wei3erHase Jul 16, 2024

simon-something Jul 20, 2024

gas1cent Jul 22, 2024

0xteddybear Jul 17, 2024

0xteddybear Jul 17, 2024

0xteddybear Jul 17, 2024

wei3erHase Jul 17, 2024

0xteddybear Jul 17, 2024

wei3erHase Jul 17, 2024

0xteddybear Jul 17, 2024

0xteddybear Jul 17, 2024

wei3erHase Jul 17, 2024

0xteddybear Jul 18, 2024

0xteddybear left a comment •

edited

Loading

0xteddybear Jul 18, 2024

0xteddybear Jul 18, 2024

0xteddybear Jul 22, 2024

wei3erHase Jul 23, 2024

0xteddybear Jul 18, 2024

0xteddybear left a comment

0xteddybear Jul 22, 2024

wei3erHase Jul 23, 2024

0xteddybear Jul 22, 2024

0xteddybear Jul 22, 2024

feat: adding CoW helper MVP #121

feat: adding CoW helper MVP #121

Conversation

wei3erHase commented Jun 27, 2024 • edited Loading

Considerations in limited compatibility:

fedgiac left a comment

Choose a reason for hiding this comment

fedgiac left a comment

Choose a reason for hiding this comment

0xteddybear 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

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

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 left a comment • 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

0xteddybear 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

wei3erHase commented Jun 27, 2024 •

edited

Loading

0xteddybear left a comment •

edited

Loading