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

feat(da): RetrieveBatchesV2 method #937

Draft
wants to merge 45 commits into
base: kirill/interchain-da
Choose a base branch
from

Conversation

keruch
Copy link
Contributor

@keruch keruch commented Jul 3, 2024

Part of #930

This PR contains changes with respect to the generalized DA specification. This is the second part of the implementation following #929.

Changelog:

  • Added batch encoding/decoding [1]
  • Added RetrieveBatchesV2 method [2]

For Author:

  • Targeted PR against correct branch
  • Included the correct type prefix in the PR title
  • Linked to Github issue with discussion and accepted design
  • Targets only one github issue
  • Wrote unit and integration tests <-- is not needed in this PR
  • All CI checks have passed
  • Added relevant godoc comments

For Reviewer:

  • Confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • Confirmed all author checklist items have been addressed

After reviewer approval:

  • In case targets main branch, PR should be squashed and merged.
  • In case PR targets a release branch, PR should be rebased.

@keruch keruch self-assigned this Jul 3, 2024
@keruch keruch changed the base branch from main to kirill/interchain-da July 3, 2024 17:49
@keruch keruch changed the title Kirill/interchain da retrieve batch feat(da): RetrieveBatchesV2 method Jul 3, 2024
@keruch keruch added the C:da DA related label Jul 3, 2024
@keruch keruch marked this pull request as ready for review July 4, 2024 12:10
@keruch keruch requested a review from a team as a code owner July 4, 2024 12:10
@keruch keruch requested review from srene and spoo-bar July 4, 2024 12:10
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

you don't need to say 'can't or 'failed to' or 'could not' in any of the errors

But otherwise really nice and thanks for wrapping them

Comment on lines +38 to +47
if err != nil {
return types.Batch{}, fmt.Errorf("can't gunzip batch: %w", err)
}

// Prepare the blob data
var batch types.Batch
err = batch.UnmarshalBinary(binary)
if err != nil {
return types.Batch{}, fmt.Errorf("can't unmarshal batch: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

dont need 'can't in the messages

Comment on lines +17 to +33
// Generate batches with random headers
expected := types.Batch{
StartHeight: h1,
EndHeight: h2,
Blocks: []*types.Block{},
Commits: []*types.Commit{},
}

data, err := interchain.EncodeBatch(expected)
require.NoError(t, err)

actual, err := interchain.DecodeBatch(data)
require.NoError(t, err)

require.Equal(t, expected, actual)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what exactly do you expect to reveal by only fuzzing the heights?

keruch and others added 18 commits July 4, 2024 14:48
danwt and others added 25 commits August 8, 2024 11:35
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Michael Tsitrin <[email protected]>
Co-authored-by: Omri <[email protected]>
Co-authored-by: Daniel T <[email protected]>
@keruch keruch marked this pull request as draft September 13, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants