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

AVS-215, AVS-259: Add Subgraphs #233

Merged
merged 12 commits into from
May 29, 2024
Merged

AVS-215, AVS-259: Add Subgraphs #233

merged 12 commits into from
May 29, 2024

Conversation

Sidu28
Copy link
Contributor

@Sidu28 Sidu28 commented May 6, 2024

Fixes # .

What Changed?

Adding subgraphs to the services in eigensdk, to replace in memroy data structures.

Reviewer Checklist

  • Code is well-documented
  • Code adheres to Go naming conventions
  • Code deprecates any old functionality before removing it

services/querier.go Outdated Show resolved Hide resolved
@Sidu28 Sidu28 changed the title Add Subgraphs AVS-215: Add Subgraphs May 13, 2024
Sidu28 and others added 5 commits May 14, 2024 08:30
* cleanup

* added optimistic test

* fixed broken test
* cleanup

* added optimistic test

* fixed broken test

* chore: forge init

* forge install: forge-std

v1.8.2

* addeed integration test
@Sidu28
Copy link
Contributor Author

Sidu28 commented May 15, 2024

@samlaf not sure Im too happy with how complicated the integration seem to be to setup. Need to import both eigenlayer-middleware and eigenlayer-middleware-offchain as submodules (I re-imported eigenlayer-middleware because for some reason foundry wont allow me to import it from the contracts/lib folder). I tried to recreate eigenda/inabox's integration test with running exec-commands from the golang test but some of it doesn't seem to be working. Need to figure out if there's a better way to do all of this, probably try to get a docker container with eveyrthing already setup on it or something like that. Any ideas?

@Sidu28 Sidu28 closed this May 15, 2024
@Sidu28 Sidu28 reopened this May 15, 2024
@samlaf
Copy link
Collaborator

samlaf commented May 15, 2024

I re-imported eigenlayer-middleware because for some reason foundry wont allow me to import it from the contracts/lib folder).

wdym by import it? eigensdk use eigenlayer-contracts repo via eigenlayer-middleware submodule (eigensdk->middleware->core), so should be the same here? Maybe just the path needs to be added in remappings.txt (or foundry.toml remapping section)?

I tried to recreate eigenda/inabox's integration test with running exec-commands from the golang test but some of it doesn't seem to be working. Need to figure out if there's a better way to do all of this, probably try to get a docker container with eveyrthing already setup on it or something like that. Any ideas?

Why are you trying to run eigenda's inabox test?
What's the minimal testing setup that you would need here? Just deploy some avs and register an operator? Let's do that instead.
I have #176 that I started to create such an integration framework. Forget what the state of it is though..

@Sidu28 Sidu28 changed the title AVS-215: Add Subgraphs AVS-215, AVS-259: Add Subgraphs May 20, 2024
@Sidu28 Sidu28 requested a review from afkbyte May 21, 2024 23:22
Copy link
Collaborator

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

Generally LGTM. added some syntactic comments

services/operatorsinfo/operatorsinfo_subgraph_test.go Outdated Show resolved Hide resolved
services/operatorsinfo/operatorsinfo_subgraph_test.go Outdated Show resolved Hide resolved
*bn254.G1Affine
}
GraphQLQuerier interface {
Query(ctx context.Context, q any, variables map[string]any) error
Copy link
Collaborator

Choose a reason for hiding this comment

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

are the anys necessary? If so, can you add comments as to how they are being used and what they are supposed to be. Hard to understand what this is used if we throwaway all type information.

Comment on lines 38 to 42
return execCmd("anvil", []string{"--host", "0.0.0.0"}, []string{})
}

func startGraph() (int, error) {
return execCmd("docker", []string{"compose", "up"}, []string{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

start anvil and graph using testcontainers like we do everywhere else. This way don't need to rely on other programs being installed on the user's computer, and also version will always be the same so won't have non-deterministic tests.

}
fmt.Println("Anvil chain started with PID", pid)

curr_path, err := os.Getwd()
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix lint error by checking err != nil

samlaf
samlaf previously approved these changes May 29, 2024
Copy link
Collaborator

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

LGTM. think we can delete the submodules though?

.gitmodules Outdated
Comment on lines 4 to 12
[submodule "services/operatorsinfo/integration_test_deployment/lib/forge-std"]
path = services/operatorsinfo/integration_test_deployment/lib/forge-std
url = https://github.com/foundry-rs/forge-std
[submodule "services/operatorsinfo/integration_test_deployment/lib/eigenlayer-middleware"]
path = services/operatorsinfo/integration_test_deployment/lib/eigenlayer-middleware
url = https://github.com/Layr-Labs/eigenlayer-middleware
[submodule "services/operatorsinfo/integration_test_deployment/lib/eigenlayer-middleware-offchain"]
path = services/operatorsinfo/integration_test_deployment/lib/eigenlayer-middleware-offchain
url = https://github.com/Layr-Labs/eigenlayer-middleware-offchain
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these still needed? don't think they are used anymore after the last commit that deleted a bunch of stuff?

@Sidu28 Sidu28 merged commit d3cc43d into master May 29, 2024
3 checks passed
@Sidu28 Sidu28 deleted the add-graph branch May 29, 2024 14:05
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.

2 participants