Skip to content
This repository has been archived by the owner on Dec 27, 2022. It is now read-only.

[DNM] ChainRpcProvider class with fallback capabilities #509

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jakekidd
Copy link
Contributor

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:

{
  "1337": "http://localhost:8545,http://localhost:8546",
  "1338": "http://localhost:8547"
}

TODO: Link change in docs where we've updated the config API.

Copy link
Contributor

@rhlsthrm rhlsthrm left a 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.

@rhlsthrm
Copy link
Contributor

Pending tests.

Copy link
Contributor

@rhlsthrm rhlsthrm left a 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.

@jakekidd jakekidd changed the title Implement chain provider class with fallback capabilities [DNM] ChainRpcProvider class with fallback capabilities May 26, 2021
Object.entries(chainProviders).map(([chainId, url]) => {
hydratedProviders[chainId] = new StaticJsonRpcProvider(url as string, parseInt(chainId));
hydratedProviders[chainId] = new ChainRpcProvider(parseInt(chainId), url);
Copy link
Contributor Author

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?

Copy link
Contributor

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 StaticJsonRpcProviders under the hood. Is that possible?

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

Copy link
Contributor

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";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove duplicate import

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants