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

feat: foundry #65

Merged
merged 3 commits into from
Jan 15, 2024
Merged

feat: foundry #65

merged 3 commits into from
Jan 15, 2024

Conversation

pegahcarter
Copy link
Contributor

We have entered a new era of Forge and Foundry. As such, it is time to send truffle to the graveyard.

This PR:

  • Replaces all truffle asserts with ds-test asserts.
    • Mis-aligned bytes tests now fail as ds-test does not support comparisons. However, given the duration this repo has been live without issue, it should be fine to comment out.
  • Removes all truffle files, imports, and dependencies
  • Removes Migration code

I came across this change as truffle has outdated dependencies that still contain bugs.

@pegahcarter
Copy link
Contributor Author

I also had an issue using this package with pnpm, based on a babel dependecy. This PR fixes that.

@pegahcarter
Copy link
Contributor Author

@GNSPS

@GNSPS
Copy link
Owner

GNSPS commented Jan 4, 2024

Finally had time to review this PR. Sorry about the wait.

None of the tests kept their functionality! 🙈 In fact, the one that you converted from AssertBytes.<X> are not even testing any of the library inner workings, the entire test files should be deleted then.

My opinion is that regardless of how long this library has existed we shouldn't butcher all the tests and, if we do, we shouldn't keep them in a format where they don't really test anything we want them to.

I honestly want to try and help you get this PR through the finish line but I don't have time to go into it too deep. I'd need you to keep the AssertBytes library being used in the tests and keep the tests relevant for me to accept this.

@GNSPS
Copy link
Owner

GNSPS commented Jan 4, 2024

One could argue that the tests you created are new tests that actually do differential testing with both my assertion methods and the compiler's internal ones, so I'd be happy to keep them, in addition. 👍

@pegahcarter
Copy link
Contributor Author

pegahcarter commented Jan 4, 2024

You make a great point about keeping the tests functional. Oops.

I'll spend some time seeing if it's better to either keep or revert the foundry tests in addition to the AssertBytes.<X> tests. The only potential issue we may have is from removing:

// This next line is needed in order for Truffle to activate the Solidity unit testing feature
// otherwise it doesn't interpret any events fired as results of tests
Assert.equal(uint256(1), uint256(1), "This should not fail! :D");

Nonetheless, I'll get this PR moving and keep our dependencies light.

@pegahcarter
Copy link
Contributor Author

@GNSPS this is ready for review.

@GNSPS
Copy link
Owner

GNSPS commented Jan 15, 2024

OK, dope! LGTM! 🙏 Thank you for this, @pegahcarter!

Merging and publishing in NPM.

@GNSPS GNSPS merged commit ed7bfb7 into GNSPS:master Jan 15, 2024
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.

2 participants