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(payment-detection): differenciate decentralized network case #1267

Merged
merged 3 commits into from
Nov 28, 2023

Conversation

alexandre-abrioux
Copy link
Member

@alexandre-abrioux alexandre-abrioux commented Nov 27, 2023

This is a fixup for #1262 and #1265

Context

When using TheGraph decentralized network, different GQL queries can target different indexers. For consistency, we can ask to target only indexers that have processed specific blocks using a block filter.

For more info, see here or here, look for number_gte in these pages.

Description of the change

When we don't have the data for which minimum block an indexer should have processed, we must set a default value. After several unsuccessful tries, here is what I've found regarding generating a default value that works with both a normal TheGraph Node and the decentralized gateway:

Block Filter TheGraph Node TheGraph Gateway (decentralized)
undefined Invalid query: Failed to determine block constraints.
null Invalid query: Failed to determine block constraints.
empty object: {} Invalid value provided for argument block: Object({})
default to zero: { number_gte: 0 } Invalid query: Requested block before minimum startBlock of manifest: 0

This shows that no default value is compatible with both setups. We need to differentiate between both. I've added a new decentralizedNetwork boolean option that auto-populates depending on the URL value and allows to distinguish between both cases.

@coveralls
Copy link

coveralls commented Nov 27, 2023

Coverage Status

coverage: 87.089%. remained the same
when pulling 0a550d0 on fix-payment-detection-decentralized
into 28cf4b6 on master.

Copy link
Contributor

@benjlevesque benjlevesque left a comment

Choose a reason for hiding this comment

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

Wow, great find... Strange that TheGraph doesn't expect the same input on hosted vs Network 🤔 I wonder if this is intentional.

I'd prefer an approach where the complexity related to the Network is kept centralized in the client , and abstracted in the info-retriever, but your solution works just fine! For the record, here's what I suggest, but not a blocker
Great work

@alexandre-abrioux alexandre-abrioux enabled auto-merge (squash) November 28, 2023 16:28
@alexandre-abrioux alexandre-abrioux merged commit 2719883 into master Nov 28, 2023
24 of 25 checks passed
@alexandre-abrioux alexandre-abrioux deleted the fix-payment-detection-decentralized branch November 28, 2023 16:34
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.

None yet

5 participants