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

node: Refactor config to accept Firehose provider #2647

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

maoueh
Copy link
Contributor

@maoueh maoueh commented Jul 22, 2021

(and made Provide a little bit more extensible along the way)

There is a backward compatible handling of config that at some point should be converted to the new syntax form. The new syntax form is

[chains]
[chains.ropsten]
provider = [
  { label = "firehose", details = { type = "web3", transport = "rpc", url = "https://ropsten.sf.io/rpc", features = [] } },
]

While a Firehose provider now looks like:

[chains]
[chains.ropsten]
provider = [
  { label = "firehose", details = { type = "firehose", url = "https://ropsten.streamingfast.io" } },
]

This is for now undocumented and is first step towards Firehose integration in the codebase directly.

… a little bit more extensible)

There is a backward compatible handling of config that at some point should be converted to the new syntax form. The new syntax form is

```
{ label = "node0", details = { type = "web3", transport = "rpc", url = "...", features = [...] } }
```

While a Firehose provider now looks like:

```
{ label = "firehose0", details = { type = "firehose", url = "..." } }
```

This is for now undocumented and is first step towards Firehose integration in the codebase directly.
@maoueh
Copy link
Contributor Author

maoueh commented Aug 2, 2021

@otaviopace @tilacog When you have a few spare minutes, you like a review here. Feel free to ping someone else with more knowledge of this part if required.

@leoyvens leoyvens requested a review from lutter August 2, 2021 16:22
@leoyvens
Copy link
Collaborator

leoyvens commented Aug 2, 2021

I think @lutter is the ideal person to review this.

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.

Very nice! I really like that it's backwards compatible, and that the tests are pretty extensive.

@maoueh
Copy link
Contributor Author

maoueh commented Aug 10, 2021

Excellent! Can we have it merged now hehe :0

@evaporei
Copy link
Contributor

@maoueh the way we usually do is that once a PR is approved, we let for the author to merge it whenever is better for them 🙂

The cases we want/need to wait for something else to go first or depend on another thing (maybe running the integration tests on that branch), people usually warn in the approve comment.

I would just double check if it's fine with @leoyvens in this case because I remember him mentioning that for one of your PRs he was waiting on something related to the tokio one to review or merge (I'm not quite sure honestly).

@leoyvens
Copy link
Collaborator

@otaviopace the thing is @maoueh doesn't have merge permissions. But it's a good idea give #2679 priority just in case there are any conflicts.

@maoueh
Copy link
Contributor Author

maoueh commented Aug 10, 2021

Totally independent from #2679, anyway, I'm pushing the merge conflicts into @jubeless hands :D

@maoueh
Copy link
Contributor Author

maoueh commented Aug 10, 2021

@otaviopace It's good, we got access now, so I'm able to approve now.

@maoueh maoueh merged commit 1415d5b into graphprotocol:master Aug 10, 2021
@maoueh maoueh deleted the feature/firehose-config branch August 10, 2021 19:35
leoyvens added a commit that referenced this pull request Aug 11, 2021
…Provider a little bit more extensible) (#2647)"

This reverts commit 1415d5b
due to backwards compatibility issues.
leoyvens added a commit that referenced this pull request Aug 11, 2021
…Provider a little bit more extensible) (#2647)"

This reverts commit 1415d5b
due to backwards compatibility issues.
maoueh added a commit to streamingfast/graph-node that referenced this pull request Aug 11, 2021
… a little bit more extensible) (graphprotocol#2647)

There is a backward compatible handling of config that at some point should be converted to the new syntax form. The new syntax form is

```
{ label = "node0", details = { type = "web3", transport = "rpc", url = "...", features = [...] } }
```

While a Firehose provider now looks like:

```
{ label = "firehose0", details = { type = "firehose", url = "..." } }
```

This is for now undocumented and is first step towards Firehose integration in the codebase directly.
maoueh added a commit that referenced this pull request Aug 16, 2021
… a little bit more extensible) (#2647) (#2698)

There is a backward compatible handling of config that at some point should be converted to the new syntax form. The new syntax form is

```
{ label = "node0", details = { type = "web3", transport = "rpc", url = "...", features = [...] } }
```

While a Firehose provider now looks like:

```
{ label = "firehose0", details = { type = "firehose", url = "..." } }
```

This is for now undocumented and is first step towards Firehose integration in the codebase directly.
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.

4 participants