-
Notifications
You must be signed in to change notification settings - Fork 29
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
Use High-Level EigenDA Client #23
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.
Mostly knits and documentation stuff - otherwise code LGTM
@@ -30,12 +31,19 @@ func (e EigenDAStore) Get(ctx context.Context, key []byte) ([]byte, error) { | |||
if err != nil { | |||
return nil, fmt.Errorf("failed to decode DA cert to RLP format: %w", err) | |||
} | |||
blob, err := e.client.RetrieveBlob(ctx, cert.BatchHeaderHash, cert.BlobIndex) | |||
blob, err := e.client.GetBlob(ctx, cert.BatchHeaderHash, cert.BlobIndex) |
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.
I feel like there are still cases where we would want to use the disperser client rather than the high level to retrieve a blob for backwards compatibility but not sure if this is something we should address in this PR.
Say someone already running an chain with an integration using the disperser client upgrades to one using this proxy server, if they wanted to retrieve and validate a historical batch we would error out.
we can also probably handle this in the integration itself -> if a Get from the Proxy Server fails then we retry with the old disperser interface.
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.
I'm leaning towards handling it in the proxy server because the backwards compatibility handling is something we'd need to duplicate across integrations if we don't handle it here
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 agreed. Right now I don't think we have any backwards compatibility concerns but in the future we should build backwards compatibility into this.
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
No description provided.