-
Notifications
You must be signed in to change notification settings - Fork 7
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
Base support #1534
Base support #1534
Conversation
✅ Deploy Preview for fractal-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Just 1 minor question to make sure all subgraphs are deployed
nativeTokenSymbol: mainnet.nativeCurrency.symbol, | ||
nativeTokenIcon: '/images/coin-icon-eth.svg', | ||
wagmiChain: mainnet, | ||
subgraphChainName: 'mainnet', | ||
subgraph: { | ||
space: 71032, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we have this subgraph deployed under new space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…n src/providers/NetworkConfig/networks/*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor suggestions about removing duplicated config. chain
and wagmiChain
seem to be the same, also name
can be derived from chain
, as well as nativeTokenSymbol
- don't see a reason to keep that configurable unless there's a clear reason why we'll be passing different values there
@@ -27,17 +27,23 @@ const CHAIN_ID = 1; | |||
const SAFE_VERSION = '1.3.0'; | |||
|
|||
export const mainnetConfig: NetworkConfig = { | |||
order: 0, | |||
chain: mainnet, | |||
rpcEndpoint: `https://eth-mainnet.g.alchemy.com/v2/${import.meta.env.VITE_APP_ALCHEMY_MAINNET_API_KEY}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hell yeah
@@ -27,17 +27,23 @@ const CHAIN_ID = 137; | |||
const SAFE_VERSION = '1.3.0'; | |||
|
|||
export const polygonConfig: NetworkConfig = { | |||
order: 20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why using tens in order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no real reason, just so that we "have room" when adding new networks (won't need to re-order all of the other networks).
For example, say we add Optimism next, and want it between Base and Polygon.
Currently Base is 10, Polygon is 20.
We could set the order
property in the new Optimism config to 15 and it'll show up where we want it to, without needing to adjust any of the other networks' configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome lgtm, same questions as Kirill
Closes #1532
Adds Base support
Some cleanup along the way:
src/providers/NetworkConfig/networks/*
The changes in this PR definitely succumbed to scope creep as I cleaned up the NetworkConfig type, but the updates were all super straightforward and I'm not running into any problems as I test locally