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

Move the Vm interface to a dedicated repo for enhanced modularity #353

Closed
PaulRBerg opened this issue Apr 18, 2023 · 6 comments
Closed

Move the Vm interface to a dedicated repo for enhanced modularity #353

PaulRBerg opened this issue Apr 18, 2023 · 6 comments

Comments

@PaulRBerg
Copy link
Contributor

To make my project PRBTest compatible with Foundry, I need access to the Vm interface (which is called the HEVM but I think in practice it's the REVM that is used now?), and this requires me to maintain a Vm copy.

Vulcan, another Foundry-based testing framework, is currently positioned as a layer built on top of Forge Std. However, I wonder if their primary interest isn't actually to build directly on Foundry? I see that there are several places where Vulcan imports Vm:

The general point is that by keeping the Vm isolated in this repository, Foundry inadvertently discourages the development of third-party testing frameworks beyond Forge Std.

Therefore, I suggest relocating the Vm interface to a dedicated repository. Doing so would enable the broader ecosystem to utilize it as a standalone module. Possible names for the repo include:

  • foundry-rs/vm
  • foundry-rs/forge-vm

Cc-ing @mds1, @ZeroEkkusu, @PatrickAlphaC, @gnkz, and @vdrg for feedback 🙏

@mds1
Copy link
Collaborator

mds1 commented Apr 18, 2023

All cheats will eventually be moved out of forge-std and directly into foundry foundry-rs/foundry#3782. The result will be that forge-std now only has things like the Vm solidity interface, other common interfaces, helper test/script contracts, etc. So this will effectively happen as a result of that change, but without the need for a new repo

@PaulRBerg
Copy link
Contributor Author

forge-std now only has things like the Vm solidity interface, other common interfaces, helper test/script contracts, etc.

I see, but this is still not as "slim" as my original proposal. I prefer my PRBTest users not to have access to those Forge Std helpers so they don't accidentally end up using them. I reckon the same rationale applies to future Vulcan versions and would-be testing frameworks not developed yet.

Anyway, I am happy to wait until all cheats are moved to Foundry to get a better sense of how Forge Std will look then.

@vdrg
Copy link
Contributor

vdrg commented Apr 19, 2023

I agree with @PaulRBerg , having a library with the core foundry stuff would enable the development of new frameworks that don't depend on forge std. As Paul said, it should include the vm interface and also struct definitions such as Log and any others that might be relevant to vm specific stuff. Including constants such as console address also makes sense.

@PaulRBerg
Copy link
Contributor Author

Any further thoughts on this issue, @mds1? I could cobble up a simple repo that implements my proposal, which would use the Vm as defined in PRBTest:

https://github.com/PaulRBerg/prb-test/blob/a245c71edc07643aedfe27df4cc7b21048aa8824/src/Vm.sol

The advantage of using that version instead of Forge Std is that my implementation uses NatSpec comments, which we could use for generating a simple documentation website with forge doc - or we could subsume the Vm documentation in the Foundry Book.

All cheats will eventually be moved out of forge-std and directly into foundry

This is the end goal, yes, but because of backward compatibility, it is likely that Forge Std will keep referencing the existing custom cheats and utils (i.e. StdCheats and StdUtils) for a long time.

@mds1
Copy link
Collaborator

mds1 commented Jun 6, 2023

After thinking about it more, I don't think this is worthwhile and is likely to cause more confusion/work than benefit:

  • As mentioned, cheats will eventually be upstreamed to forge eventually making forge-std smaller
  • There is already some user confusion around the foundry vs. forge-std distinction, e.g. around what's a native cheat vs. what's a stdCheat. Adding a third repo will fragment things more and likely lead to more confusion.
  • It results in more maintenance overhead for contributors, as new cheats now require updates to 4 repos instead of 3. (foundry, forge-std, book, and now vm repo).

The advantage of using that version instead of Forge Std is that my implementation uses NatSpec comments

Can't NatSpec just be upstreamed to the forge-std repo? Maybe I'm misunderstanding here.

To be clear, I definitely want to encourage forge-std alternatives, and love that users have both vulcan and PRB-Test as alternates. But I don't see how the current forge-std strucutre is problematic, as there are two options for vulcan/PRB-Test:

  1. Include forge-std as a dependency, import the Vm file only, and don't use the rest. Pros are that this is no extra work except for updating submodule version when the Vm file changes, cons are you now have a new dependency that you and your users may not want.
  2. The Vm file doesn't change too often, so maintain a separate copy in your forge-std-alternate repo and maintain that manually. Pros are you achieve the goals of this issue with a lightweight solution (no deps), cons are manually keeping that Vm version in sync with the forge-std version

@PaulRBerg
Copy link
Contributor Author

Thanks for your feedback. I don't agree with everything you said, but I also don't care so much about this issue, so I'm happy to agree to disagree and consider this issue settled.

around what's a native cheat vs. what's a stdCheat

Doesn't this run counter to your first point? If all cheats are eventually upstreamed in Foundry, then there won't be any confusion about what's a native cheat anymore.

as new cheats now require updates to 4 repos instead of 3. (foundry, forge-std, book, and now vm repo).

Fair point, but, as I said above, the book part could be automated by forge doc.

Can't NatSpec just be upstreamed to the forge-std repo?

Yeah, it could. That was a moot point on my part.

Include forge-std as a dependency,

I have deliberately attempted to avoid installing Forge Std because of its dependency on DSTest. Even if I don't use it explicitly, it feels awkward to have another testing assertions library as a dependency, which gets pulled every time PRBTest is installed (this would also increase the installation times with git submodules).

cons are you now have a new dependency that you and your users may not want

Yes - and which may accidentally be used by some users.

maintain a separate copy in your forge-std-alternate repo

Well, this is what all of us (Vulcan, plus all future Foundry ecosystem devs) will have to do if we want to avoid installing Forge Std as a dependency.

@PaulRBerg PaulRBerg closed this as not planned Won't fix, can't repro, duplicate, stale Jun 7, 2023
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

No branches or pull requests

3 participants