chore(api-reference) Add Contract Manager Support #2785
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
7 Skipped Deployments
|
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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?