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

fix: parseNamespace for <= 10 byte namespace IDs #2325

Merged
merged 8 commits into from
Jun 16, 2023

Conversation

rootulp
Copy link
Contributor

@rootulp rootulp commented Jun 5, 2023

Closes #2321

@rootulp rootulp self-assigned this Jun 5, 2023
@github-actions github-actions bot added the external Issues created by non node team members label Jun 5, 2023
@rootulp rootulp added kind:fix Attached to bug-fixing PRs and removed external Issues created by non node team members labels Jun 5, 2023
@rootulp rootulp marked this pull request as ready for review June 5, 2023 21:26
cmd/celestia/rpc_test.go Outdated Show resolved Hide resolved
cmd/celestia/rpc.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2023

Codecov Report

Merging #2325 (a30025b) into main (f05b71f) will increase coverage by 0.12%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main    #2325      +/-   ##
==========================================
+ Coverage   50.55%   50.68%   +0.12%     
==========================================
  Files         154      154              
  Lines        9733     9736       +3     
==========================================
+ Hits         4921     4935      +14     
+ Misses       4385     4373      -12     
- Partials      427      428       +1     
Impacted Files Coverage Δ
state/core_access.go 27.53% <0.00%> (-0.31%) ⬇️
cmd/celestia/rpc.go 12.65% <100.00%> (+2.71%) ⬆️

... and 4 files with indirect coverage changes

cmd/celestia/rpc.go Outdated Show resolved Hide resolved
@rootulp rootulp requested a review from Wondertan June 6, 2023 14:53
@Wondertan
Copy link
Member

@rootulp, note that I am waiting for @distractedm1nd to return from sick leave and look deeper into this PR, as he is an author. I am not deep in the context of rpc cli

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.

Sent @rootulp a fix for the missing test case and added two additional ones, now the coverage is pretty good. Once applied i will approve

distractedm1nd
distractedm1nd previously approved these changes Jun 7, 2023
cmd/celestia/rpc.go Outdated Show resolved Hide resolved
@rootulp
Copy link
Contributor Author

rootulp commented Jun 7, 2023

Synced w/ @distractedm1nd offline and parseNamespace is intended to only be used for the namespace ID portion of a namespace so a test case in this PR is incorrect.

I'll push a few commits to this PR that renames things like namespace to namespace ID where applicable but we should really do #2301 ASAP.

@rootulp
Copy link
Contributor Author

rootulp commented Jun 7, 2023

The scope of this PR has increased b/c it wasn't clear to me what the original intended behavior was. Would appreciate extra attention when reviewing b/c this couples a fix, refactor, and behavior change (left pad instead of right pad)

@Wondertan
Copy link
Member

Synced w/ @distractedm1nd offline and parseNamespace is intended to only be used for the namespace ID portion of a namespace so a test case in this PR is incorrect.

@distractedm1nd, why do we want to disable versioning here?

Wondertan
Wondertan previously approved these changes Jun 15, 2023
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.

LGTM

cmd/celestia/rpc.go 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.

I still don't get whats the point of panicking on user input though

@Wondertan Wondertan enabled auto-merge (squash) June 15, 2023 15:44
cmd/celestia/rpc_test.go Outdated Show resolved Hide resolved
cmd/celestia/rpc.go 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.

Good catch @rootulp

@Wondertan Wondertan merged commit 258dc78 into celestiaorg:main Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:fix Attached to bug-fixing PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

panic: Error creating blob: invalid size of the namespace id. got:10, want:29
6 participants