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/module): integrate blob service #2200

Merged

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented May 15, 2023

Overview

Integration of the BlobService to the node

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 kind:break! Attached to breaking PRs area:blob labels May 15, 2023
@vgonkivs vgonkivs self-assigned this May 15, 2023
@vgonkivs vgonkivs force-pushed the integrate_blob_service branch from 9f51204 to 2ae756a Compare May 16, 2023 16:53
@vgonkivs vgonkivs requested review from adlerjohn and liamsi as code owners May 16, 2023 16:53
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

utACK approve: changes in the ADR are straightforward and make sense.

docs/adr/adr-009-public-api.md Outdated Show resolved Hide resolved
@vgonkivs vgonkivs force-pushed the integrate_blob_service branch 2 times, most recently from 2a49dc0 to 2dd47c9 Compare May 22, 2023 17:07
@vgonkivs vgonkivs force-pushed the integrate_blob_service branch from 2dd47c9 to d31f921 Compare May 22, 2023 17:35
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2023

Codecov Report

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

@@                      Coverage Diff                      @@
##             feature_branch_blob_module    #2200   +/-   ##
=============================================================
  Coverage                              ?   56.18%           
=============================================================
  Files                                 ?      222           
  Lines                                 ?    14578           
  Branches                              ?        0           
=============================================================
  Hits                                  ?     8191           
  Misses                                ?     5573           
  Partials                              ?      814           

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.

You must integrate the blob service into the RPC client in api/rpc/client.go. This will probably also include adding an example type to api/docgen/examples.go. You can verify this works by running make openrpc-gen and confirming the output has the new methods

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.

Forgot that RPC integration is a different PR. Looks good to me, lets test on rpc branch

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.

looks good but why is this breaking?

blob/service.go Outdated Show resolved Hide resolved
@vgonkivs
Copy link
Member Author

It introduces a new component in node. I assumed that even it does not break our code but it's still a very important for node.

@Wondertan
Copy link
Member

Breaking is when something existing breaks. Solely an additional/feature is not breaking

@vgonkivs
Copy link
Member Author

Okey

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.

First pass, have to review swamp tests as well

blob/service.go Show resolved Hide resolved
header/headertest/testing.go Show resolved Hide resolved
blob/testing.go Outdated Show resolved Hide resolved
nodebuilder/blob/module.go Outdated Show resolved Hide resolved
nodebuilder/blob/module.go Outdated Show resolved Hide resolved
nodebuilder/blob/blob.go Outdated Show resolved Hide resolved
nodebuilder/blob/blob.go Outdated Show resolved Hide resolved
nodebuilder/blob/blob.go Outdated Show resolved Hide resolved
nodebuilder/blob/blob.go Outdated Show resolved Hide resolved
nodebuilder/blob/blob.go Outdated Show resolved Hide resolved
@renaynay renaynay changed the title feat!(blob/module): integrate blob service feat(blob/module): integrate blob service May 24, 2023
@renaynay renaynay removed the kind:break! Attached to breaking PRs label May 24, 2023
nodebuilder/tests/blob_test.go Outdated Show resolved Hide resolved
nodebuilder/tests/blob_test.go Outdated Show resolved Hide resolved
nodebuilder/tests/blob_test.go Outdated Show resolved Hide resolved
nodebuilder/tests/blob_test.go Outdated Show resolved Hide resolved
nodebuilder/tests/blob_test.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.

Love these tests ❤️

nodebuilder/tests/blob_test.go Show resolved Hide resolved
blob/blobtest/testing.go Show resolved Hide resolved
@vgonkivs vgonkivs merged commit b100118 into celestiaorg:feature_branch_blob_module May 29, 2023
vgonkivs added a commit that referenced this pull request May 29, 2023
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
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants