valiafetisov
commented
1 year ago CI is failing, but not because of our code. Both test commands proposed above work without an issue (see below).
Closed valiafetisov closed 1 year ago
valiafetisov
commented
1 year ago CI is failing, but not because of our code. Both test commands proposed above work without an issue (see below).
valiafetisov
commented
1 year ago I still need to review the tests
The tests are just cleaned up and simplified version of the Simulation.t.sol
I get that routes can't be discovered without the 1inch API, but this feels dirty. Might be necessary though.
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.
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
Besides dapp not working on M1 and being much slower, it also doesn't support envOr() cheat code that we had to use to make it work with the external API.
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 🙂
valiafetisov
commented
1 year ago 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)?
You have to set foundry-native FOUNDRY_ETH_RPC_URL env var. See full instruction on what needs to be done in order to execute the node script in the top PR comment, "Functional testing" section
valiafetisov
commented
1 year ago @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 envOr calls, otherwise we would prefer to keep them (since without it, node.js script is rendered unusable)
rockyfour
commented
1 year ago @valiafetisov latest changes look good, also got the js script running. Added 2 more minor comments.
Other than that let's wait for @godsflaw's review and foundry handling.
rockyfour
commented
1 year ago Hey @valiafetisov can you merge again from master please? We fixed some tests there, so once you do all tests should be green.
godsflaw
commented
1 year ago bump @valiafetisov, if you can pull from master and push that here we can finish this one up.
valiafetisov
commented
1 year ago Hey there, and thanks for introducing foundry to this repo!
The problem is: tests are still failing on the master branch with [FAIL. Reason: Assertion failed.] testGetLink() inside Simulation.t.sol. Currently, via the 8499d3f this PR have the latest changes from master and also fails with the same error.
I've also noticed that you don't use foundry.toml for storing foundry params, but custom test.sh instead. Also, no remappings.txt (which are helpful for the editor to pick up imported libs). Is there a specific reason for that? Do you imply that we should also remove those files or can we keep them?
rockyfour
commented
1 year ago 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.
valiafetisov
commented
1 year ago I've removed all extra files (thinking it might be a problem) but currently Ci still fails with Invalid provider url: builder error: relative URL without a base the error is not even coming from our test. Since I'm not an admin, I can't see or set secrets in the pipeline, could you please check if $ETH_RPC_URL has schema (eg starts with https://)?
valiafetisov
commented
1 year ago 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.
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 https://github.com/makerdao/exchange-callees/pull/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?
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 but something we're doing in this test (grabbing an old block?) is causing the test failures
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 hevm.rollFork(uint256 blockNumber) method: https://github.com/makerdao/exchange-callees/blob/1f9ba7b0e5d202774212c8ccfb9b0ac929ee9124/src/test/OneInchCallee.t.sol#L163
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
commented
1 year ago but something we're doing in this test (grabbing an old block?) is causing the test failures
So, this is the proof that the specified line was causing the problem: https://github.com/makerdao/exchange-callees/pull/40/commits/82cf064346ce11c7e324005d2e9791fa533e06aa You can go ahead and merge https://github.com/makerdao/exchange-callees/pull/40 instead of this one, if you don't mind fragile tests 🙂
rockyfour
commented
1 year ago @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). If I understand correctly the test only passes now because the LINK/DAI price went up since you created the tx data on January 18th, and in fact, it could have gone down by 1% (configured slippage) and it would still pass.
In order to be able to debug failures let's add the actual fromTokenAmount and toTokenAmount from the 1inch api response for the data we use, as well as the block (well, more or less since it's off-chain) for when it was queried.
Also, the configured allowed slippage is needed.
A good place to add these is near where we set oneInchTxData in the test.
Though we're all bullish on LINK, If you intend to rerun the js code to re-manufacture the above let's add a larger slippage this time (say 30%).
valiafetisov
commented
1 year ago Let's indeed comment out the rolling to a specific block (on this PR, as this is what we want to merge).
Done via e176bc1. Will you allow forks to access the RPC_URL? (since now it's the only reason it fails)
In order to be able to debug failures let's add the actual
fromTokenAmountandtoTokenAmountfrom the 1inch api response for the data we use
I'm not sure what you propose exactly. oneInchTxData already contains those values under SwapDescription.amount and SwapDescription.minReturnAmount accordingly, which we rewrite inside the callee in order to match exact values of the auction. This way, the only relevant information we take from 1inch is the swap route
as well as the block
The block number is already there 🙂
rockyfour
commented
1 year ago Will you allow forks to access the RPC_URL? (since now it's the only reason it fails) Let's leave that to @godsflaw, he's currently on vacation so let's not block on that.
This way, the only relevant information we take from 1inch is the swap route Ah right, I forgot how it works,
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)