feat: foundry - Githubissues

GNSPS / solidity-bytes-utils

Utility Solidity library composed of basic operations for tightly packed bytes arrays

The Unlicense

514 stars 109 forks source link

feat: foundry #65

Closed pegahcarter closed 10 months ago

pegahcarter commented 11 months ago

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 commented 11 months ago

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

pegahcarter commented 10 months ago

@GNSPS

GNSPS commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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:

https://github.com/GNSPS/solidity-bytes-utils/blob/42221d3ee53aff8888057fb9f3b7a8fc42c290f8/test/TestBytesLib1.sol#L41-L43

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

pegahcarter commented 10 months ago

@GNSPS this is ready for review.

GNSPS commented 10 months ago

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

Merging and publishing in NPM.