makerdao / exchange-callees

Exchange Callee contracts for LIQ2.0 Flash Loans
GNU Affero General Public License v3.0
31 stars 11 forks source link

Lp token support #12

Closed julienmartinlevrai closed 3 years ago

julienmartinlevrai commented 3 years ago

Yes @hexonaut thank you for your review, I also had these errors during développement. Going to figure out what they are.

julienmartinlevrai commented 3 years ago

@hexonaut it was a dust issue, now solved

hexonaut commented 3 years ago

Now I'm getting different failures:

Running 18 tests for src/test/Simulation.t.sol:SimulationTests
[PASS] testTakeLinkProfit() (gas: 1155061)
[PASS] testFrobMax() (gas: 355969)
[PASS] testSwapEthDai() (gas: 176412)
[BAIL] testGetLpDaiEth()
[BAIL] testBarkLpDaiEth()
[PASS] testSwapEthLink() (gas: 155817)
[BAIL] testBurnLpDaiEth()
[PASS] testDrip() (gas: 50085)
[PASS] testTakeLinkBasic() (gas: 1149598)
[PASS] testFailTakeLinkInsufficientProfit() (gas: 282788)
[PASS] testBarkLink() (gas: 736940)
[PASS] testWrapEth() (gas: 59887)
[PASS] testFailTakeLinkTooExpensive() (gas: 282722)
[PASS] testSwapLinkDai() (gas: 322338)
[PASS] testJoinLink() (gas: 257453)
[PASS] testTakeLinkNoProfit() (gas: 1155062)
[BAIL] testJoinLpDaiEth()
[BAIL] testTakeLpDaiEthBasic()

Failure: testTakeLpDaiEthBasic
Failure: testJoinLpDaiEth
Failure: testBurnLpDaiEth
Failure: testBarkLpDaiEth
Failure: testGetLpDaiEth
julienmartinlevrai commented 3 years ago

Probably the recent liquidations had something to do. Going to review.

julienmartinlevrai commented 3 years ago

@hexonaut there was a price dépendency assuming ETH was higher than 3K. I removed that reliance on the price, but I think there might be other such reliances elsewhere.

julienmartinlevrai commented 3 years ago

...and now other test are failing. Checking...

julienmartinlevrai commented 3 years ago

@hexonaut I fixed this new error. These simulation tests are really good, but also they tend to fail easily when mainnet conditions change.

hexonaut commented 3 years ago

@julienmartinlevrai Simulation tests should be made stable enough that they will not fail that easily. Often this will come up if using hard coded assumptions vs querying for values. For example, the amount of test collateral should not hard coded to some value as prices for collateral changes. Instead it should determine how much is needed to always be over the dust limit.

julienmartinlevrai commented 3 years ago

Thanks for your feedback. I will try to get rid of all of these assumptions.

Sent from ProtonMail mobile

-------- Original Message -------- On May 21, 2021, 1:09 PM, Sam MacPherson wrote:

@.***(https://github.com/julienmartinlevrai) Simulation tests should be made stable enough that they will not fail that easily. Often this will come up if using hard coded assumptions vs querying for values. For example, the amount of test collateral should not hard coded to some value as prices for collateral changes. Instead if should determine how much is needed to always be over the dust limit.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

julienmartinlevrai commented 3 years ago

Thanks @hexonaut, I am still working on making these tests even more resilient to market changes, I'm expecting to have them ready today.

hexonaut commented 3 years ago

Feel free to polish the tests some more, but this is not exactly a "high-stakes" contract. I wouldn't worry about spending too, too much time. The tests are passing and the numbers look like they are more resilient. Ideally you want to generalize the tests to cover all market conditions, but I think we are okay with just merging this as-is.

julienmartinlevrai commented 3 years ago

@hexonaut I finished improving the tests. Now I think they are very resilient to market changes.

julienmartinlevrai commented 3 years ago

...and just as I said that, CI failed with an error...

julienmartinlevrai commented 3 years ago

Fixed. Resiliency is hard when you are doing lots of swaps.