-
Notifications
You must be signed in to change notification settings - Fork 176
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
Make built-in indexer work for retrieval client #110
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you summarize the issues that this is fixing?
clients/retrieval_client.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
err = indexedState.Start(context.Background()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to Start
the indexedState from the retrieval client? Currently, it's left as the responsibility of caller to provide an indexedState that's actively indexing.
Even if we do, I don't think this should be called at initialization. If we need to explicitly start a goroutine, it should be separated out as an explicit method
core/indexer/indexer.go
Outdated
"github.com/ethereum/go-ethereum/common" | ||
) | ||
|
||
func SetupNewIndexer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it setup or actually CreateNewIndexer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually is a setup of a new Indexer, because create a new indexer is done by https://github.com/wmb-software-consulting/eigenda/blob/56a730c0b9f383e45f9c72b3945796495cc047f3/indexer/indexer.go#L50
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this function is call that indexer.New(), so this will be better named as CreateNewIndexer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
core/indexer/indexer.go
Outdated
"github.com/ethereum/go-ethereum/common" | ||
) | ||
|
||
func SetupNewIndexer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this function is call that indexer.New(), so this will be better named as CreateNewIndexer
.
Why are these changes needed?
Context issue:
Solution in this PR:
Checks