search

foundry-rs / forge-std

Forge Standard Library is a collection of helpful contracts for use with forge and foundry. It leverages forge's cheatcodes to make writing tests easier and faster, while improving the UX of cheatcodes. For more in-depth usage examples checkout the tests.
Apache License 2.0
846 stars 333 forks source link

`v0.3` changes #139

Closed ZeroEkkusu closed 2 years ago

ZeroEkkusu commented 2 years ago

Required changes

Proposed changes

See source for description

Related issues

Should these issues be resolved?


Cc @mds1 @paulrberg Feel free to edit this post.

PaulRBerg commented 2 years ago

Feel free to edit this post.

It looks like I don't have permission to edit it (perhaps because I'm not part of the foundry-rs org). I would suggest editing the top of the post to be a task list, and add #126 as a checked off item.

Should these issues be resolved?

I don't have strong opinions on #127 (it would be nice to have, but not necessary), but I think #118 has become topical now in the context of the recent modularization we've built into Forge Std.

If Forge's future will be glorious, a large developer ecosystem will spawn around it. There will be many forks and remixes of the Assertions ,Cheats, etc. components offered by this repo, and similarly people will experiment with different flavors ofTest`.

But the name of the main contract Test is a bit vague, and it might often lead to confusion within Forge's future developer ecosystem (and it's also a lost opportunity in the sense that the contract name doesn't provide any marketing to Forge Std).

ZeroEkkusu commented 2 years ago

Wen PRB Test powered by Forge Std?


Proposed changes

Sgtm. I propose the filename to be SafeVm.sol, interface SafeVm, instance vm.

This would be a bigger breaking change. I would like Forge Std to follow the convention. We have already discussed this in #69. Cc @gakonst Edit: I just remember that we use snake_case for some functions e.g. checked_write and lower case for events e.g. log. Because of this inconsistency, I wouldn't mind if the names stay the same - we aren't following the convention in other places.

I am not a lawyer. I think the license I proposed can replace the dual license, is permissive, and there is a SPDX License Identifier.


Should these issues be resolved?

No.


Any edits

PaulRBerg commented 2 years ago

Wen PRB Test powered by Forge Std?

Well, I was hoping that Forge Std will be the one powered by PRBTest (as explained in https://github.com/foundry-rs/forge-std/issues/128).

What I will do now though is update my Foundry template to install both Forge Std and PRBTest.

Update: my template now comes with both PRBTest and Forge Std:

import { Cheats } from "forge-std/Cheats.sol";
import { PRBTest } from "@prb/test/PRBTest.sol";

contract ContractTest is PRBTest, Cheats {
    // ...
}
ZeroEkkusu commented 2 years ago

>= 0.8.0

If we are going to continue supporting earlier versions, then only PRB Test can do it.

install both Forge Std and PRBTest

Yeah, that's also an option.

gakonst commented 2 years ago

I'd like us to keep supporting older versions of Solidity in forge-std, so let's be careful with merging in anything that may break that!

PaulRBerg commented 2 years ago

@gakonst none of the features and changes proposed for v0.3 imply a change of the pragma. I have opened a separate discussion to discuss that: https://github.com/foundry-rs/forge-std/issues/125.

ZeroEkkusu commented 2 years ago

Update on CapWords:

We could add forge-std/CapWords/*.sol in v0.3 and use PRBTest instead of DSTest. Those contracts would not be backward compatible. Everything would be inherited/delegated to forge-std/*.sol contracts to minimize required maintenance. This is similar to what I did in #147.

So, if a user wants to use this version, they need only change the path:

import "forge-std/Test.sol";
import "forge-std/CapWords/Test.sol";
onbjerg commented 2 years ago

I don't think there is any compelling reason for us to bundle PRBTest in Forge Std instead of DSTest. The most compelling argument I've seen would be that DSTest is not versioned, but imo it is a non issue for Forge Std since we pick what commit of DSTest we use, and Forge Std is versioned. The rest of the arguments (missing assertions) are fully solveable by adding them to DSTest.

In terms of naming of Test, I don't feel strongly about it, but it's worth noting that you can provide aliases for imported types in Solidity.

ZeroEkkusu commented 2 years ago

I don't think there is any compelling reason for us to bundle PRBTest in Forge Std instead of DSTest.

If you're referring to what I said above, that is specifically to address the naming style (e.g. PRBTest has CapWords events, while they are snake_case in DSTest). Forge Std would still use DSTest, if I didn't communicate that well.

Users can choose the CapWords version with corrected names, if they want:

forge-std/Test.sol

import "ds-test/test.sol";

abstract contract Test is TestBase, DSTest, Assertions, Cheats, Utils {}

forge-std/CapWords/Test.sol

import "paulrberg/prb-test.sol";

abstract contract Test is TestBase, PRBTest, Cheats, Utils {}

That's a suggestion if we want to address https://github.com/foundry-rs/forge-std/issues/13.

PaulRBerg commented 2 years ago

I don't think there is any compelling reason for us to bundle PRBTest in Forge Std instead of DSTest

Building on top of the arguments I made in https://github.com/foundry-rs/forge-std/issues/128

  1. Letting PRBTest handle all testing assertions would take a burden off the shoulders of Forge Std.
  2. Path dependence. PRBTest can afford to be much more agile than DSTest, which is in line with Forge Std's philosophy to move fast and break things.
  3. PRBTest should be faster than DSTest for Solidity v0.8 users due to using unchecked and other modern features like <address>.code - see discussion in https://github.com/foundry-rs/forge-std/issues/125.
  4. As @ZeroEkkusu said, PRBTest uses the CapWords naming style.
  5. Friendlier open-source license (MIT instead of GPL) - see discussion in https://github.com/foundry-rs/forge-std/issues/132.
  6. Better internal documentation. If a Forge Std user looks up PRBTest.sol, they will see every function documented with NatSpec comments, which may prove to be helpful, especially for devs who are new to web3.

re fully solveable by adding them to DSTest.

Yes, but in practice updating DSTest is cumbersome because all DappHub repos depend on it via the master branch rather than a specific version. See my explanation here and the comment made by one of the DSTest maintainers here.

but imo it is a non issue for Forge Std since we pick what commit of DSTest we use

Forge Std users may have installed ds-test as a git submodule, in which case their commit of DSTest would take precedence over the commit picked by Forge Std.

Of course, the same argument applies to PRBTest, but PRBTest is versioned, so even if users install it directly, they can pull a specific version - in fact, this is what I recommend doing in the README.

onbjerg commented 2 years ago
  1. The burden is on DSTest mostly? Otherwise, I don't really think it's much of a burden if we do extend it, but there may be disagreements on that
  2. I don't see a point where we need to break DSTest more than we have, so I don't really consider this an issue - generally the maintainers have been helpful in adding new assertions etc, just not breaking existing behavior which I think is definitely doable for an assertion library.
  3. I don't agree with changing the pragma to >=0.8.0 <0.9.0
  4. I don't think this is a huge issue either, given the fact that these are debug events and they do in fact make them stand out more, which I think is desireable. Even moreso, this would actually increase the burden on Forge since we would now have to consider PRBTest-style events in addition to DSTest-style events.
  5. I don't know much about licenses so I can't really say much on this point
  6. I guess that's a good point but I see it as unlikely that the DSTest maintainers would not be open to adding docs to their functions

Yes, but in practice updating DSTest is cumbersome because all DappHub repos depend on it via the master branch rather than a specific version. See my explanation here and the comment made by one of the DSTest maintainers https://github.com/dapphub/ds-test/pull/21#issuecomment-903668033.

You are not required to update DSTest to use Forge. As far as I know, Forge supports all DSTest versions

Forge Std users may have installed ds-test as a git submodule, in which case their commit of DSTest would take precedence over the commit picked by Forge Std.

Could be solved by decoupling the library-features of Forge Std from the testing aspects, which I think is ok. Default experience would be what we have now, a more advanced experience would be w/o DSTest bundled in which should be really rare.

I think moving to PRBTest breaks a lot more things and requires a lot more coordinated effort across Foundry, Forge Std and the Book than it seems like

ZeroEkkusu commented 2 years ago

given the fact that these are debug events and they do in fact make them stand out more, which I think is desireable

Good point - in that case, PRBTest won't be needed.

Do we want to make a CapWords version, with debug events being an exception (that inherits from / delegates to original implementations)? @mds1

Otherwise, we are ready merge v0.3 if #147 looks good.

mds1 commented 2 years ago

+1 on keeping Test is DSTest for the reasons @onbjerg mentioned above.

Re CapWords issue, my impression was that it's scope was limited to just changing from e.g. library stdStorage to library StdStorage, and leaving the rest untouched. Though I see that would cause a clash with struct StdStorage, so I'm not sure the benefits from the rename are worth the hassle / breaking changes and would suggest closing #13

IIRC v0.3 has some breaking changes, including #147 (by removing cheatcodes from Script) as well as removing tip, and maybe some others. So IMO we should coordinate that merge with @onbjerg and co. to sync with the main foundry release.

We should also plug the v0.3 branch into some of our own repos and make sure nothing (unexpected) breaks. I have a repos I can test with next week, though they're all 0.8.x. If anyone has 0.6.x or 0.7.x repos to test against that would be great

PaulRBerg commented 2 years ago

TLDR; I'm not adamant about switching from DSTest to PRBTest - in fact, I didn't even want to discuss this in the context of v0.3, it was @ZeroEkkusu who proposed implementing it as part of the CapWords subdirectory. All I wanted out of v0.3 was modularity, which I've gotten as part of https://github.com/foundry-rs/forge-std/pull/126. What I will say though, is that if in the future "planets align" and there's greater interest in PRBTest, I would be happy to make the necessary changes in the Foundry Book and in Foundry (support CapWords debug events) myself.

  1. "burden" might not have been the best choice of words. Having assertions in multiple places is not great because of two reasons:
    1. Concerns are not separated
    2. Makes it more difficult for end users to know where to find the source code for the assertions that they use, where to ask for help, where to make PR to make contributions, etc.
  2. They have certainly missed some feature requests (see this and this). And then, there's nothing wrong with breaking changes if the assertions library is versioned. For instance, one of the ideas I have for PRBTest is Chai-like expressive language & readable style, which I don't think it's something that might get implemented in DSTest anytime soon.
  3. That's fine - though that wasn't what @ZeroEkkusu suggested. He didn't suggest changing the pragma globally, but only in the CapWords subdirectory.
  4. Fair enough, though I would offer to write the implementation for PRBTest-style events myself.

You are not required to update DSTest to use Forge

I think that we've been talking past each other on this point. What I was getting at by saying that "updating DSTest is cumbersome" is path dependence from a development point of view, i.e. DSTest not being as agile as PRBTest can afford to be.

I think moving to PRBTest breaks a lot more things and requires a lot more coordinated effort across Foundry, Forge Std and the Book

That's fair.

PaulRBerg commented 2 years ago

We should also plug the v0.3 branch into some of our own repos and make sure nothing (unexpected) breaks

I've been using this branch for a few weeks now in a private repo with 200+ tests, and it's been working fine. Though I've only used the Cheats contract so far (in conjunction with PRBTest) and it too was a v0.8 repo.

ZeroEkkusu commented 2 years ago

We were not all on the same page here, but we've resolved some issues 😄

Re CapWords @mds1: This is actually a compromise solution to correct all names and make no breaking changes. My only concern is maintenance. I'm going to try it now and push a branch, so we can take a look.

Edit: Here it is. See CapWords/.

Changes include:

If a user wants to use this version, they add CapWords/ in the path:

import "forge-std/Test.sol";
import "forge-std/CapWords/Test.sol";
mds1 commented 2 years ago

If a user wants to use this version, they add CapWords/ in the path:

In general I like the new name changes, but I'm not sure if carrying around both naming conventions is the right approach, as I think that will be confusing and fragment readability/tutorials, etc.

If we decide to go with the rename, this does introduce a lot of breaking changes in that all docs need to be updated, existing third-party articles/tutorials become outdated, etc. So worth considering carefully if the change is worth the costs. Currently where I lean is that v0.3 should minimize changes and we can reconsider larger breaking changes like this in v1.0.0.

ZeroEkkusu commented 2 years ago

We should decide whether to bump pragma to 0.6.2 to get interface inheritance. See https://github.com/foundry-rs/forge-std/pull/147#discussion_r944974402.

I've opened PRs for the remaining changes.

mds1 commented 2 years ago

Given a convo I had with someone from maker around potentially only supporting 0.8 for parseJson, I think bumping the minimum pragma to 0.6.2 is ok since I don't think anyone is using lower than that

PaulRBerg commented 2 years ago

potentially only supporting 0.8 for parseJson

Sounds like something worth mentioning in https://github.com/foundry-rs/forge-std/issues/125!

mds1 commented 2 years ago

We just added pragma experimental ABIEncoderv2 which gave support for lower versions too so it ended up not being an issue, though we thought it might be for a bit 🙂

I'll leave a comment on that issue later with some thoughts/takeaways from that

ZeroEkkusu commented 2 years ago

When should we merge?

mds1 commented 2 years ago

@ZeroEkkusu I think we can probably open a PR and get some eyes on it. Do you mind opening that PR and copying over the list of changes/breaking changes into the PR description? Once that PR is open I'm going to ask a few people to test it on some repos as well before we merge it

ZeroEkkusu commented 2 years ago

@mds1, PR ready - https://github.com/foundry-rs/forge-std/pull/184.

ZeroEkkusu commented 2 years ago

We'll continue the conversation there.

I think it is okay to make it v1.0.0.