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

EigenDA Proxy + Historical Nitro Changes #11

Closed
wants to merge 18 commits into from

Conversation

afkbyte
Copy link

@afkbyte afkbyte commented Jun 8, 2024

No description provided.

@afkbyte afkbyte marked this pull request as ready for review June 10, 2024 14:41
) ([]byte, error) {
// we start from the 41st byte of sequencerMsg because bytes 0 - 40 are the header, and 40 - 41 is the eigenDA header flag
// we use the binary domain here because this is what we use in the derivation pipeline
return eigenda.RecoverPayloadFromEigenDABatch(ctx, sequencerMsg[41:], e.eigenDAReader, preimages, "binary")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Knit - domain could be enum

Comment on lines 883 to +888
if use4844 {
methodName = sequencerBatchPostWithBlobsMethodName
}
if useEigenDA {
methodName = sequencerBatchPostWithEigendaMethodName
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

knit - could be useful enforce that use488 and useEigenDA are never both true

Copy link
Author

Choose a reason for hiding this comment

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

there are cases where we would want both flags to be true to be able to specify between using 4844 as fallback or calldata imo

@@ -152,16 +152,17 @@ func (r *BlobPreimageReader) Initialize(ctx context.Context) error {

// struct for recovering data from preimage, impl interface EigenDAReader

func (dasReader *PreimageEigenDAReader) QueryBlob(ctx context.Context, ref *eigenda.EigenDARef) ([]byte, error) {
dataPointer, err := ref.Serialize()
func (dasReader *PreimageEigenDAReader) QueryBlob(ctx context.Context, cert *eigenda.EigenDABlobInfo, domain string) ([]byte, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

knit - we use blobInfo and cert for the same variable reference throughout. Might be useful to converge on a single naming scheme to disambiguate abstraction

Copy link
Author

Choose a reason for hiding this comment

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

I prefer blobInfo because it's more meaningful in the context of EigenDA, but i know cert is the term used in the eigenda-proxy - would love @teddyknox to weigh in here

eigenda/eigenda.go Show resolved Hide resolved
Comment on lines +222 to +226
data, err := daReader.QueryBlob(ctx, blobInfo, domain)
if err != nil {
log.Error("Failed to query data from EigenDA", "err", err)
return nil, err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we be verifying the data after its been read?

Copy link
Author

@afkbyte afkbyte Jun 12, 2024

Choose a reason for hiding this comment

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

I believe theeigenda-proxy does the verification for us now, should we still include a second verification in when querying the blob?


abi, err := abi.JSON(strings.NewReader(sequencerInboxABI))
if err != nil {
panic(err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets remove the use of panic and return errors

eigenda/eigenda.go Outdated Show resolved Hide resolved
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/rlp"
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we not just import the existing proxy client?

Copy link
Author

Choose a reason for hiding this comment

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

unfortunately we can't import the existing proxy client as there's a bit too much dependency hell for us to resolve because of differing go version requirements and conflicting go-ethereum versions across nitro, optimism, and the golang eigensdk

@teddyknox teddyknox force-pushed the afk/new-eigenproxy-integration branch from 0a3cb87 to c5d1db9 Compare June 14, 2024 14:53
@epociask epociask closed this Jul 2, 2024
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.

3 participants