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

fix: prevent exhausting failover providers on erc165 reverts #806

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eli-lim
Copy link

@eli-lim eli-lim commented Oct 29, 2024

Background

  1. I noticed slowness when checking my degree on the site - took about 4 - 5s to verify
  2. Looking at the networks tab, I noticed it was making a bunch of requests to different public node providers in sequence
SR-compressed.mp4

Root cause

  1. oa-verify is checking if the documentStore supports some interface, compliant with the ERC165 spec.
  2. When the rpc call returns execution reverted, ethers will throw an error. oa-verify mitigates this with a try-catch, returning false if an error is thrown.
  3. Unfortunately, website's OAFailoverProvider catches this deeper in the call stack (rendering the try-catch in (2) moot), and moves on to the next provider, until the list of providers is exhausted.

TLDR:

async function isBatchableDocumentStore(): boolean {
  try {
    // this is expected to throw if erc165 is not implemented, but it keeps retrying instead
    return await OAFailoverProvider.perform('call', { contract, erc165Check }) as boolean; 
  } catch (e) {
    return false;
  } 
}

Fix

  1. (This PR) Stop the OAFailoverProvider from overzealously retrying on erc165 reverts. It can also be made more generic, to stop retrying on any call exception.
  2. (Optionally) switch to use public node providers with lower latency

    Ignoring other concerns like privacy / agreements with the existing node providers.

Doing these:

Screen.Recording.2024-10-30.at.2.36.18.AM.mov

Note

An alternative is to change oa-verify to use a direct rpc call, to bypass ethers's throw-on-revert

Copy link
Member

@HJunyuan HJunyuan left a comment

Choose a reason for hiding this comment

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

@eli-lim Thank you for proposing these improvements!

I've made a few minor suggestions before we proceed to merge this PR in.

src/services/failover-provider.ts Outdated Show resolved Hide resolved
@@ -52,6 +52,11 @@ export class OAFailoverProvider extends providers.StaticJsonRpcProvider {
try {
return await this.failoverProviders[i].perform(method, params);
} catch (error) {
// Don't retry if it's just a revert from an ERC165 check
if (this.isErc165Check(method, params, error)) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return false;
throw error;

Copy link
Author

Choose a reason for hiding this comment

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

actually the point of this is to not throw on call exceptions

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

Successfully merging this pull request may close these issues.

2 participants