Modularize forge-std

[PaulRBerg]

Implements #122, #123, and #124.

Modularization

• Assertions and logs moved to Assertions.sol
• Cheatcodes moved to Cheats.sol
• Errors lib stdErrors moved to Errors.sol
• Math lib stdMath moved to Math.sol
• Utilities (such as bound) moved to Utils.sol - also moved the utilities from Script.sol to Utils.sol
• Test now inherits from Assertions, Cheats and Utils

New Features

• createCompute2Address utility

Fixes

• Drop the pragma to 0.8.0 in StdErrors.t.sol

Breaking Changes

• Removed the now-deprecated lowLevelError custom error
• Removed the now-deprecated tip cheat
• Test doesn't inherit from Script anymore

I'm very much open to any feedback. Feel free to destroy this.

[mds1]

In general some cleanup and modularity seems good to me. I haven't looked closely at this yet, but just commenting to say that if we're going to a big refactor like this there's some other things we should consider including. Namely, splitting Vm into something like VmSafe and Vm is VmSafe, where VmSafe is a subset that's safe to use on scripts intended for a live network, so you don't accidentally e.g. change the block timestamp and use a fake timestamp as a value somewhere. You can also imagine a version where all cheatcodes like vm.roll are replaced with roll which are aliases for anvil RPC calls, to help scaffold local nodes for e.g. frontend dev/testing.

I wrote up some of this this in #78, I'll need to review that to see if I still agree with it 😅

As a side note, thanks for all the recent issues/PRs to forge-std! 🙌

[PaulRBerg]

if we're going to a big refactor like this there's some other things we should consider including.

Yep, happy to move this to another bigger PR if appropriate.

Alternatively, happy to split this PR into its constituent components so reviewing each proposal is easier.

splitting Vm into something like VmSafe and Vm is VmSafe, where VmSafe is a subset that's safe to use on scripts intended for a live network

That would be cool!

As a side note, thanks for all the recent issues/PRs to forge-std! 🙌

Very glad that I can help.

[brockelmore]

fwiw this is how it used to be structured, but from a dev UX standpoint, it seemed monolithic was generally better. I am sympathetic to wanting to modularize, and as long as basically all normal users can still import a single test contract and it be clean (single inherit, no MyLib.funcCall, etc), I am okay with it.

cc @gakonst

[PaulRBerg]

it seemed monolithic was generally better

My proposal would still be "monolithic" for the end user - it's just that we internally would maintain the Cheats as a separate entity. The primary advantage (as I said in #122) would be to make Forge Std more modular and composable with other testing frameworks than DSTest.

single inherit, no MyLib.funcCall

Yeah that wouldn't be cool.

[ZeroEkkusu]

I'm very supportive of making Forge Std modular to allow for maximal interoperability. It'd be great to standardize more things about testing in Solidity / frameworks, but that's out of scope of this PR, of course.

I just took a look at the changes and have a few questions.

One suggestion I have is to modularize every part of Forge Std (Std Errors, etc) and then just import them in Test.

Notes:

• We should make a new release before implementing these changes
• The tip std-cheat, which was deprecated, is removed

+1 for keeping naming simple with Cheats, Storage, etc!

[gakonst]

this should be inherited

Suggested change

	Vm private constant vm_cheats = Vm(address(uint160(uint256(keccak256("hevm cheat code")))));
	Vm internal constant vm_cheats = Vm(address(uint160(uint256(keccak256("hevm cheat code")))));

[PaulRBerg]

You mean inherited in Test?

The problem with marking it as internal is that there would be collisions with the third-party testing frameworks which need to also define vm such that that framework can used independently of forge-std (for example, I define vm in PRBTest).

I suggest leaving it as private. Forge Std already redeclares the vm in Test.

[ZeroEkkusu]

I think a good approach to enabling composability long-term may be for individual components (Cheats, etc) to provide only the functionality they are made for.
E.g. Cheats should expose std-cheats; not vm because it's a Forge thing and not a feature of Cheats. This will also prevent collisions when composing, and some devs might come up with interesting combinations we cannot foresee.

