forge-std icon indicating copy to clipboard operation
forge-std copied to clipboard

feat: add StdTypes

Open PaulRBerg opened this issue 2 years ago • 2 comments

The rationale behind this PR is two-fold:

  1. StdCheats has become cluttered with structs
  2. Users cannot easily use the structs outside of a contract that doesn't inherit from StdCheats

I've recently bumped into the second issue while trying to use the new type Account introduced in https://github.com/foundry-rs/forge-std/pull/331:

import { StdCheats } from "forge-std/StdCheats.sol";

struct Users {
    StdCheats.Account admin;
    StdCheats.Account broker;
    StdCheats.Account recipient;
    StdCheats.Account sender;
}

It would be much cleaner (and more logical, since Account should not be connected to StdCheats) to be able to do this instead:

import { Account } from "forge-std/StdTypes.sol";

struct Users {
    Account admin;
    Account broker;
    Account recipient;
    Account sender;
}

Notes:

  • I have also ordered the structs alphabetically.
  • This PR will introduce a breaking change for some StdCheats users who depend on the structs being in the namespace of the contract. They will have to rewrite their tests to import the structs from StdTypes. I acknowledge this as a trade-off, but the PR still seems worth it to me.

PaulRBerg avatar Apr 17 '23 14:04 PaulRBerg

It would be much cleaner (and more logical, since Account should not be connected to StdCheats) to be able to do this instead:

Hmm, I agree it's cleaner because it's shorter, but that comes at the cost of losing context. Now it's less clear whether Account is something specific to your protocol vs. a test helper from forge-std. And if you do want a struct named Account in your protocol, now one of them needs to be aliased.

Related to this is: why do we move StdCheats structs into StdTypes, but not all the other structs from StdChains, StdStorage, etc? If we did that, then all structs no longer have any context which I think is confusing, as you need to search the code to know which structs are used where.

We'd also probably want to rename many structs to be prefixed with Std to make namespace collisions less likely.

This PR will introduce a breaking change for some StdCheats users who depend on the structs being in the namespace of the contract. They will have to rewrite their tests to import the structs from StdTypes.

Since we're at v1, I do want to try avoiding unnecessary breaking changes (there of course have been exceptions), but I'm not quite convinced this one is worth it given the above

mds1 avatar Apr 18 '23 12:04 mds1

comes at the cost of losing context

We could address this by nesting the types under a StdTypes library

not all the other structs from StdChains, StdStorage, etc?

Argh, I missed those.

all structs no longer have any context which I think is confusing

I don't think that that would be more confusing if we applied my herein suggestion to nest the types under StdTypes.

I'm not quite convinced this one is worth it

Sure, happy to wait for some time to see if others find this PR valuable.

PaulRBerg avatar Apr 18 '23 12:04 PaulRBerg

Going to close this one as stale and to avoid breaking changes

mds1 avatar Apr 25 '24 12:04 mds1