Implement verify function #1

fedgiac · 2024-01-25T19:08:02Z

This PR implements the order verification function for a CoW AMM based on this paper as a conditional order.

How to test

Added unit tests that cover all functionalities of verify.

Other notes

For comparison, this repository keeps a copy of the contract written by Felix at this commit.

MartinquaXD

Can't contribute anything meaningful. 🙈

src/ConstantProduct.sol

mfw78

Minor nits on tests, and looks like 1 return value not used in testing harness (intentionally?)

test/ConstantProduct/ConstantProductTestHarness.sol

test/ConstantProduct/verify/ValidateOrderParametersTest.sol

mfw78 · 2024-01-29T13:07:46Z

test/ConstantProduct/verify/ValidateOrderParametersTest.sol

+        for (uint256 i = 0; i < sellTokenInvalidCombinations.length; i += 1) {
+            defaultOrder.sellToken = sellTokenInvalidCombinations[i][0];
+            defaultOrder.buyToken = sellTokenInvalidCombinations[i][1];
+
+            vm.expectRevert(abi.encodeWithSelector(IConditionalOrder.OrderNotValid.selector, "invalid sell token"));
+            verifyWrapper(orderOwner, defaultData, defaultOrder);
+        }
+
+        for (uint256 i = 0; i < buyTokenInvalidCombinations.length; i += 1) {
+            defaultOrder.sellToken = buyTokenInvalidCombinations[i][0];
+            defaultOrder.buyToken = buyTokenInvalidCombinations[i][1];
+
+            vm.expectRevert(abi.encodeWithSelector(IConditionalOrder.OrderNotValid.selector, "invalid buy token"));
+            verifyWrapper(orderOwner, defaultData, defaultOrder);
+        }


nit: could be extracted to a helper test function that also takes the revert message as an optional input.

I'd rather not do it because I want to make it clearer that we're rewriting the same order instance each time, and a helper function would take the order by reference and hide a bit that the same object is being modified.

test/ConstantProduct/verify/ValidateAmmMath.sol

mfw78 · 2024-01-29T13:28:09Z

test/ConstantProduct/verify/ValidateAmmMath.sol

+        uint256 poolOut = 1000 ether;
+        uint256 poolIn = 10 ether;
+        (ConstantProduct.Data memory data, GPv2Order.Data memory order) = setUpOrderWithReserves(poolOut, poolIn);
+
+        uint256 amountOut = 100 ether;


nit: should this be extracted to the setUp function?

I don't really want to move the amounts too far away from where the expected out amount is computed, since it's a key part of the test. In principle we may also want to add tests with different amounts.

fleupold · 2024-01-29T15:00:52Z

src/ConstantProduct.sol

