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

Refactor code into indexer-common crate #53

Merged
merged 3 commits into from
Sep 21, 2023

Conversation

Jannis
Copy link
Collaborator

@Jannis Jannis commented Sep 18, 2023

This moves out much of the common code into an indexer-common crate. The next step is likely to make more clear how one would go about implementing a different kind of indexer service.

@aasseman
Copy link
Collaborator

This is awesome, I actually just barely started working on this to share components with the "indexer-tap-agent" (or better name if someone feels inspired)

@aasseman aasseman added size:large Large p1 High priority type:refactor Changes not visible to users labels Sep 18, 2023
@Jannis Jannis force-pushed the jannis/crate-refactor branch 3 times, most recently from 7a4faaf to aa73048 Compare September 18, 2023 21:38
@Jannis Jannis changed the title WIP: Refactor code into indexer-common crate Refactor code into indexer-common crate Sep 18, 2023
@Jannis
Copy link
Collaborator Author

Jannis commented Sep 18, 2023

@aasseman Ok, I expect this to pass the checks.

I didn't want to move the escrow monitor just yet. To avoid that, I had to split up the test_vectors. I think the TAP manager tests needed the allocation monitor and so I had to add a mock feature to the indexer-common crate to export the faux code generated for it across for testing. Probably something we can clean up again later.

@aasseman
Copy link
Collaborator

@aasseman Ok, I expect this to pass the checks.

I didn't want to move the escrow monitor just yet. To avoid that, I had to split up the test_vectors. I think the TAP manager tests needed the allocation monitor and so I had to add a mock feature to the indexer-common crate to export the faux code generated for it across for testing. Probably something we can clean up again later.

I agree on all points. The mocking was what slowed me down considerably in my attempt to split things off into a common crate this weekend. I can take a stab at the EscrowMonitor in particular in another PR. I'm the only one depending on this for now so you don't need to struggle with this.

Copy link
Collaborator

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

LGTM except for 2 small things.
Also I think this should wait for #52 since this branch is based on top of it.

common/src/attestations/signers.rs Outdated Show resolved Hide resolved
common/src/attestations/signers.rs Outdated Show resolved Hide resolved
aasseman
aasseman previously approved these changes Sep 19, 2023
Copy link
Collaborator

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

:shipit:

@Jannis Jannis dismissed aasseman’s stale review September 20, 2023 13:27

The merge-base changed after approval.

@Jannis
Copy link
Collaborator Author

Jannis commented Sep 20, 2023

Rebased after #52 was merged. Let's see if the tests still pass.

.json::<Value>()
.await
// FIXME: Don't use expect here
.expect("Failed to parse network subgraph query result"),
Copy link
Collaborator

@hopeyen hopeyen Sep 20, 2023

Choose a reason for hiding this comment

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

could use another bad_request_response?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, good catch. Fixed!

aasseman
aasseman previously approved these changes Sep 20, 2023
@Jannis Jannis merged commit 513d1ad into graphprotocol:main Sep 21, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 High priority size:large Large type:refactor Changes not visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants