Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Oct 21, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Aug 26, 2024

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 @ fix: update snapshots, nv23 DigitalMOB2/fevm-uniswap-v3-core#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 rvagg force-pushed the rvagg/upd branch 2 times, most recently from c44399b to 3785737 Compare August 26, 2024 06:02
@rvagg
Copy link
Member Author

rvagg commented Aug 26, 2024

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
Copy link
Member Author

rvagg commented Aug 26, 2024

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
Copy link
Member

BigLep commented Aug 27, 2024

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

@rvagg rvagg requested a review from snissn August 28, 2024 01:42
README.md Show resolved Hide resolved
libs-tests/package.json Show resolved Hide resolved
README.md Show resolved Hide resolved
@snissn
Copy link

snissn commented Sep 10, 2024

.. 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
  1. 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
Copy link
Member Author

rvagg commented Sep 10, 2024

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
Copy link

snissn commented Sep 10, 2024

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
Copy link
Member

BigLep commented Sep 17, 2024

@rvagg : are we good to merge this now?

@rvagg
Copy link
Member Author

rvagg commented Oct 2, 2024

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
Copy link
Member Author

rvagg commented Oct 21, 2024

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.

@rvagg rvagg merged commit 00e7061 into filecoin-project:main Oct 21, 2024
1 check failed
@rvagg rvagg deleted the rvagg/upd branch October 21, 2024 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

3 participants