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

feat: add fallback nodes to provider #980

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

piwonskp
Copy link
Contributor

@piwonskp piwonskp commented Feb 23, 2024

Motivation and Resolution

Resolves #872

RPC version (if applicable)

Usage related changes

Development related changes

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

Copy link
Collaborator

@tabaktoni tabaktoni left a comment

Choose a reason for hiding this comment

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

  1. Maybe we can keep nodeUrl, and pass param, if it is singular it would be non-fallback and, if it is an array it would have fallbacks. The logical order would be 0 then 1 ... if there are fallbacks. In that case, it would be the same as before.
  2. I would rather add setNode and getNode on the channel than pre-pending fetch.
    This way you intentionally change channel source rather than hacking into one method.

My initial idea for this was to dedicate to one and switch to the next when it fails, this solution proposes to use all nodes per request without dedication. This seems like an interesting idea and it could also work.

wdyt @penovicp @ivpavici

@ivpavici
Copy link
Collaborator

My initial idea for this was to dedicate to one and switch to the next when it fails

I thought of the same approach as you said here, I think Ethers works like this, but I have to double check.
I think if we want to go with this approach it wouldn't be a big change in this PR

response = await this.fetch(nodes[i], method, params);

if (response.ok) {
this.setPrimaryNode(nodes[i], i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't quite understand why we need this primaryNode here

@tabaktoni
Copy link
Collaborator

Another option is to create n channels for each provider

@tabaktoni
Copy link
Collaborator

TODO: I would like to refactor this approach and will do on the same PR

@tabaktoni tabaktoni added the pending need update label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending need update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for multiple RPC providers
3 participants