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

SthChains improvements #452

Open
sakulstra opened this issue Sep 6, 2023 · 5 comments
Open

SthChains improvements #452

sakulstra opened this issue Sep 6, 2023 · 5 comments

Comments

@sakulstra
Copy link

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:

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

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

  • in viem the chain struct also contains the blockexplorer

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:

@sakulstra sakulstra changed the title SthChains getters SthChains improvements Sep 6, 2023
@mds1
Copy link
Collaborator

mds1 commented Sep 6, 2023

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
Copy link
Author

@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
Copy link
Collaborator

mds1 commented Sep 6, 2023

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:

assumeNotPrecompile(addr, getChain("optimism_goerli").chainId);

@sakulstra
Copy link
Author

@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
Copy link
Collaborator

mds1 commented Sep 6, 2023

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

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

2 participants