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 limit handling on rpc configuration #4353

Merged
merged 1 commit into from
Feb 15, 2023
Merged

Conversation

mangas
Copy link
Contributor

@mangas mangas commented Feb 9, 2023

  • Set 0 for Disabled
  • Omitted hosts can are disabled if list of rules is not empty
  • Empty rules = Unlimited
  • Limit != 0 does exactly what you'd think it does

@mangas mangas marked this pull request as ready for review February 9, 2023 18:09
@mangas mangas force-pushed the filipe/fix-rpc-limit branch 4 times, most recently from 426c183 to d5813c8 Compare February 9, 2023 21:10
@mangas mangas requested review from lutter and neysofu February 9, 2023 21:19
@neysofu neysofu self-assigned this Feb 13, 2023
Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

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

LGTM. I am not sure that the SubgraphLimit enum is really needed, but the behavior is now defintiely what we want (based on the unit tests, nice!)

Limit(usize),
NoTraffic,
Unlimited,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need this enum? I don't really think it buys us much above using a plain usize (where Disabled -> 0 and Unlimited -> usize::MAX)

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's mostly to improve readability since otherwise we'd need to set a value for 0 and some for "not set" in order to differentiate so there would be a few places with Option where we were not sure of the meaning. Also Unlimited will also get transformed in the different adapters, eg firehose uses 100 max so it gets set to the adapter-specific "maximum"

Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

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

LGTM.

The file docs/config.md also needs an update: the sentence

Any node whose name does not match one of these patterns will use mainnet-0 and mainnet-1 for an unlimited number of subgraphs.

needs to be changed to say that nodes whose names do not match will not use that provider

None => self.cheapest_with(capabilities.unwrap_or(&NodeCapabilities {
// call_only_adapter can fail if we're out of capcity or,this is fine since
// we would want to fallback onto a full adapter
// so we will ignore this error and return whatever comes out of `cheapest_with`
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 fully understand this comment, especially the 'or, this is fine' throws me off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially had something else there :P I will fix this, good catch :)

- Set 0 for unlimited
- Unset hosts can make no calls
- Limit != 0 does exactly what you'd think it does
@mangas mangas merged commit 00a40b4 into master Feb 15, 2023
@mangas mangas deleted the filipe/fix-rpc-limit branch February 15, 2023 13:07
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.

3 participants