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

improvement(blob/cmd): remove base64 representation #3553

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Jul 4, 2024

This PR removes the base64 flag and its related functionality as it was completely unused. Additionally, it changes the return type of the commitment to its hex representation.

{
  "result": {
    "height": 1399400,
    "commitments": [
      "0x21783ed3c1d5e51b0f1772e57bc3c7f81d93506b0652c062b1e7e5c61eb00791"
    ]
  }
}

celestia blob get 1337536 0x42690c204d39600fddd3 0x21783ed3c1d5e51b0f1772e57bc3c7f81d93506b0652c062b1e7e5c61eb00791
{
  "result": {
    "namespace": "0x42690c204d39600fddd3",
    "data": "gm",
    "share_version": 0,
    "commitment": "0x21783ed3c1d5e51b0f1772e57bc3c7f81d93506b0652c062b1e7e5c61eb00791",
    "index": 1
  }
}

celestia blob get-all 1337536 0x42690c204d39600fddd3
{
  "result": [
    {
      "namespace": "0x42690c204d39600fddd3",
      "data": "gm",
      "share_version": 0,
      "commitment": "0x21783ed3c1d5e51b0f1772e57bc3c7f81d93506b0652c062b1e7e5c61eb00791",
      "index": 1
    }
  ]
}

@vgonkivs vgonkivs added kind:misc Attached to miscellaneous PRs area:blob labels Jul 4, 2024
@vgonkivs vgonkivs requested a review from jcstein July 4, 2024 11:52
@vgonkivs vgonkivs self-assigned this Jul 4, 2024
ramin
ramin previously approved these changes Jul 4, 2024
@distractedm1nd
Copy link
Collaborator

Have you synced w @jcstein about this?
I think we need to remove it in docs as well
https://docs.celestia.org/developers/node-tutorial#submitting-data

@vgonkivs
Copy link
Member Author

vgonkivs commented Jul 4, 2024

I've discussed this change with him. But we haven't agreed on removing the flag and I think as we allow now only hexed namespace and commitment it won't have much sense in keeping base64 flag. For the commitment, I think it makes sense to keep all fields consistent.

ramin
ramin previously approved these changes Jul 4, 2024
distractedm1nd
distractedm1nd previously approved these changes Jul 4, 2024
walldiss
walldiss previously approved these changes Jul 4, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.

Project coverage is 45.44%. Comparing base (2469e7a) to head (962d367).
Report is 136 commits behind head on main.

Files Patch % Lines
nodebuilder/blob/cmd/blob.go 0.00% 32 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3553      +/-   ##
==========================================
+ Coverage   44.83%   45.44%   +0.61%     
==========================================
  Files         265      274       +9     
  Lines       14620    15446     +826     
==========================================
+ Hits         6555     7020     +465     
- Misses       7313     7624     +311     
- Partials      752      802      +50     

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

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.

no 🚔 today

@vgonkivs vgonkivs enabled auto-merge (squash) July 4, 2024 14:47
@ramin ramin self-requested a review July 4, 2024 14:51
@vgonkivs vgonkivs merged commit 70fbb37 into celestiaorg:main Jul 4, 2024
28 checks passed
@vgonkivs vgonkivs mentioned this pull request Jul 5, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:blob kind:misc Attached to miscellaneous PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants