feat: add StdTypes
The rationale behind this PR is two-fold:
-
StdCheatshas become cluttered with structs - 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
StdCheatsusers who depend on the structs being in the namespace of the contract. They will have to rewrite their tests to import the structs fromStdTypes. I acknowledge this as a trade-off, but the PR still seems worth it to me.
It would be much cleaner (and more logical, since
Accountshould not be connected toStdCheats) 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
StdCheatsusers who depend on the structs being in the namespace of the contract. They will have to rewrite their tests to import the structs fromStdTypes.
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
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.
Going to close this one as stale and to avoid breaking changes