Additionally, every component should be a self-contained unit, with its own private vm and stdstore if it needs it, so it does not depend on another component. I don't think that would be too much hassle.

Specifically made end-user contracts, such as Test and Script can expose vm and stdstore.

[PaulRBerg]

Very much agree with @ZeroEkkusu's resolution here.

every component should be a self-contained unit, with its own private vm and stdstore

+1

[PaulRBerg]

Hi @ZeroEkkusu, thanks very much for your review.

One suggestion I have is to modularize every part of Forge Std (Std Errors, etc) and then just import them in Test.

I'll work on this and push another commit to this PR.

We should make a new release before implementing these changes

yep! v0.2.0 I guess?

The tip std-cheat, which was deprecated, is removed

Yeah well I have thought that since this introduces breaking changes anyway, we may just be able to delete the tip function.

[PaulRBerg]

@ZeroEkkusu I have addressed all of your comments. Also, in my recent commit, I have created two new files Errors.sol and Math.sol to modularize stdErrors and stdMath. Let me know if that looks good to you!

[ZeroEkkusu]

I left a couple of suggestions.

[Modularize] I'll work on this and push another commit to this PR.

Should we move out the assertions, as well?
The logs can stay inside Test because they are specific to Foundry, no need for a header.

As for bound, I thought we could add it and the utils from Script to Utils.sol, but haven't thought about this much.

[PaulRBerg]

Should we move out the assertions, as well?

Just did it!

The logs can stay inside Test because they are specific to Foundry, no need for a header.

Actually, no. I had to move the logs to the newly created Assertions file because the assertions depended upon some logs, e.g. log_array.

As for bound, I thought we could add it and the utils from Script to Utils.sol, but haven't thought about this much.

Hah, this was my 2nd proposal from #123.

I just did this in the last commit. The only cost was the inability to use log_named_uint in bound anymore - though this should be fine? I don't see why users would want Forge Std to log the bounded value.

[ZeroEkkusu]

I had to move the logs to the newly created Assertions file because the assertions depended upon some logs, e.g. log_array.

Right, I forgot about this!

Here's an idea for logging the bound result:

[PaulRBerg]

Okay, this is starting to look good! @ZeroEkkusu, I have made the following final touches:

1. Fix the compiler warning in bound - Solidity complained that the return value from the staticcall wasn't used.
2. Re-add vm to Script to fix the mistake you spotted in one of the comments above.
3. Define console2's address as a constant in Utils.sol, for better readability.

I will work on fixing the git conflicts tomorrow if this also LGT everyone else?

[ZeroEkkusu]

It is starting to lg. Thank you for all the effort and patience.

I will work on fixing the git conflicts

I checked today, the conflicts are minimal.

[PaulRBerg]

Hey @mds1, thanks for your review.

There's lot of related issues, where's the best place to get an explanation of the new architecture and UX being implemented in this PR?

@ZeroEkkusu and I have worked our way up to modularize the code base commit by commit .. besides the original proposals that I made in #122, #123, and #124, there's no central place where all changes are documented. What I will do now though is write a brief spec in my PR comment body at the top - let me know what you think.

[ZeroEkkusu]

Team, this is the recap of the PR:

• No breaking changes besides the ones we've made deliberately
• The library is now modular
• One gotcha: DSTest is not flexible and we could not separate IS_TEST from the assertions; we inherited DSTest in both Test and Assertions

Lgtm.
@mds1, we should consider merging into v0.3.

I will open an issue to track proposals for v0.3.

[PaulRBerg]

Thanks very much @ZeroEkkusu for your continuous guidance on this PR!

No breaking changes besides the ones we've made deliberately

In case you're looking for a list of breaking changes, see my top comment.

[gakonst]

Went through the code and the refactor LGTM. Trust you guys above that the user testing makes sense and that there's no critical breaking changes. Would be helpful to document if any.

[ZeroEkkusu]

Alright, we're merging this into v0.3! Thank you @PaulRBerg for taking the initiative and bearing with us!

If Matt finds something is missing, we can make changes in v0.3.

[PaulRBerg]

Whoop whoop!

I went ahead and closed issues #122, #123 and #124.