-
Notifications
You must be signed in to change notification settings - Fork 302
chore(api-reference) Add Contract Manager Support #2785
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
7 Skipped Deployments
|
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.
I have a few minor suggestions, but to me the most important thing is that I really don't think NETWORK_INFO makes sense when using contract manager, so I don't think it's wise to approach this by trying to rebuild it to match the old shape. Instead, my suggestion is to get rid of NETWORK_INFO entirely, and to update the call sites to use contract manager directly.
| } else if (chain.rpcUrls.default.http[0]) { | ||
| return [chain.id, http(chain.rpcUrls.default.http[0])]; |
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.
This logic should live in getRpcUrl -- it doesn't make sense that we have a function that should get the rpc URL which only sometimes returns the RPC url and other times there is still a valid URL but the consumer needs to know about different logic to retrieve it.
| rpcUrl: string; | ||
| networkId: number; | ||
| type: string; | ||
| nativeToken?: string; |
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.
I don't think you need to worry about adding types for fields that aren't in use with this app. Typescript objects are not closed (meaning they allow undeclared fields to be present), and only including fields you actually need allows typescript to be more helpful if we wanted to e.g. restructure the shape of EvmChains (typescript would be able to tell us if we've restructured things in a way that's breaking anything in use)
| const contractAddress = Object.values(EvmPriceFeedContracts).find( | ||
| (contract) => contract.chain === chain.id, | ||
| ); | ||
| return contractAddress?.address as `0x${string}`; |
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.
We should not be typecasting. For one, this is going to incorrectly mask the undefined case, and for two, if an invalid string were entered into EvmPriceFeedContracts, the typesystem would be incorrect and runtime would not operate in a way that matches what the type checker is guaranteeing.
Instead, it would be better to explicitly verify and throw exceptions if the data fails invariants you expect, e.g. something along these lines:
if (contractAddress === undefined) {
throw new Error(`No contract found for ${chain.id}`);
} else if (isZeroXString(contractAddress.address)) {
return contractAddress.address;
} else {
throw new Error(`Invalid contract address for ${chain.id}: ${contractAddress.address}`);
}
...
const isZeroXString = (val: string): val is `0x${string}` => val.startsWith("0x");| const mapEvmChainsToNetworkInfo = (chains: EvmChains[]) => { | ||
| const networkInfo: Record<number, NetworkInfo> = {}; | ||
|
|
||
| for (const chain of chains) { |
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.
A few notes:
- This doesn't seem to make a lot of sense to me. Why not just get rid of
NETWORK_INFOentirely and just useEvmChainsdirectly in the consuming files? This looks like it's just added indirection that just attempts to keep theNETWORK_INFOstructure the same as it was before, but the indirection doesn't add any other value and it shouldn't be hard to refactor things to get rid of this file entirely (the type checker should be very helpful here) - If you're going to do this, a more declarative / functional way is to use
Object.fromEntries:
const mapEvmChainsToNetworkInfo = (chains: EvmChains[]) =>
Object.fromEntries(chains.map(chain => [
Number.parseInt(chain.networkId, 10),
{
name: chain.id,
rpcUrl: chain.rpcUrl,
isMainnet: chain.mainnet,
contractAddress: getPriceFeedContractAddress(chain)
}
]));- as a minor nit, you should use
Number.parseIntto convert string to int (I think that's what's happening here), notNumber(), as theNumberconstructor is ambiguous and unclear (e.g. it's not clear to me from reading the code what conversion is actually happening)
| }; | ||
|
|
||
| // Convert EvmChains to array format | ||
| const evmChainsArray = Object.values(EvmChains) as EvmChains[]; |
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.
Please don't typecast -- if this does not automatically unify then it means something needs to be verified in order for the typesystem to be reliable. But I think if you follow my advice of doing away with NETWORK_INFO entirely, this will go away.
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.
Main things look good, but I have suggestions for how to improve the implementation
| export const getEvmPriceFeedContractAddress = ( | ||
| chainId: number, | ||
| ): `0x${string}` | undefined => { | ||
| const chain = evmChainsData.find((c) => c.networkId === chainId); | ||
| if (!chain) { | ||
| return undefined; | ||
| } | ||
| const contract = evmPriceFeedContractsData.find((c) => c.chain === chain.id); | ||
| if (!contract?.address || !contract.address.startsWith("0x")) { | ||
| return undefined; | ||
| } | ||
| return contract.address as `0x${string}`; | ||
| }; | ||
|
|
||
| export const getEvmWormholeContractAddress = ( | ||
| chainId: number, | ||
| ): `0x${string}` | undefined => { | ||
| const chain = evmChainsData.find((c) => c.networkId === chainId); | ||
| if (!chain) { | ||
| return undefined; | ||
| } | ||
| const contract = evmWormholeContractsData.find((c) => c.chain === chain.id); | ||
| if (!contract?.address || !contract.address.startsWith("0x")) { | ||
| return undefined; | ||
| } | ||
| return contract.address as `0x${string}`; | ||
| }; |
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.
This can be simplified:
| export const getEvmPriceFeedContractAddress = ( | |
| chainId: number, | |
| ): `0x${string}` | undefined => { | |
| const chain = evmChainsData.find((c) => c.networkId === chainId); | |
| if (!chain) { | |
| return undefined; | |
| } | |
| const contract = evmPriceFeedContractsData.find((c) => c.chain === chain.id); | |
| if (!contract?.address || !contract.address.startsWith("0x")) { | |
| return undefined; | |
| } | |
| return contract.address as `0x${string}`; | |
| }; | |
| export const getEvmWormholeContractAddress = ( | |
| chainId: number, | |
| ): `0x${string}` | undefined => { | |
| const chain = evmChainsData.find((c) => c.networkId === chainId); | |
| if (!chain) { | |
| return undefined; | |
| } | |
| const contract = evmWormholeContractsData.find((c) => c.chain === chain.id); | |
| if (!contract?.address || !contract.address.startsWith("0x")) { | |
| return undefined; | |
| } | |
| return contract.address as `0x${string}`; | |
| }; | |
| export const getEvmPriceFeedContractAddress = (chainId: number) => | |
| getContractAddress(chainId, evmPriceFeedContractsData); | |
| export const getEvmWormholeContractAddress = (chainId: number) => | |
| getContractAddress(chainId, evmWormholeContractsData); | |
| const getContractAddress = (chainId: number, contractsData: { chain: string, address: string }[]) => { | |
| const chain = evmChainsData.find(({ networkId }) => networkId === chainId); | |
| if (chain === undefined) { | |
| return undefined; | |
| } else { | |
| const contract = contractsData.find(contract => contract.chain === chain.id); | |
| if (contract?.address === undefined) { | |
| return undefined; | |
| } else if (isZeroXString(contract.address)) { | |
| return contract.address; | |
| } else { | |
| throw new Error(`Invariant failed: invalid contract address ${contract.address} for chain ${contract.chain}`); | |
| } | |
| } | |
| } | |
| const isZeroXString = (str: string): str is `0x${string}` => str.startsWith("0x"); |
Note in particular a few features:
- Removed code duplication
- Use of type guard for enforcing the dependent
0x${string}type, rather than using a typecast - Because we said that all contract addresses should always start with
0x, anything in contract manager that does not is invalid config data. Thus, this is an invariant failure and we should know about it as soon as possible (and have systems in place that make it impossible). Therefore, it is appropriate to throw so that it does not go undetected. - Removed early returns (I personally prefer to avoid early returns for the exact same reasons why I avoid goto statements, they break control flow and mean the code cannot be analyzed simply based on the control structure -- I've seen a lot of code that grows over time and early returns make grokking the code hard because it masks the actual control structure of the code).
- Using explicit
value === undefinedrather than!value; the latter may not behave how you want in all cases since falsiness in javascript is more broad and sometimes leads to cases that are broken in ways that are very nonobvious, e.g. if a0is ever passed. It doesn't affect anything in this specific code, but it's good habit to get into (I often catch myself using!valuetoo so it takes discipline) - Switched
ifclauses to be positives (i.e.thing === valueas opposed tothing !== value) which generally reads more naturally IMO
| export const getEvmChainRpcUrl = (chainId: number): string | undefined => { | ||
| const chain = evmChainsData.find((c) => c.networkId === chainId); | ||
| if (!chain) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // Let's try to use the viem chains without checking if they are working | ||
| const viemChain = Object.values(chains).find( | ||
| (c) => c.id === Number(chain.id), | ||
| ); | ||
| if (viemChain && viemChain.rpcUrls && viemChain.rpcUrls.default) { | ||
| return viemChain.rpcUrls.default.http[0]; | ||
| } | ||
|
|
||
| // Now let's try to use the json rpc url | ||
| if (chain.rpcUrl) { | ||
| return chain.rpcUrl; | ||
| } | ||
|
|
||
| return undefined; | ||
| }; |
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.
Minor nits but for similar reasons as above, I'd write this as:
| export const getEvmChainRpcUrl = (chainId: number): string | undefined => { | |
| const chain = evmChainsData.find((c) => c.networkId === chainId); | |
| if (!chain) { | |
| return undefined; | |
| } | |
| // Let's try to use the viem chains without checking if they are working | |
| const viemChain = Object.values(chains).find( | |
| (c) => c.id === Number(chain.id), | |
| ); | |
| if (viemChain && viemChain.rpcUrls && viemChain.rpcUrls.default) { | |
| return viemChain.rpcUrls.default.http[0]; | |
| } | |
| // Now let's try to use the json rpc url | |
| if (chain.rpcUrl) { | |
| return chain.rpcUrl; | |
| } | |
| return undefined; | |
| }; | |
| export const getEvmChainRpcUrl = (chainId: number) => { | |
| const chain = evmChainsData.find(({ networkId }) => networkId === chainId); | |
| if (chain === undefined) { | |
| return undefined; | |
| } else { | |
| const viemChain = Object.values(chains).find(({ id }) => | |
| id === Number.parseInt(chain.id, 10) | |
| ); | |
| return viemChain?.rpcUrls?.default?.http?.[0] ?? chain.rpcUrl; | |
| } | |
| }; |
Things to note here:
- Using
Number.parseIntinstead ofNumber(). Generally, I recommendparseIntoverNumberbecause it's more explicit (e.g. what doesNumber(something)actually mean? WithparseInt(something), it's very obvious that I'm parsing a string into an int) - Use optional chaining and nullish coalescing rather than explicit checks, which makes the code far more terse
- As above, removes early returns
Summary
TODO: Add a method to check if RPC is working. We can verify by calling
eth_chainIDand match with the present chainID.Rationale
How has this been tested?