feat: move shared contract addresses to shared repo #58
Conversation
src/constants.ts
Outdated
| BNB = 56 | ||
| } | ||
|
|
||
| export const SUPPORTED_CHAINS = [ | ||
| ChainId.MAINNET, | ||
| ChainId.RINKEBY, |
There was a problem hiding this comment.
you can get rid of some of these chains - theyre gone forever
There was a problem hiding this comment.
rinkeby, ropsten (i think on L2s too), and kovan as well - all test networks that dont exist anymore
There was a problem hiding this comment.
@lynnshaoyu I brought these chains over from the BE repo, can we deprecate across the org?
There was a problem hiding this comment.
confirmed offline that these can be removed across the org
| tickLensAddress: '0xd7f33bcdb21b359c8ee6f0251d30e94832baad07' | ||
| } | ||
|
|
||
| export const CHAIN_TO_ADDRESSES_MAP: Record<SupportedChainsType, ChainAddresses> = { |
There was a problem hiding this comment.
yay! this will make sor code referencing the addresses a lot simpler!
src/addresses.ts
Outdated
| v1MixedRouteQuoterAddress?: string | ||
| } | ||
|
|
||
| const DEFAULT_NETWORKS = [ChainId.MAINNET, ChainId.GOERLI, ChainId.SEPOLIA] |
There was a problem hiding this comment.
what makes a network default? slightly confused that sepolia is part of this when it doesn't use the DEFAULT_ADDRESSES unlike goerli and mainnet
There was a problem hiding this comment.
good callout, it's mainly used in the constructSameAddressMap which I put Sepolia in as it shares a uni address, but this was a mistake as it doesn't share many address overlaps as you pointed out and additional ones such as ens registrar for example. I will remove it from DEFAULT_NETWORKS and instead append it to the UNI_ADDRESSES map individually
cbachmeier
changed the title
cbachmeier
requested review from
NoahZinsmeister,
JFrankfurt,
ianlapham,
lynnshaoyu and
hensha256
|
Take a look at #59 to see how a new chain's addresses (Avalanche) are added |
| return DEFAULT_NETWORKS.concat(additionalNetworks).reduce<AddressMap>((memo, chainId) => { | ||
| memo[chainId] = address | ||
| return memo | ||
| }, {}) | ||
| } |
There was a problem hiding this comment.
I find the DEFAULT_NETWORKS piece a bit confusing personally--I know you've copied it from interface and divergent implementations would be a little annoying, but requiring explicit enumeration of the full set at runtime feels more predictable to me.
There was a problem hiding this comment.
Do you think the added clarity is worth the tradeoff of the redundancy of having to copy&paste Mainnet and Goerli in several locations? Could we address this in a different way such as renaming DEFAULT_NETWORKS?
We want to consolidate much of the repeated code in the UX and BE involved with launching on a new chain. This PR is to centralize a lot of the Uni contract addresses across chains in one place.
I've also restructured how these contracts are stored, so instead of a series of repeated strings such as
SEPOLIA_V3_CORE_FACTORY_ADDRESSESwithout any programmatic method to enforce consistency, there is a new datatypeChainAddresseswhose fields correspond to the related contract. There is a mapCHAIN_TO_ADDRESSES_MAPwhich will return all theChainAddressesfor a givenChainId. This helps simplify a lot of the helperconstssuch asV3_CORE_FACTORY_ADDRESSESwhich is now simply a reduce function across allSUPPORTED_CHAINS.Last change primarily affecting UX is I've swapped
SupportedChainIdforChainIdand nowSUPPORTED_CHAINSis an array which is the subset of all availableChainswhere Uni addresses have been deployed. This is the format used by the BE and is more robust than the previous soleSupportedChainId. This will change will likely require some small refactoring on the UX but will have us closer aligned to the BE.