Skip to content
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

Reduce usage of RPC providers via relying on them sparingly #2271

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { useIsActiveWallet } from 'hooks/useIsActiveWallet'
import { ConnectWalletOption } from '@cow/modules/wallet/api/pure/ConnectWalletOption'

import CowImage from 'assets/cow-swap/cow_v2.svg'
import { RPC_URLS } from 'constants/networks'
import { getRpcUrls } from 'constants/networks'

import { TryActivation, onError } from '.'
import { Web3ReactConnection } from '../types'
Expand Down Expand Up @@ -38,7 +38,7 @@ const [web3CoinbaseWallet, web3CoinbaseWalletHooks] = initializeConnector<AsyncC
new m.CoinbaseWallet({
actions,
options: {
url: RPC_URLS[SupportedChainId.MAINNET],
url: getRpcUrls(SupportedChainId.MAINNET)[0],
tukantje marked this conversation as resolved.
Show resolved Hide resolved
appName: 'CoW Swap',
appLogoUrl: CowImage,
reloadOnDisconnect: false,
Expand Down
4 changes: 2 additions & 2 deletions src/cow-react/modules/wallet/web3-react/connection/ledger.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { ConnectWalletOption } from '@cow/modules/wallet/api/pure/ConnectWalletO
import { default as LedgerImage } from '@cow/modules/wallet/api/assets/ledger.svg'
import { initializeConnector } from '@web3-react/core'
import { Web3ReactConnection } from '../types'
import { RPC_URLS } from 'constants/networks'
import { PROVIDERS_RPC_URLS_FIRST_ONLY } from 'constants/networks'
import { AsyncConnector } from './asyncConnector'

const BASE_PROPS = {
Expand All @@ -26,7 +26,7 @@ const [ledger, ledgerHooks] = initializeConnector<AsyncConnector>(
return new m.Ledger({
actions,
options: {
rpc: RPC_URLS,
rpc: PROVIDERS_RPC_URLS_FIRST_ONLY,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we could add the comment in the same line as the one of coinbase explaining that ledger allows just one RPC per network

},
kit,
})
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { initializeConnector } from '@web3-react/core'
import { Network } from '@web3-react/network'

import { RPC_URLS } from 'constants/networks'
import { ConnectionType } from '@cow/modules/wallet'
import { Web3ReactConnection } from '../types'
import { PROVIDERS } from '@src/custom/constants/networks'
Copy link
Contributor

Choose a reason for hiding this comment

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

drop the @src/custom part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


const [web3Network, web3NetworkHooks] = initializeConnector<Network>(
(actions) => new Network({ actions, urlMap: RPC_URLS, defaultChainId: 1 })
(actions) => new Network({ actions, urlMap: PROVIDERS })
shoom3301 marked this conversation as resolved.
Show resolved Hide resolved
)
export const networkConnection: Web3ReactConnection = {
connector: web3Network,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { useWalletMetaData } from '@cow/modules/wallet'
import { initializeConnector } from '@web3-react/core'
import { WalletConnect } from '@web3-react/walletconnect'

import { RPC_URLS } from 'constants/networks'
import { PROVIDERS_RPC_URLS } from 'constants/networks'
import { Web3ReactConnection } from '../types'
import { default as WalletConnectImage } from '@cow/modules/wallet/api/assets/walletConnectIcon.svg'
import { WC_DISABLED_TEXT } from '@cow/modules/wallet/constants'
Expand All @@ -29,7 +29,7 @@ const [web3WalletConnect, web3WalletConnectHooks] = initializeConnector<WalletCo
new WalletConnect({
actions,
options: {
rpc: RPC_URLS,
rpc: PROVIDERS_RPC_URLS,
qrcode: true,
},
onError,
Expand Down
15 changes: 1 addition & 14 deletions src/cow-react/modules/wallet/web3-react/hooks/switchChain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,9 @@ import { walletConnectConnection } from '@cow/modules/wallet/web3-react/connecti
import { Connector } from '@web3-react/types'
import { getChainInfo } from 'constants/chainInfo'
import { SupportedChainId } from '@cowprotocol/cow-sdk'
import { RPC_URLS } from 'constants/networks'
import { getRpcUrls } from 'constants/networks'
import { isChainAllowed } from '../connection'

function getRpcUrls(chainId: SupportedChainId): [string] {
switch (chainId) {
case SupportedChainId.MAINNET:
case SupportedChainId.GOERLI:
return [RPC_URLS[chainId]]
case SupportedChainId.GNOSIS_CHAIN:
return ['https://rpc.gnosischain.com/']
default:
}
// Our API-keyed URLs will fail security checks when used with external wallets.
throw new Error('RPC URLs must use public endpoints')
}

export const switchChain = async (connector: Connector, chainId: SupportedChainId) => {
if (!isChainAllowed(connector, chainId)) {
throw new Error(`Chain ${chainId} not supported for connector (${typeof connector})`)
Expand Down
10 changes: 10 additions & 0 deletions src/cow-react/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ export type Writeable<T> = { -readonly [P in keyof T]: T[P] }

export type Nullish<T> = T | null | undefined

/*
A generic class type.

You can use this to refer to a class and not an instance of it.
More info at: https://www.typescriptlang.org/docs/handbook/2/generics.html#using-class-types-in-generics
*/
export type Newable<T extends new (...args: any) => any> = {
new (...args: ConstructorParameters<T>): InstanceType<T>
}
tukantje marked this conversation as resolved.
Show resolved Hide resolved

// This is for Pixel tracking injected code
declare global {
interface Window {
Expand Down
152 changes: 134 additions & 18 deletions src/custom/constants/networks.ts
Original file line number Diff line number Diff line change
@@ -1,30 +1,146 @@
import { JsonRpcProvider } from '@ethersproject/providers'
import {
Network,
Web3Provider,
JsonRpcProvider,
UrlJsonRpcProvider,
StaticJsonRpcProvider,
InfuraProvider,
CloudflareProvider,
AlchemyProvider,
PocketProvider,
AnkrProvider,
} from '@ethersproject/providers'
import * as Sentry from '@sentry/react'
Copy link
Contributor

Choose a reason for hiding this comment

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

Fror this networks file, i think we should move it to cow-react maybe wait until my refactor is done. When we "refactor/centralise" sth, is good we remove it from custom and put it in the right place of cow-react

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a good point - I'll update in place for now and wait for your refactor to be finished to move them around.


import { SupportedChainId } from '@cowprotocol/cow-sdk'
import { Newable } from '@cow/types'

const INFURA_KEY = process.env.REACT_APP_INFURA_KEY
if (typeof INFURA_KEY === 'undefined') {
throw new Error(`REACT_APP_INFURA_KEY must be a defined environment variable`)
}

export const MAINNET_PROVIDER = new JsonRpcProvider(`https://mainnet.infura.io/v3/${INFURA_KEY}`)
interface RpcConfig extends Network {
// You should only need to provide this if you are trying to connect to a custom RPC / network.
readonly rpcUrl?: string
}

const STATIC_RPC_URLS: Partial<Record<SupportedChainId, string>> = {
[SupportedChainId.GNOSIS_CHAIN]: 'https://rpc.gnosis.gateway.fm',
}

export const RPC_CONFIG: Record<SupportedChainId, RpcConfig> = {
[SupportedChainId.MAINNET]: {
chainId: SupportedChainId.MAINNET,
name: 'homestead',
alfetopito marked this conversation as resolved.
Show resolved Hide resolved
},
[SupportedChainId.GOERLI]: {
chainId: SupportedChainId.GOERLI,
name: 'goerli',
},
[SupportedChainId.GNOSIS_CHAIN]: {
chainId: SupportedChainId.GNOSIS_CHAIN,
name: 'gnosis',
rpcUrl: STATIC_RPC_URLS[SupportedChainId.GNOSIS_CHAIN],
},
}

/**
* These are the network URLs used by the interface when there is not another available source of chain data
* We use this class for type inference.
*
* UrlJsonRpcProvider is an abstract class, therefore we have trouble inferring the type of its constructor.
Copy link
Contributor

Choose a reason for hiding this comment

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

we have trouble or can't?

i would think if it's abstract, we don't know the constructor of the children. Is this what you are refering to?

Copy link
Contributor Author

@tukantje tukantje Apr 14, 2023

Choose a reason for hiding this comment

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

I wrote it with my downton abbey glasses apparently 😅 Will rephrase.

* However, as all of the RPC providers we use extend UrlJsonRpcProvider, we can use this class to infer the type of the constructor, by turning it into a concrete one.
*/
export const RPC_URLS: { [key in SupportedChainId]: string } = {
[SupportedChainId.MAINNET]: `https://mainnet.infura.io/v3/${INFURA_KEY}`,
// [SupportedChainId.RINKEBY]: `https://rinkeby.infura.io/v3/${INFURA_KEY}`,
// [SupportedChainId.ROPSTEN]: `https://ropsten.infura.io/v3/${INFURA_KEY}`,
[SupportedChainId.GOERLI]: `https://goerli.infura.io/v3/${INFURA_KEY}`,
// [SupportedChainId.KOVAN]: `https://kovan.infura.io/v3/${INFURA_KEY}`,
// [SupportedChainId.OPTIMISM]: `https://optimism-mainnet.infura.io/v3/${INFURA_KEY}`,
// [SupportedChainId.OPTIMISTIC_KOVAN]: `https://optimism-kovan.infura.io/v3/${INFURA_KEY}`,
// [SupportedChainId.ARBITRUM_ONE]: `https://arbitrum-mainnet.infura.io/v3/${INFURA_KEY}`,
// [SupportedChainId.ARBITRUM_RINKEBY]: `https://arbitrum-rinkeby.infura.io/v3/${INFURA_KEY}`,
// [SupportedChainId.POLYGON]: `https://polygon-mainnet.infura.io/v3/${INFURA_KEY}`,
// [SupportedChainId.POLYGON_MUMBAI]: `https://polygon-mumbai.infura.io/v3/${INFURA_KEY}`,
// [SupportedChainId.CELO]: `https://forno.celo.org`,
// [SupportedChainId.CELO_ALFAJORES]: `https://alfajores-forno.celo-testnet.org`,
[SupportedChainId.GNOSIS_CHAIN]: `https://rpc.gnosis.gateway.fm`,
class BaseUrlJsonRpcProvider extends UrlJsonRpcProvider {}
type ProviderClass = Newable<typeof BaseUrlJsonRpcProvider>

// List of public providers that don't require an API key, per chain support.
const PUBLIC_PROVIDER_LIST: Record<SupportedChainId, ProviderClass[]> = {
[SupportedChainId.MAINNET]: [InfuraProvider, CloudflareProvider, AlchemyProvider, PocketProvider, AnkrProvider],
[SupportedChainId.GOERLI]: [InfuraProvider, AlchemyProvider, PocketProvider, AnkrProvider],
[SupportedChainId.GNOSIS_CHAIN]: [],
}

function getProvider(chainId: SupportedChainId): JsonRpcProvider[] {
const result: JsonRpcProvider[] = []

if (window.ethereum) {
result.push(new Web3Provider(window.ethereum, RPC_CONFIG[chainId]))
}

if (typeof RPC_CONFIG[chainId].rpcUrl === 'string') {
result.push(new StaticJsonRpcProvider(RPC_CONFIG[chainId].rpcUrl))
} else {
result.push(new InfuraProvider(RPC_CONFIG[chainId], INFURA_KEY))
}

if (result.length > 0) {
return result
}

for (const Provider of PUBLIC_PROVIDER_LIST[chainId]) {
try {
const provider = new Provider(RPC_CONFIG[chainId])

result.push(provider)
} catch (error) {
const { sentryError, tags } = constructSentryError(error, { message: "Couldn't create public provider" })

Sentry.captureException(sentryError, { tags })
}
}

return result
}

export const PROVIDERS: Record<SupportedChainId, JsonRpcProvider[]> = {
[SupportedChainId.MAINNET]: getProvider(SupportedChainId.MAINNET),
[SupportedChainId.GOERLI]: getProvider(SupportedChainId.GOERLI),
[SupportedChainId.GNOSIS_CHAIN]: getProvider(SupportedChainId.GNOSIS_CHAIN),
}

export const getRpcUrls = (chainId: SupportedChainId) => PROVIDERS[chainId].map((provider) => provider.connection.url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exporting this, plus the consts? I would think one thing or the other, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exports URLs specifically instead of the provider itself. So for example, when you are using the public provider, we can give you the URL. It is there to support wallets that expect a URL instead of a provider instance, while still allowing for a public provider to be injected.


export const PROVIDERS_RPC_URLS: Record<SupportedChainId, string[]> = {
[SupportedChainId.MAINNET]: getRpcUrls(SupportedChainId.MAINNET),
[SupportedChainId.GOERLI]: getRpcUrls(SupportedChainId.GOERLI),
[SupportedChainId.GNOSIS_CHAIN]: getRpcUrls(SupportedChainId.GNOSIS_CHAIN),
}

export const PROVIDERS_RPC_URLS_FIRST_ONLY: Record<SupportedChainId, string> = {
[SupportedChainId.MAINNET]: PROVIDERS_RPC_URLS[SupportedChainId.MAINNET][0],
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this PROVIDERS and PROVIDERS_RPC_URLS is a bit repetitive. Considere using an IIFE returning the 3 consts, the function would just reduce the list of supported chains

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this, right? we already have PROVIDERS?

[SupportedChainId.GOERLI]: PROVIDERS_RPC_URLS[SupportedChainId.GOERLI][0],
[SupportedChainId.GNOSIS_CHAIN]: PROVIDERS_RPC_URLS[SupportedChainId.GNOSIS_CHAIN][0],
}

export const providers = {
Copy link
Contributor

Choose a reason for hiding this comment

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

why this providers are lowercase?

get mainnet() {
const _providers = getProvider(SupportedChainId.MAINNET)
Copy link
Contributor

Choose a reason for hiding this comment

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

you have already a const where you pre-calculated this, why not using it? PROVIDERS[SupportedChainId.MAINNET]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it, good point.


if (_providers.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn´t this strange to control a casae where we didn´t add the provider for mainnet. Since we do this in a static function, why do we need to do it?

Copy link
Contributor

Choose a reason for hiding this comment

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

you actually did sth similar for the URLs in coinbase, and we asusme there is at list one URL (i feel is a similar case, but you did sth different)

return _providers[0]
} else {
const message = 'No provider found for MAINNET_PROVIDER'
const { sentryError, tags } = constructSentryError(new Error(), {
message,
})

Sentry.captureException(sentryError, { tags })

throw new Error(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you create a sentry error, but throw another error? sentryError also has the message, wouldn't be simpler to just throw that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this to use the same instance, good point.

}
},
}

function constructSentryError(baseError: unknown, { message }: { message: string }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: potencially can be abstracted into something more generic

Copy link
Contributor

@anxolin anxolin Apr 14, 2023

Choose a reason for hiding this comment

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

maybe some JS doc would be good here, like what is this for. Would this belong more in some common sentry utils or sth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some comments. I think we can abstract it, but I've stolen it from another part in code; so would love to handle that as a follow up to not mix up refactoring those parts with this change.

const constructedError = Object.assign(new Error(), baseError, {
message,
name: 'ProviderError',
})

const tags = {
errorType: 'provider',
}

return { baseError, sentryError: constructedError, tags }
Copy link
Contributor

Choose a reason for hiding this comment

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

why returning also the baseError that is not modified and part of the params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it, good point.

}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { useWalletInfo } from '@cow/modules/wallet'
import { nanoid } from '@reduxjs/toolkit'
import { TokenList } from '@uniswap/token-lists'
import { MAINNET_PROVIDER } from 'constants/networks'
import { providers } from 'constants/networks'
import getTokenList from 'lib/hooks/useTokenList/fetchTokenList'
import resolveENSContentHash from 'lib/utils/resolveENSContentHash'
import { useCallback } from 'react'
Expand All @@ -21,7 +21,7 @@ export function useFetchListCallback(): (listUrl: string, sendDispatch?: boolean
const requestId = nanoid()
// Mod: add chainId
sendDispatch && dispatch(fetchTokenList.pending({ requestId, url: listUrl, chainId }))
return getTokenList(listUrl, (ensName: string) => resolveENSContentHash(ensName, MAINNET_PROVIDER))
return getTokenList(listUrl, (ensName: string) => resolveENSContentHash(ensName, providers.mainnet))
.then((tokenList) => {
// Mod: add chainId
sendDispatch && dispatch(fetchTokenList.fulfilled({ url: listUrl, tokenList, requestId, chainId }))
Expand Down
4 changes: 2 additions & 2 deletions src/hooks/useFetchListCallback.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { nanoid } from '@reduxjs/toolkit'
import { TokenList } from '@uniswap/token-lists'
import { MAINNET_PROVIDER } from 'constants/networks'
import { providers } from 'constants/networks'
import getTokenList from 'lib/hooks/useTokenList/fetchTokenList'
import resolveENSContentHash from 'lib/utils/resolveENSContentHash'
import { useCallback } from 'react'
Expand All @@ -16,7 +16,7 @@ export function useFetchListCallback(): (listUrl: string, sendDispatch?: boolean
async (listUrl: string, sendDispatch = true) => {
const requestId = nanoid()
sendDispatch && dispatch(fetchTokenList.pending({ requestId, url: listUrl }))
return getTokenList(listUrl, (ensName: string) => resolveENSContentHash(ensName, MAINNET_PROVIDER))
return getTokenList(listUrl, (ensName: string) => resolveENSContentHash(ensName, providers.mainnet))
.then((tokenList) => {
sendDispatch && dispatch(fetchTokenList.fulfilled({ url: listUrl, tokenList, requestId }))
return tokenList
Expand Down
6 changes: 3 additions & 3 deletions src/state/routing/slice.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { BaseProvider, JsonRpcProvider } from '@ethersproject/providers'
import { BaseProvider, Web3Provider } from '@ethersproject/providers'
import { createApi, fetchBaseQuery, FetchBaseQueryError } from '@reduxjs/toolkit/query/react'
import { Protocol } from '@uniswap/router-sdk'
import { ChainId } from '@uniswap/smart-order-router'
import { RPC_URLS } from 'constants/networks'
import { PROVIDERS } from 'constants/networks'
import { getClientSideQuote, toSupportedChainId } from 'lib/hooks/routing/clientSideSmartOrderRouter'
import ms from 'ms.macro'
import qs from 'qs'
Expand All @@ -16,7 +16,7 @@ function getRouterProvider(chainId: ChainId): BaseProvider {

const supportedChainId = toSupportedChainId(chainId)
if (supportedChainId) {
const provider = new JsonRpcProvider(RPC_URLS[supportedChainId])
const provider = PROVIDERS[supportedChainId]
routerProviders.set(chainId, provider)
return provider
}
Expand Down