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.

Sign up for GitHub

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

Jump to bottom

feat(blob): introduce structure of blob #2012

Merged
merged 3 commits into from
Apr 11, 2023
Merged

feat(blob): introduce structure of blob #2012

merged 3 commits into from
Apr 11, 2023

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Apr 4, 2023 •
edited
Loading

Overview

Based on #1963.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@vgonkivs vgonkivs added kind:feat Attached to feature PRs area:blob labels Apr 4, 2023
@vgonkivs vgonkivs self-assigned this Apr 4, 2023
@vgonkivs vgonkivs force-pushed the introduce_blob branch 3 times, most recently from 13ed32a to db91dd8 Compare April 5, 2023 08:35
@vgonkivs vgonkivs marked this pull request as ready for review April 5, 2023 08:47
distractedm1nd
distractedm1nd approved these changes Apr 6, 2023
View reviewed changes
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

My comment can be addressed later on, when we are making sure everything checks out with the RPC :)

Wondertan
Wondertan reviewed Apr 11, 2023
View reviewed changes
blob/blob.go
Comment on lines +52 to +55
// Version is the format that was used to encode Blob data and namespaceID into shares.
func (b *Blob) Version() uint32 {
return b.Blob.ShareVersion
}
Copy link
Member

Choose a reason for hiding this comment

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

Users should be able to set the version and constructor of the Blob does not reflect that

Copy link
Member Author

Choose a reason for hiding this comment

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

Then it requires additional changes on the app side, bc now they are using DefaultShareVersion by default

Copy link
Member

Choose a reason for hiding this comment

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

I think we should communicate this to the app team then. They requested the ability to configure the version in the Blob discussion

blob/blob.go Show resolved Hide resolved
blob/blob.go Outdated

// Blob represents any application-specific binary data that anyone can submit to Celestia.
type Blob struct {
*types.Blob
Copy link
Member

Choose a reason for hiding this comment

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

Does it have to be a pointer here?

Copy link
Member Author

Choose a reason for hiding this comment

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

no

renaynay
renaynay approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

generally approve - left two comments.

blob/blob.go Show resolved Hide resolved
blob/blob.go Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (feature_branch_blob_module@18cb0c0). Click here to learn what that means.
The diff coverage is n/a.

@@                      Coverage Diff                      @@
##             feature_branch_blob_module    #2012   +/-   ##
=============================================================
  Coverage                              ?   54.66%           
=============================================================
  Files                                 ?      216           
  Lines                                 ?    14021           
  Branches                              ?        0           
=============================================================
  Hits                                  ?     7665           
  Misses                                ?     5536           
  Partials                              ?      820

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vgonkivs vgonkivs force-pushed the introduce_blob branch 2 times, most recently from 7c02bb9 to b1902af Compare April 11, 2023 12:51
Wondertan
Wondertan reviewed Apr 11, 2023
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

My last concern is share versioning. We can merge it as is if the app’s blob doesn't allow us to modify it, but if not we should allow changing this in this PR

@vgonkivs
Copy link
Member Author

app already had an opened PR that allows passing share version. Will extend our c-tor with one more param.

@vgonkivs vgonkivs merged commit 5fd7381 into celestiaorg:feature_branch_blob_module Apr 11, 2023
vgonkivs added a commit to vgonkivs/celestia-node that referenced this pull request Apr 11, 2023
@vgonkivs
feat(blob): introduce structure of blob (celestiaorg#2012)
0387e6f
vgonkivs added a commit that referenced this pull request May 11, 2023
@vgonkivs
feat(blob): introduce structure of blob (#2012)
8be5abb
vgonkivs added a commit to vgonkivs/celestia-node that referenced this pull request May 16, 2023
@vgonkivs
feat(blob): introduce structure of blob (celestiaorg#2012)
df7e50a
vgonkivs added a commit that referenced this pull request May 29, 2023
@vgonkivs
feat(blob): introduce structure of blob (#2012)
4a19142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@distractedm1nd distractedm1nd distractedm1nd approved these changes

@Wondertan Wondertan Wondertan approved these changes

@renaynay renaynay renaynay approved these changes

@walldiss walldiss Awaiting requested review from walldiss

Assignees

@vgonkivs vgonkivs

Labels
area:blob kind:feat Attached to feature PRs
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vgonkivs @codecov-commenter @distractedm1nd @Wondertan @renaynay