+        }
+        // These are the checks needed to satisfy the conditions on in/out
+        // amounts for the function-maximising AMM.
+        if ((sellReserve - 2 * order.sellAmount) * order.buyAmount < buyReserve * order.sellAmount) {


can we put brackets around the subtraction (I never know what operation takes priority from just reading the code).

Also, I'm not sure I follow the formula. The FM-AMM has an implicit k (which is sellReserve * buyReserve). After the trade it will have a new k (which is at least (sellReserve - sellAmount) * (buyReserve + buyAmount)). The new k should be >= the old k.

Specifically:

sellReserve*buyReserve < (sellReserve-order.sellAmount)*(buyReserve+order.buyAmount)

Is this equivalent or am I missing something?

I agree with @fleupold : the check here is too strict. The formula as written checks that the solution maximizes the product of the solvers' liquidity reserves, under the assumption that there is a linear constraint: solvers can exchange any amount of tokens at a given price. But if the constraint is non-linear, then the check may fail even if solvers are optimizing correctly.

For this reason, I think the smart contract should simply check that it is trading at better terms that a constant product AMM. The right condition is
((sellReserve - order.sellAmount) * order.buyAmount < buyReserve * order.sellAmount)
and then trust the solvers' competition that the proposed traded amount actually maximizes the FM-AMM objective function

I'm surprised that we want to go for a weaker check onchain, when the cost of a stronger one is just an extra multiplication by 2.
The expressions you suggest (they are equivalent) means that the AMM is happy to trade as long as the trade is the same as coming from a Uniswap constant product curve.
My understanding instead is that we want to enforce the FM-AMM rules. They can be sidestep (this is discussed in "why batching trades is necessary" in the paper) but doing so has an extra gas cost and would be more visible to detect from the trades as a glance (because you need to split a trade in many smaller ones).
The difference in trade amount is very significant when the trade is large (in the example from my earlier writeup, for an ETH/DAI CoW AMM storing (25M DAI, 10k ETH), a trade of 2.5M can incur in an effective loss of 139 ETH).
The FM-AMM rules in the paper are enforced at a negligible extra cost with the formula that account for the FM-AMM rules.

I think I understand the discrepancy now (which isn't obvious IMO). The formula as stated right now asserts that the execution price exactly the new spot price after the trade rather than at least as good as the average price (what Uniswap does currently).

If I understand correctly, this will result in FM-AMM order executions most of the time contributing 0 surplus to the batch (as they will be matched exactly at their limit price). I think this is problematic in the current mechanism (as solvers trying to arb the AMM will have no advantage from an objective value perspective over solvers that don't arb).

Also, in the case of small price swings the limit price computed in getTradeableOrder may no longer be achievable and therefore no trade might happen (I'm not even sure partial fills make sense in that case as the acceptable price seems very closely related to the full order's size).

Therefore, I'd prefer the AMM expressing their minimum willingness to trade (as good as Uniswap) and have solvers compete for improving that price as much as possible via surplus.

I agree with @fleupold : to fit it within our current framework, the smart contract should check some minimal requirements (i.e., the limit price) and leave it to the solver competition to spit out the "correct" trade.

Math has been updated in bebead7.

Maybe this is related to the other discussion, but I believe the math as it currently stands allows violating k (ie it is not the same as asserting .

sellReserve*buyReserve < (sellReserve-order.sellAmount)*(buyReserve+order.buyAmount)

Let's say sellReserve is 1 and buyReserve is 1000, suggested sell amount 0.25 and suggested buy amount 330. Then the condition as currently implemented holds since

(sellReserve - order.sellAmount) * order.buyAmount < buyReserve * order.sellAmount = 0.75 * 330 = 247.5 < 1000 * 0.25 = 250

However 0.75 * 1330 = 997.5 and thus k has decreased.

Can we use forge fuzz tests to suggest random orders and if they are accepted assert that k is not violated?

In your example k decreases. The check in the code computes 247.5 < 250 and since the condition is true then the function reverts, so everything should work as expected.

Note that we can derive the current condition on the code from the condition on the k:

sellReserve*buyReserve ≤ (sellReserve-order.sellAmount)*(buyReserve+order.buyAmount)

Expanding:

sellReserve*buyReserve ≤ sellReserve*buyReserve - order.sellAmount*buyReserve + sellReserve*order.buyAmount - order.sellAmount*order.buyAmount

Simplifying:

0 ≤ - order.sellAmount*buyReserve + sellReserve*order.buyAmount - order.sellAmount*order.buyAmount

Factoring:

0 ≤ - order.sellAmount*buyReserve + order.buyAmount*(sellReserve - order.sellAmount)

Rearranging:

order.sellAmount*buyReserve ≤ order.buyAmount*(sellReserve - order.sellAmount)

For the revert condition the inequality must be swapped:

(sellReserve - order.sellAmount) * order.buyAmount < buyReserve * order.sellAmount

I'll look into fuzzing, but this will add more things to do to my backlog and I don't think the code will be ready by the end of today.

You are right, I keep missing that the if condition causes a revert (and thus needs to be negated mentally)

In the meantime, I added a few tests to make sure the inequality is in the right direction (that is, try to use a very large buy amount or very low sell amount and see that the order works).

…s the right one

fleupold

👏

Implement verify function

6b2fba0

fedgiac requested a review from a team January 25, 2024 19:09

mfw78 assigned fedgiac Jan 26, 2024

MartinquaXD reviewed Jan 29, 2024

View reviewed changes

src/ConstantProduct.sol Outdated Show resolved Hide resolved

More expressive error messages

82d73ec

mfw78 approved these changes Jan 29, 2024

View reviewed changes

mfw78 requested a review from fleupold January 29, 2024 13:47

fleupold reviewed Jan 29, 2024

View reviewed changes

fedgiac added 7 commits January 30, 2024 14:12

Remove unused return value

1b0119b

Move getDefaultData

4aff64c

Typo

e3b82a6

Move computation of expected out amount to dedicated function

f4e9518

Merge together setup steps for testing order parameters

68d5a80

Update AMM math to match Uniswap v2

bebead7

Check limit cases to make sure the direction of the math inequality i…

24bae24

…s the right one

fleupold approved these changes Jan 31, 2024

View reviewed changes

fedgiac merged commit 611dfc1 into main Jan 31, 2024
3 checks passed

fedgiac deleted the implement-verification-function branch June 6, 2024 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement verify function #1

Implement verify function #1

fedgiac commented Jan 25, 2024

MartinquaXD left a comment

mfw78 left a comment

mfw78 Jan 29, 2024

fedgiac Jan 30, 2024

mfw78 Jan 29, 2024

fedgiac Jan 30, 2024

fleupold Jan 29, 2024

acanidio-econ Jan 30, 2024

fedgiac Jan 30, 2024

fleupold Jan 30, 2024

acanidio-econ Jan 30, 2024

fedgiac Jan 30, 2024 •

edited

Loading

fleupold Jan 31, 2024 •

edited

Loading

fedgiac Jan 31, 2024

fleupold Jan 31, 2024

fedgiac Jan 31, 2024

fleupold left a comment

Implement verify function #1

Implement verify function #1

Conversation

fedgiac commented Jan 25, 2024

How to test

Other notes

MartinquaXD left a comment

Choose a reason for hiding this comment

mfw78 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

fedgiac Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

fleupold Jan 31, 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

fleupold left a comment

Choose a reason for hiding this comment

fedgiac Jan 30, 2024 •

edited

Loading

fleupold Jan 31, 2024 •

edited

Loading