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

ethereum: rm truffle #4070

Merged
merged 3 commits into from
Aug 20, 2024
Merged

Conversation

evan-gray
Copy link
Contributor

@evan-gray evan-gray commented Aug 9, 2024

This PR fixes #4069, removing the remaining CI dependencies on truffle and porting the remaining tests from jest to forge (borrowed from #3395).

This also offers some modest speed improvements such as faster npm ci and an ethereum-upgrade test runtime of 4m 22s instead of 7m 20s. 😀

Note: This change explicitly does not remove the remappings line for 'truffle/=node_modules/truffle/', as that, irritatingly, resulted in a hash change in the byte code. Actually had to add it back into the relayer one. Just like #4049, this can be verified by building locally (ensure you are on forge 0.2.0 (55bf415 2024-08-04T00:22:40.167411524Z)) or by exec'ing into eth-devnet-0:tests and diffing the following, where only test files should have changed:

cd ~/app/ethereum/build-forge
find . -type f -exec sha256sum {} \;
cd ~/app/relayer/ethereum/build-forge
find . -type f -exec sha256sum {} \;

@evan-gray evan-gray force-pushed the rm-truffle branch 3 times, most recently from a4ed7d0 to 0a50809 Compare August 11, 2024 21:19
Copy link
Contributor

@panoel panoel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look at the forge tests or scripts.

Copy link
Contributor

@gator-boi gator-boi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the ethereum directory. The tests in this PR are meant to replicate the existing javascript tests. However, these tests need a lot of work and there should be a follow up PR that fixes these tests (and adds a more comprehensive test suite). Here is a short list of enhancements we could add to improve the test suite:

  • Splitting out tests into positive and negative tests. Currently a lot of the revert statements are triggered in the same test that evaluate happy path logic.
  • Changing the test names (and add a comment for each test describing what the test does). Some of the test names are insanely long and are difficult to read.
  • Adding helper functions for payload generation. For example, the Bridge tests could use a method that generates a token transfer payload given some common inputs.
  • Write helper functions to set up certain testing scenarios rather than calling other tests internally.
  • Use the wormhole interfaces in the tests instead of duplicating events, and constants.
  • Fuzz testing

@evan-gray evan-gray merged commit c60e755 into wormhole-foundation:main Aug 20, 2024
24 checks passed
@evan-gray evan-gray deleted the rm-truffle branch August 20, 2024 22:30
@nonergodic
Copy link
Collaborator

I'm late to this, but given that truffle has been removed, could we now merge the relayer back into the Ethereum folder? (see original PR why it was moved out)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ethereum: rm truffle
5 participants