Add 1inch callee #34
Merged
Add 1inch callee #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CI is failing, but not because of our code. Both test commands proposed above work without an issue (see below). Expected test output |
godsflaw
suggested changes
Jan 9, 2023
There was a problem hiding this comment.
Here's a quick review to get you started. I still need to review the tests and decide if all this node infrastructure is really appropriate here. I get that routes can't be discovered without the 1inch API, but this feels dirty. Might be necessary though.
I'm also aware you guys had trouble with dapp, and may try to make a foundry PR so you guys can base of that. I can show you better how to use foundry correctly. Not sure if we should do this before or after this PR yet.
src/OneinchCallee.sol
Outdated
|
|
||
| interface GemJoinLike { | ||
| function dec() external view returns (uint256); | ||
|
|
There was a problem hiding this comment.
Just a few nits on adding spaces between these interface definitions. I tightened it up for you.
Suggested change
There was a problem hiding this comment.
This was because of the prettier that is enabled by default by the vscode solidity extension. Disabled it for now or do you use a standard linter/prettier config across all projects that I can find somewhere?
There was a problem hiding this comment.
Mmm we currently don't use a linter afaik, so it's mostly manual code-style work/reviewing. I personally don't use vscode atm.
There was a problem hiding this comment.
Do you at least have those rules described in a natural language somewhere? In other words, how can we get and overview of all rules and pass this knowledge to other team members? 🙂
I personally don't use vscode atm
In principle, you don't have to use specific code editor to lint or prettify the code, most of the time those are standalone libraries, e.g.: https://consensys.github.io/smart-contract-best-practices/security-tools/linters-and-formatters/
src/OneinchCallee.sol
Outdated
Comment on lines
82
to
85
| constructor(address daiJoinAddress) public { | ||
| daiJoin = DaiJoinLike(daiJoinAddress); | ||
| dai = daiJoin.dai(); | ||
| dai.approve(daiJoinAddress, 2**256 - 1); |
There was a problem hiding this comment.
convention when setting variables in the contract that need to be passed in as function args is to either prefix or suffix them with an _. This repo already suffixes, so I changed the convention to that.
I also stripped the Address from the variable names for the same reason. Lastly, I changed the convention for max uint, usually expressed in 0.7.x and later as type.max(uint256), to the 0.6.12 equivalent uint256(-1). You had trouble compiling this because foundry was just told to use the latest version, where dapp is hard locked to 0.6.12. I think we should stick with dapp in this PR, but this entire repo should be migrated over to foundry, which we can do in another PR.
Suggested change
| constructor(address daiJoinAddress) public { | |
| daiJoin = DaiJoinLike(daiJoinAddress); | |
| dai = daiJoin.dai(); | |
| dai.approve(daiJoinAddress, 2**256 - 1); | |
| constructor(address daiJoin_) public { | |
| daiJoin = DaiJoinLike(daiJoin_); | |
| dai = daiJoin.dai(); | |
| dai.approve(daiJoin_, uint256(-1)); |
There was a problem hiding this comment.
Actually foundry also compiles it with 0.6.12 according to the logs, it's only the linter which didn't pick it up, I've set it to the correct version.
But can you explain why uint256(-1) is preferred over 2**256 - 1? I though they are interchangeable, but 2**256 - 1 does not depend on the value overflow, according to this discussion
There was a problem hiding this comment.
As per another suggestion, this was changed to type(uint256).max
The tests are just cleaned up and simplified version of the Simulation.t.sol
We've started with hardcoded response from their API, but later added js file as a proof of concept. Which I think speaks better than a documentation we might have written.
Besides dapp not working on M1 and being much slower, it also doesn't support It would be great if you can open a PR that adds foundry and also fixes existing failing tests unrelated to our code. Otherwise, we can also take care of that in the next PR. Feel free to give us some directions 🙂 |
rockyfour
reviewed
Jan 10, 2023
rockyfour
reviewed
Jan 11, 2023
rockyfour
reviewed
Jan 11, 2023
rockyfour
reviewed
Jan 11, 2023
rockyfour
reviewed
Jan 15, 2023
There was a problem hiding this comment.
Added some lower-resolution comments.
Wasn't able to run the index.js script to completion since the rpc url definition is missing from the test command line. Do you use an external env variable for that (see relevant comment)?
There is also the fact that dapp commands (and make test for example) are now failing due to the use of env vars. I think @godsflaw is looking into migrating to forge completely before this PR is merged, but otherwise the env vars used will need to be removed for now.
rockyfour
reviewed
Jan 15, 2023
You have to set foundry-native |
|
@rockyfour: all comments are now addressed. Let me know what's your progress on the migration of this repo to foundry or if you want us to do that (by directly commenting on our approach). If this is the only blocker from merging this PR, we can temporary remove |
rockyfour
reviewed
Jan 17, 2023
src/OneinchCallee.sol
Outdated
| @@ -0,0 +1,153 @@ | |||
| // SPDX-License-Identifier: AGPL-3.0-or-later | |||
There was a problem hiding this comment.
Please rename this file and the equivalent test file to match the contract name (so OneInchCallee.sol).
Currently, the branch does not compile.
There was a problem hiding this comment.
I've renamed it, but it's just a known problem with git that it ignores case-sensitive filename changes. So don't be surprised if you pull and it deletes your local files 🙂
rockyfour
reviewed
Jan 17, 2023
|
@valiafetisov latest changes look good, also got the js script running. Other than that let's wait for @godsflaw's review and foundry handling. |
5 tasks
|
Hey @valiafetisov can you merge again from master please? |
|
bump @valiafetisov, if you can pull from master and push that here we can finish this one up. |
|
Hey there, and thanks for introducing foundry to this repo! The problem is: tests are still failing on the master branch with I've also noticed that you don't use |
|
Pushed a fix for that failing test. Please try to merge again. We can add the foundry.toml file (see some examples for ones we use elsewhere - https://github.com/makerdao/dss-direct-deposit/blob/master/foundry.toml, https://github.com/makerdao/spells-mainnet/blob/master/foundry.toml). Let's try to remove the remappings.txt file as we don't use it elsewhere. |
|
I've removed all extra files (thinking it might be a problem) but currently Ci still fails with |
godsflaw
reviewed
Feb 13, 2023
There was a problem hiding this comment.
This is a complicated CI failure. The first problem is easy, your PR from a fork doesn't have access to the secret variable for ETH_RPC_URL. This is a default for security reasons, and we either need to find a way to give your specific fork access, or accept you as collaborators on this repo directly so that you can run the CI. I tested this by copying your PR over to a branch on this repo and just running it:
#40
If you look at the test output, you can see we get past the RPC url error, but we get this other error. Here is where things get a little more complicated. I also got this error locally a few times when I was using a specific RPC endpoint. I changed that endpoint, and the error stopped. I switch back locally, and now all the tests pass. I'm honestly not sure yet what's causing CI to fail on this, but something we're doing in this test (grabbing an old block?) is causing the test failures you see on that branch sometimes. I can no longer reproduce those failures locally, and they are only in CI at this point. Here are my local results on this branch:
Running 8 tests for src/test/OneInchCallee.t.sol:OneInchTests
[PASS] testBarkLink() (gas: 519745)
[PASS] testDrip() (gas: 72251)
[PASS] testFailTakeLinkOneInchWithTooMuchProfit() (gas: 2990661)
[PASS] testFrobMax() (gas: 184893)
[PASS] testGetLink() (gas: 24059)
[PASS] testJoinLink() (gas: 87644)
[PASS] testTakeLinkOneInchNoProfit() (gas: 2685350)
[PASS] testTakeLinkOneInchProfit() (gas: 4179072)
Test result: ok. 8 passed; 0 failed; finished in 1.45s
Running 4 tests for src/test/OasisDexCallee.t.sol:CalleeOtcDaiTest
[PASS] test_flash_take_no_profit() (gas: 1963089)
[PASS] test_flash_take_profit() (gas: 2184953)
[PASS] test_flash_take_profit_thin_orderbook() (gas: 2031309)
[PASS] test_rounding_error() (gas: 2169755)
Test result: ok. 4 passed; 0 failed; finished in 1.76s
Running 5 tests for src/test/UniswapV2Callee.t.sol:UniswapV2CalleeDaiTest
[PASS] test_complex_path() (gas: 2301483)
[PASS] test_flash_take_no_profit() (gas: 2114424)
[PASS] test_flash_take_profit() (gas: 2341713)
[PASS] test_flash_take_profit_thin_orderbook() (gas: 2180644)
[PASS] test_rounding_error() (gas: 2321112)
Test result: ok. 5 passed; 0 failed; finished in 1.76s
Running 1 test for src/test/OasisDexCalleeCharter.t.sol:CalleeOtcDaiTest
[PASS] test_flash_take_profit() (gas: 2333826)
Test result: ok. 1 passed; 0 failed; finished in 1.90s
Running 1 test for src/test/UniswapV2CalleeCharter.t.sol:UniswapV2CalleeDaiTest
[PASS] test_flash_take_profit() (gas: 2490340)
Test result: ok. 1 passed; 0 failed; finished in 1.90s
Running 7 tests for src/test/WstETHCurveUniv3Callee.t.sol:CurveCalleeTest
[PASS] testFail_badPoolFee() (gas: 1060186)
[PASS] testFail_maxPrice() (gas: 728593)
[PASS] test_baseline() (gas: 1005804)
[PASS] test_bigAmtWithComplexPath() (gas: 1277336)
[PASS] test_maxPrice() (gas: 1028368)
[PASS] test_poolFee() (gas: 1001757)
[PASS] test_profit() (gas: 1003392)
Test result: ok. 7 passed; 0 failed; finished in 17.37s
Running 30 tests for src/test/Simulation.t.sol:SimulationTests
[PASS] testBarkLink() (gas: 647940)
[PASS] testBarkLpUsdcEth() (gas: 770709)
[PASS] testBarkSteCRV() (gas: 1015986)
[PASS] testBurnLpUsdcEth() (gas: 334050)
[PASS] testDrip() (gas: 71921)
[PASS] testDripSteCRV() (gas: 71980)
[PASS] testExitSteCRV() (gas: 584695)
[PASS] testFrobMax() (gas: 305838)
[PASS] testFrobMaxSteCRV() (gas: 631847)
[PASS] testGetEthPrice() (gas: 12159)
[PASS] testGetLink() (gas: 142793)
[PASS] testGetLinkPrice() (gas: 12273)
[PASS] testGetLpUsdcEth() (gas: 255452)
[PASS] testGetLpUsdcEthPrice() (gas: 12141)
[PASS] testGetSteCRV() (gas: 103354)
[PASS] testGetSteCRVPrice() (gas: 12117)
[PASS] testGetUsdc() (gas: 143356)
[PASS] testGetWeth() (gas: 38458)
[PASS] testJoinLink() (gas: 215422)
[PASS] testJoinLpUsdcEth() (gas: 327308)
[PASS] testJoinSteCRV() (gas: 545173)
[PASS] testSwapLinkDai() (gas: 258382)
[PASS] testTakeLinkV2NoProfit() (gas: 2283114)
[PASS] testTakeLinkV2Profit() (gas: 3775840)
[PASS] testTakeLinkV3NoProfit() (gas: 2377326)
[PASS] testTakeLinkV3Profit() (gas: 3868832)
[PASS] testTakeLpUsdcEthNoProfit() (gas: 4569272)
[PASS] testTakeLpUsdcEthProfit() (gas: 8936453)
[PASS] testTakeSteCRVNoProfit() (gas: 1673304)
[PASS] testTakeSteCRVProfit() (gas: 1673278)
Test result: ok. 30 passed; 0 failed; finished in 17.36s
But I'm already a collaborator with write access to this repo, I can even merge this PR right now without any approvals 😉. Others, who don't have write access to the repo, doesn't even trigger the CI, as you can see from #35. Which I think is a correct behaviour – untrusted entities shouldn't even be able to run any code. Based on this, do you want to make secrets available to all CI actions? Otherwise, should I try resubmitting this PR as a brach of this repo, not from a fork to circumvent this limitation?
Yes, your assumption is correct, we're using historical data to guarantee reproducibility of the test that uses hardcoded 1inch transaction data. The data otherwise might become invalid in the future turning this into a glitching test, therefore to avoid it we've used exchange-callees/src/test/OneInchCallee.t.sol Line 163 in 1f9ba7b If it's too much operational overhead to enable historical data on the RPC endpoint provided to the CI, should we just remove this line and hope that it wouldn't break? (currently it works without it) 🙂 |
|
@valiafetisov sorry for the late response. Let's indeed comment out the rolling to a specific block (on this PR, as this is what we want to merge). In order to be able to debug failures let's add the actual A good place to add these is near where we set |
Done via e176bc1. Will you allow forks to access the RPC_URL? (since now it's the only reason it fails)
I'm not sure what you propose exactly.
The block number is already there 🙂 |
|
rockyfour
approved these changes
Feb 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds new 1inch callee contract and its tests.
Notes:
Functional testing
.envfile (in the root of the project) containing valid rpc url underFOUNDRY_ETH_RPC_URLnameforge test --match testTakeLinkOneinchProfit(cd scripts/1inch-callee-data-fetcher && npm ci && node index.js)