-
Notifications
You must be signed in to change notification settings - Fork 44
[DNM] ChainRpcProvider class with fallback capabilities #509
base: main
Are you sure you want to change the base?
Conversation
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 looks great, nice work.
Pending tests. |
bdf188a
to
bc20c0c
Compare
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.
Not fully clear on why we need a custom class yet.
bd7795e
to
927015f
Compare
Object.entries(chainProviders).map(([chainId, url]) => { | ||
hydratedProviders[chainId] = new StaticJsonRpcProvider(url as string, parseInt(chainId)); | ||
hydratedProviders[chainId] = new ChainRpcProvider(parseInt(chainId), url); |
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.
Noting that we replaced StaticJsonRpcProvider here - is there something special / different about StaticJsonRpcProvider vs JsonRpcProvider? Do we need to implement additional functionality or a 'Static' version of ChainRpcProvider?
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.
Hm, we switched to StaticJsonRpcProvider
because it cuts down on the number of RPC calls. We should just instantiate as StaticJsonRpcProvider
s under the hood. Is that possible?
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.
Yes but lower priority atm
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.
Do you mean replace JsonRpcProvider with StaticJsonRpcProvider entirely? Or just in this one use case?
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.
Yes replace entirely. That's what we already did AFAIK.
import { Provider } from "@ethersproject/abstract-provider"; | ||
import { BigNumber } from "@ethersproject/bignumber"; | ||
import { JsonRpcProvider, StaticJsonRpcProvider } from "@ethersproject/providers"; | ||
import { JsonRpcProvider } from "@ethersproject/providers"; | ||
import { ChainRpcProvider } from "@connext/vector-types"; |
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.
remove duplicate import
The Problem
Issue #410: #410
The Solution
The user should be able to specify in their router's config multiple provider addresses which can be used to fallback to.
For fallback functionality, we are leaning on ethersproject FallbackProvider, but have made a custom extending class
ChainProvider
which wraps that functionality but also enforces we are using JsonRpcProvider.Here we've implemented this change with backwards compatability by using comma-separated values. Example for
chainProviders
in config:TODO: Link change in docs where we've updated the config API.