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(blob): extend blob struct with index field #3165

Merged
merged 15 commits into from
Mar 1, 2024

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Feb 5, 2024

What was done:

  • added index to the blob struct;
  • introduced a new parser entity that collects shares and transforms them to blob;
  • added tests to compare shares received from share service by coordinates that have gotten from blob index;

@vgonkivs vgonkivs added kind:feat Attached to feature PRs area:blob labels Feb 5, 2024
@vgonkivs vgonkivs requested a review from jcstein February 5, 2024 21:09
@vgonkivs vgonkivs self-assigned this Feb 5, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2024

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (5e1895c) 51.81% compared to head (522e65c) 52.23%.
Report is 3 commits behind head on main.

Files Patch % Lines
blob/parser.go 69.56% 14 Missing and 7 partials ⚠️
blob/helper.go 54.54% 9 Missing and 6 partials ⚠️
blob/service.go 87.61% 11 Missing and 2 partials ⚠️
blob/blob.go 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3165      +/-   ##
==========================================
+ Coverage   51.81%   52.23%   +0.41%     
==========================================
  Files         178      179       +1     
  Lines       11286    11422     +136     
==========================================
+ Hits         5848     5966     +118     
- Misses       4930     4955      +25     
+ Partials      508      501       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

blob/service.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs marked this pull request as ready for review February 14, 2024 09:54
@Wondertan
Copy link
Member

We should mark this API and proto breaking

@walldiss
Copy link
Member

walldiss commented Feb 14, 2024

We should mark this API and proto breaking

Looks like there are only new fields added, so it is backwards compatible.

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.

Initial review of semantics

blob/blob.go Outdated Show resolved Hide resolved
blob/blob.go Outdated Show resolved Hide resolved
blob/helper.go Show resolved Hide resolved
blob/blob.go Outdated Show resolved Hide resolved
blob/blob.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

Looks like there are only new fields added, so it is backwards compatible.

I know, but that's not the point. We need to mark it so that eiger team can react to this PR and update rust client respectively

@vgonkivs vgonkivs changed the title feat(blob): extend blob struct with index field feat(blob)!: extend blob struct with index field Feb 14, 2024
@vgonkivs vgonkivs changed the title feat(blob)!: extend blob struct with index field feat(blob): extend blob struct with index field Feb 15, 2024
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.

Another semantical review

blob/service_test.go Outdated Show resolved Hide resolved
blob/service_test.go Outdated Show resolved Hide resolved
blob/service_test.go Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/blob.go Outdated Show resolved Hide resolved
blob/blob.go Outdated Show resolved Hide resolved
blob/blob.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/blob.go Outdated Show resolved Hide resolved
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.

another general comment, the code inside retrieve is getting long/hairy and would benefit from some comments throughout.

blob/blob.go Outdated Show resolved Hide resolved
blob/blob.go Outdated Show resolved Hide resolved
blob/blob.go Outdated Show resolved Hide resolved
blob/helper.go Show resolved Hide resolved
blob/helper.go Outdated Show resolved Hide resolved
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.

Last little comments/questions. Looking slightly better

blob/blob.go Show resolved Hide resolved
blob/helper.go Show resolved Hide resolved
blob/parser.go Outdated Show resolved Hide resolved
blob/helper.go Outdated Show resolved Hide resolved
blob/service_test.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Feb 28, 2024
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.

Please improve the PR description and add other secondary changes in this PR, like tracing

blob/parser.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Feb 28, 2024
Wondertan
Wondertan previously approved these changes Feb 28, 2024
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs merged commit f2b664f into celestiaorg:main Mar 1, 2024
25 checks passed
@jcstein
Copy link
Member

jcstein commented Mar 1, 2024

  • update examples for rpc docs
{
  "id": 1,
  "jsonrpc": "2.0",
  "result": [
    {
      "namespace": "AAAAAAAAAAAAAAAAAAAAAAAAAAECAwQFBgcICRA=",
      "data": "VGhpcyBpcyBhbiBleGFtcGxlIG9mIHNvbWUgYmxvYiBkYXRh",
      "share_version": 0,
      "commitment": "AD5EzbG0/EMvpw0p8NIjMVnoCP4Bv6K+V6gjmwdXUKU="
      "index": 1
    }
  ]
}

@jcstein
Copy link
Member

jcstein commented Mar 4, 2024

  • update the node-tutorial with new values once this is in a release on mainnet/mocha/arabica. task for @jcstein

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:blob kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants