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

tukantje · 2023-04-05T23:32:29Z

Current Situation

We are currently relying on a JsonRpcProvider, in every instance, via Infura.

What

This PR includes a couple of proposals. Code quality can definitely be improved, this iteration is mainly for discussion of the ideas.

Use RPC providers only when we cannot setup a provider based off of window.ethereum.
- This would allow us to not hit infura, for said users. Might be beneficial.
- Due to reduced calls, our usage of infura would go down significantly.
- Certain calls are "free" when done in this fashion - such as eth_chainId (Based on ethers.js discussion here eth_chainId is called every time the provider is used ethers-io/ethers.js#901)
Use InfuraProvider instead of JsonRpcProvider for infura endpoints.
- ethers.js claims this would include optimisations on their end, specifically for infura.
Use StaticJsonRpcProvider instead of JsonRpcProvider for non-infura cases.
- The difference is essentially making the eth_chainId call.
- This is useful for when the chainId could change on the fly. However, the way we use these endpoints, it does seem like that is not a possibility (i.e. we create a provider for Gnosis Chain, specifically).

Why

Main point for this was trying to figure out amount of calls we are making to infura, as it has been a recent source of pain. During this process, I've noticed that eth_chainId and eth_blockNumber are the highest hit calls, main reason being this information needing to be polled. Specifically,

Every 10-15 secs, we poll for eth_blockNumber.
- This is in line with what Infura suggests.
Every call we make, we verify eth_chainId.
- StaticJsonRpcProvider would stop making this call, albeit definitely should be discussed.
- Relying on wallets instead of RPC would make this a cost-free call.
- Main reason this is being called all the time is due to EIP-155.

Which made me also think about if we could reduce our reliance in general. A recent Slack discussion (https://cowservices.slack.com/archives/C036G0J90BU/p1679062770503109) was related to this topic, as such I've started exploring this PR.

Important Points

I've mainly tested this out as;

One browser with Metamask
One browser without any wallet

After which I have mainly tested;

Can I load the application?
Can I connect using Wallet Connect?
Can I send a transaction?

Which seems to be working, however this is nowhere near enough. I presume, if we desire such a change, we should;

Test all wallets
Think about a list of functionality that must pass instead of what I could think about randomly

Thank you, and looking forward to hearing what everybody thinks 🐮

vercel · 2023-04-05T23:32:34Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
swap-dev	✅ Ready (Inspect)	Visit Preview	💬 Add feedback

🌃 Cosmos ↗︎

alfetopito · 2023-04-06T10:14:41Z

The test did not go so well for me
Latest FF with a bunch of tracker blockers on - which may or may not be the root cause - the behaviour is the same as when the infura key was blocked. The page does not fully load

On the console, there are various failures:

On Chrome though, where I have no blockers enabled, the page loads

As you mentioned, changing the network in the app while not connected is not possible, the app breaks

While connected using WalletConnect, it works fine, balances load.

Have not tried to submit any tx to test, though.

In summary, if the connection without a wallet can be fixed, we should ship it

tukantje · 2023-04-12T23:38:30Z

@alfetopito thank you for the review!

So multiple things are at play here from what I can see;

Application doesn't open
Most likely this is due to not being on the same default chain (i.e. your wallet was on Goerli / Gnosis and not Mainnet).
WalletConnect does work
WC apparently expects RPC URLs to be given to it.

I've updated the PR - apologies for the size of it.

What I've addressed here are;

Try to centralise where RPC configuration is coming from
- I think this part is not done yet, however it should be an improvement. Not having a centralised setup makes things quite confusing, and tough to debug (i.e. "why am I sending a call to infura when I set this to alchemy? oh wait in this file I also have an infura link")
Return a list of RPC providers instead of singular ones, to have a fallback style handling
- Initially I've started with creating a custom provider to handle all these cases. However, after quite a bit of research and pain, I've noticed that suggested approach on this is to handle it at a higher level than ethers (i.e. web3-react). As such, I've tried to rely on their functionality on this as much as possible.
- web3-react does allow you to give multiple providers, and use them in order.
Add public providers as a last resort
Add sentry logging
Remove the assumption of user arriving with mainnet selected on their wallet
- This was not an issue earlier due to chainId checks galore.

What do you think?

src/custom/constants/networks.ts

alfetopito

A couple of questions in the code, but overall it looks quite good.
The explanations help understand what's going on.

As for the behaviour, briefly tested on FF with multiple networks while disconnected, connected with metamask and via wallet connect.
It all worked great 👍

src/cow-react/modules/wallet/web3-react/connection/network.tsx

src/custom/constants/networks.ts

src/cow-react/modules/wallet/web3-react/connection/coinbase.tsx

src/cow-react/modules/wallet/web3-react/connection/network.tsx

src/custom/constants/networks.ts

anxolin · 2023-04-14T10:35:08Z

src/custom/constants/networks.ts

+}
+
+export const PROVIDERS_RPC_URLS_FIRST_ONLY: Record<SupportedChainId, string> = {
+  [SupportedChainId.MAINNET]: PROVIDERS_RPC_URLS[SupportedChainId.MAINNET][0],


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

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

anxolin · 2023-04-14T10:36:31Z

src/cow-react/modules/wallet/web3-react/connection/ledger.tsx

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


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

anxolin · 2023-04-14T10:36:56Z

src/cow-react/modules/wallet/web3-react/connection/network.tsx

 import { ConnectionType } from '@cow/modules/wallet'
 import { Web3ReactConnection } from '../types'
+import { PROVIDERS } from '@src/custom/constants/networks'


drop the @src/custom part

anxolin · 2023-04-14T10:39:19Z

src/custom/constants/networks.ts

+  PocketProvider,
+  AnkrProvider,
+} from '@ethersproject/providers'
+import * as Sentry from '@sentry/react'


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

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

anxolin · 2023-04-14T10:40:56Z

src/custom/constants/networks.ts

- * 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.


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?

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

anxolin · 2023-04-14T10:42:06Z

src/custom/constants/networks.ts

+  [SupportedChainId.GNOSIS_CHAIN]: getProvider(SupportedChainId.GNOSIS_CHAIN),
+}
+
+export const getRpcUrls = (chainId: SupportedChainId) => PROVIDERS[chainId].map((provider) => provider.connection.url)


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

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.

anxolin · 2023-04-14T10:43:13Z

src/custom/constants/networks.ts

+  },
+}
+
+function constructSentryError(baseError: unknown, { message }: { message: string }) {


nit: potencially can be abstracted into something more generic

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

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.

anxolin · 2023-04-14T10:45:45Z

src/custom/constants/networks.ts

+    errorType: 'provider',
+  }
+
+  return { baseError, sentryError: constructedError, tags }


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

Removed it, good point.

anxolin · 2023-04-14T10:47:26Z

src/custom/constants/networks.ts

+
+      Sentry.captureException(sentryError, { tags })
+
+      throw new Error(message)


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

Updated this to use the same instance, good point.

anxolin · 2023-04-14T10:49:09Z

src/custom/constants/networks.ts

+
+export const providers = {
+  get mainnet() {
+    const _providers = getProvider(SupportedChainId.MAINNET)


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

Updated it, good point.

src/cow-react/types.ts

anxolin · 2023-04-14T10:55:26Z

src/custom/constants/networks.ts

+  [SupportedChainId.GNOSIS_CHAIN]: PROVIDERS_RPC_URLS[SupportedChainId.GNOSIS_CHAIN][0],
+}
+
+export const providers = {


why this providers are lowercase?

anxolin · 2023-04-14T10:57:59Z

src/custom/constants/networks.ts

+  get mainnet() {
+    const _providers = getProvider(SupportedChainId.MAINNET)
+
+    if (_providers.length > 0) {


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?

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)

elena-zh · 2023-07-25T15:08:29Z

Should we still keep it opened?

Reduce usage of RPC providers via relying on them sparingly

7f991b4

tukantje self-assigned this Apr 5, 2023

Update logic to provide a list of providers, instead of singular URLs

23f47fa

vercel bot deployed to Preview April 12, 2023 23:27 View deployment

tukantje commented Apr 13, 2023

View reviewed changes

src/custom/constants/networks.ts Outdated Show resolved Hide resolved

tukantje requested a review from a team April 13, 2023 09:41

alfetopito approved these changes Apr 13, 2023

View reviewed changes

shoom3301 reviewed Apr 13, 2023

View reviewed changes

src/cow-react/modules/wallet/web3-react/connection/network.tsx Show resolved Hide resolved

shoom3301 reviewed Apr 13, 2023

View reviewed changes

src/custom/constants/networks.ts Outdated Show resolved Hide resolved

tukantje added 2 commits April 13, 2023 14:28

Simplify RPC_URLS implementation

1c4ae74

Delete debug line

0d54f9b

vercel bot deployed to Preview April 13, 2023 12:38 View deployment

tukantje changed the title ~~Draft: Reduce usage of RPC providers via relying on them sparingly~~ Reduce usage of RPC providers via relying on them sparingly Apr 13, 2023

tukantje requested a review from anxolin April 13, 2023 17:26

Remove getSigner injection

e8fa9f1

vercel bot deployed to Preview April 14, 2023 09:48 View deployment

shoom3301 approved these changes Apr 14, 2023

View reviewed changes

anxolin reviewed Apr 14, 2023

View reviewed changes

src/cow-react/types.ts Show resolved Hide resolved

anxolin reviewed Apr 14, 2023

View reviewed changes

shoom3301 closed this Aug 15, 2023

github-actions bot locked and limited conversation to collaborators Aug 15, 2023

alfetopito deleted the refactor/use-rpc-providers-only-when-needed branch August 15, 2023 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tukantje commented Apr 5, 2023 •

edited

Loading

vercel bot commented Apr 5, 2023 •

edited by github-actions bot

Loading

alfetopito commented Apr 6, 2023

tukantje commented Apr 12, 2023 •

edited

Loading

alfetopito left a comment

anxolin Apr 14, 2023

anxolin Apr 14, 2023

anxolin Apr 14, 2023

anxolin Apr 14, 2023

tukantje Apr 14, 2023

anxolin Apr 14, 2023

tukantje Apr 14, 2023

anxolin Apr 14, 2023

tukantje Apr 14, 2023 •

edited

Loading

anxolin Apr 14, 2023

tukantje Apr 14, 2023

anxolin Apr 14, 2023

anxolin Apr 14, 2023 •

edited

Loading

tukantje Apr 14, 2023

anxolin Apr 14, 2023

tukantje Apr 14, 2023

anxolin Apr 14, 2023

tukantje Apr 14, 2023

anxolin Apr 14, 2023

tukantje Apr 14, 2023

anxolin Apr 14, 2023

anxolin Apr 14, 2023

anxolin Apr 14, 2023

elena-zh commented Jul 25, 2023


		Sentry.captureException(sentryError, { tags })

		throw new Error(message)

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

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

Conversation

tukantje commented Apr 5, 2023 • edited Loading

Current Situation

What

Why

Important Points

vercel bot commented Apr 5, 2023 • edited by github-actions bot Loading

alfetopito commented Apr 6, 2023

tukantje commented Apr 12, 2023 • edited Loading

alfetopito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tukantje Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anxolin Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elena-zh commented Jul 25, 2023

tukantje commented Apr 5, 2023 •

edited

Loading

vercel bot commented Apr 5, 2023 •

edited by github-actions bot

Loading

tukantje commented Apr 12, 2023 •

edited

Loading

tukantje Apr 14, 2023 •

edited

Loading

anxolin Apr 14, 2023 •

edited

Loading