AllOrNone check problem

LoopringSecondary / protocol2

Loopring protocol smart contract version2

8 stars 7 forks source link

AllOrNone check problem #20

Closed kongliangzhong closed 6 years ago

kongliangzhong commented 6 years ago

Consider the following test case:

Orders:
- Order 0: sell 10 token A, buy 10 token B, allOrNone == true
- Order 1: sell 5 token B, buy 5 token A, allOrNone == false
- Order 2: sell 6 token B, buy 6 token A, allOrNone == true
Rings:
- Ring 0: order 0, order 1
- Ring 1: order 0, order 2

Expected fill amounts of Order 0, 1, 2 should be (10, 10), (4, 4) and (6,6) Actual result: failed

kongliangzhong commented 6 years ago

By swap rings position from [[0, 1], [0, 2]] to [[0, 2], [0, 1]], it works as expected. so maybe we can rely on relay on this.

autumn84 commented 6 years ago

I think it is a right result, it should fail. And if we want to success, the position of the two rings should be swapped. Maybe it is better to optimize by relay.

kongliangzhong commented 6 years ago

please check test/testExchangeAudit.ts for details.

Brechtpd commented 6 years ago

It only failed in the simulator, the smart contracts partially filled order 0 (which of course should not be possible). Should be fixed in 3db2c64ffad1f95d2df07b933f9d6d70508dc87b.

But I agree, the relay should send the rings in the best order to maximize profits.

kongliangzhong commented 6 years ago

Ok, issue closed.