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
850 stars 335 forks source link

SthChains improvements #452

Open sakulstra opened 1 year ago

sakulstra commented 1 year ago

Hey first time I noticed StdChains https://twitter.com/msolomon44/status/1699116841753534877 and checked if we can replace with our internal tooling for this.

Two things that would be quite useful for us is:

This is quite handy for when you want to e.g. ensure a script is run on a specific network.

This is quite handy as you can easily log addresses as blockexplorer link, making them directly clickable.

perhaps could make sense to somehow sync https://github.com/foundry-rs/forge-std/blob/master/src/StdChains.sol#L180 automatically with viem? It's a bit sad that support of chains is so different between:

mds1 commented 1 year ago

perhaps could make sense to somehow sync https://github.com/foundry-rs/forge-std/blob/master/src/StdChains.sol#L180 automatically with viem? It's a bit sad that support of chains is so different between:

The best I can think of here is to have all tools start from something like the chainlist chains.json file and each run their own script to convert that data into the format needed for their supported chains.

explicit constant getters for the chains defined (aka StdChains.Arbitrum)

What do you mean by this? Perhaps an example would be helpful. Since we do have the getChain method currently

in viem the chain struct also contains the blockexplorer

Adding block explorer URL for the reason mentioned is a nice idea. If we only add it to the Chain struct it should be backwards compatible since that's a return type only. If we added it to ChainData so you can call setChain with it that would be a breaking change, so we'd probably want to create a ChainDataV2 struct with a new version of setChain

sakulstra commented 1 year ago

@mds1, sure an example from a current project is having sth like this. We currently maintain a separate library which just exposes chainIds via a easy accessible namespace: https://github.com/bgd-labs/aave-helpers/blob/master/src/ChainIds.sol#L4

We use them e.g. to validate that a script is run on the correct network: https://github.com/bgd-labs/aave-helpers/blob/master/src/ScriptUtils.sol#L31

Or to work around issues with deal on certain networks: https://github.com/bgd-labs/aave-helpers/blob/master/src/CommonTestBase.sol#L34

Generally for us it has proved to be useful having chainIds exposed via human readable name, opposed to hardcoding numbers everywhere. Was just thinking as StdChains exposes this data anyways via getChain() could be useful to also expose supported chainIds via a more straightforward getter.

mds1 commented 1 year ago

Makes sense, so you can get the chain ID from a human readable name using the getChain(string) method, for example getChain("mainnet").chainId. Here's a forge-std that does this: https://github.com/foundry-rs/forge-std/blob/1d9650e951204a0ddce9ff89c32f1997984cef4d/test/StdCheats.t.sol#L404

sakulstra commented 1 year ago

@mds1 i understand but usability wise it is not the same right?

With ChainIds.ARBITRUM_GOERLI(or similar) I have a constant that autocompletes in a common editor. With assumeNotPrecompile(addr, getChain("optimism_goerli").chainId); on the other hand i need to know it's optimism_goerli, not Optimism_goerli etc.

mds1 commented 1 year ago

Right, it's just a string so it won't autocomplete. Probably could add an enum that has all the same strings as the enum values, along with a getChain(enum ChainAliases) overload