search

filecoin-project / fevm-contract-tests

Integration tests for the Ethereum JSON-RPC API in Lotus
Other
1 stars 2 forks source link

Update, update deps, add Makefile, make run faster #26

Closed rvagg closed 6 days ago

rvagg commented 2 months ago

A pretty large work-over to get this updated and running nicely, with easier setup and execute steps. In broad strokes:

  1. Add a Makefile to do all of the things - install, build, run tests, and some miscellaneous fixy things
  2. Move the web3.js and ethers.js tests into libs-tests/
  3. Solidity 0.8.23 for these tests
  4. Make kit/ a separate package, with its own dependencies
  5. Update dependencies to their latest versions
  6. Fix (hack) hardhat in each of the places it's used to have a shorter ethers.js polling interval, this can't be set anywhere with hardhat@2 because it wraps the ethers.js Provider in a proxy, maybe v3 will address this; for now we just sed in a pollingInterval so we get quicker tests runs and don't have to wait 4s for every .. single .. test
  7. Some minor fixes in the test kit Lotus, both JS and Go, most mostly cosmetic, also Go 1.22
  8. fevm-hardhat-kit has been changed to remove its connection with this repo (itest), so Makefile just sed's the config back in so it works again with the latest; we also just run deploy with this repo, there are no tests (I notice it was entirely removed from https://github.com/consensus-shipyard/fevm-contract-tests but deploy seems worthwhile to still do).
  9. For now, when running fevm-uniswap-v3-core tests, we just remove all of the jest snapshots for gas outputs because they're outdated and will fail. But we should restore those. I have a PR @ https://github.com/DigitalMOB2/fevm-uniswap-v3-core/pull/2 with current numbers but we may need to fork that repo into here if nobody's maintaining it there?

The tests run, but there are some failures in openzeppelin and uniswap, I'll post a follow-up with details of those.

rvagg commented 2 months ago

Also removed CircleCI config from here; although it may be useful as a historical reference for setting up GitHub Actions. But @snissn also has something comparable here already: https://github.com/consensus-shipyard/fevm-contract-tests/blob/main/.github/workflows/ci.yaml

rvagg commented 2 months ago

One outstanding item here is that I haven't sped up openzeppelin tests like the others, I think it might still be using the 4s polling interval with ethers.js but the fix I came up with doesn't apply to the way it's built. Later version of hardhat or something. I need to spend some time figuring that one out.

BigLep commented 2 months ago

PR to get @snissn write permissions to the repo: https://github.com/filecoin-project/github-mgmt/pull/63 (This will also allow us to request his review).

snissn commented 1 month ago

.. I just looked back over my set up and I see what my confusion was earlier. With these two changes I'm able to get the tests to run locally

  1. running 

    git submodule init
git submodule update

  2. the extern/fevm-hardhat-kit is expected a private key environment variable defined as PRIVATE_KEY but it doesn't seem to be set up anywhere in this PR

For above either fixing in documentation or in the setup scripts should be good.

Also I just want to document that am finding the test somewhat flaky I believe because of the 200ms block time. make test-libs is occasionally failing for me and make test-uniswap-v3-core consistently fails for example

      initialized with 5 observations with starting time of 5
        ✔ index, cardinality, cardinality next (65ms)
        ✔ latest observation same time as latest
        ✔ latest observation 5 seconds after latest (665ms)
        ✔ current observation 5 seconds after latest (687ms)
        ✔ between latest observation and just before latest observation at same time as latest
        ✔ between latest observation and just before latest observation after the latest observation (662ms)
        3) older than oldest reverts

I think leaving a note right now to document the issue is a good start and we can also: a. set time to be higher b. implement retry with doubling up to a certain number of times

rvagg commented 1 month ago

I was trying to abstract away all of the submodule stuff and the env var stuff too and make it handled by the Makefile.

Submodules are handled by the node building, but I think we could probably pull that up since it's not the only thing that needs submodules updated. The PRIVATE_KEY and all of the other uses of keys are all done in the Makefile. For fevm-hardhat Iinsert a new line into the hardhat config that picks it up properly and set up an .env in each of the submodules that has the vars they need.

My ideal here is that it's all make, nothing special, just a couple of top-level targets that handle it all for you. Then we can use that in CI too.

snissn commented 1 month ago

The PRIVATE_KEY and all of the other uses of keys are all done in the Makefile. For fevm-hardhat Iinsert a new line into the hardhat config that picks it up properly and set up an .env in each of the submodules that has the vars they need.

~Can you verify that? I see the Makefile setting up DEPLOYER_PRIVATE_KEY and USER_1_PRIVATE_KEY but not PRIVATE_KEY~

Ok I see the line here in the makefile https://github.com/filecoin-project/fevm-contract-tests/pull/26/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R45

I think I hit a really weird edge case where I ran make install before the submodules were checked out. This created a .env file in the project directory which prevented the make scripts from generating the .env file at extern/fevm-hardhat-kit/.env

I think that this a very common pattern for a future developer to run into and is worth either making a section in the readme or adding to the install scripts.

Otherwise it looks great to me! I think we should merge this as is and discuss the submodule nuance in a new ticket - at the very least it's a good way to document it.

BigLep commented 1 month ago

@rvagg : are we good to merge this now?

rvagg commented 3 weeks ago

FYI I still have some work to do on this, I've just got pulled away on more important things and haven't had time to fit this back in my head yet. I'll try and wrap this up soon but I have FIP0081 and ChainIndexer to chew through first.

rvagg commented 6 days ago

Some very minor tweaks and dependency updates but I'm calling this done for now. I'm currently running the full test suite and will post the results here afterward. But we don't have them all passing right now, particularly annoying is the ApplyWithGas errors that lotus throws that we need to sort out before this can be properly passing.