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: account for secondary reserved namespace range #2562

Closed
wants to merge 1 commit into from
Closed

feat: account for secondary reserved namespace range #2562

wants to merge 1 commit into from

Conversation

rootulp
Copy link
Contributor

@rootulp rootulp commented Aug 12, 2023

@rootulp rootulp added the area:core_and_app Relationship with Core node and Celestia-App label Aug 12, 2023
@rootulp rootulp self-assigned this Aug 12, 2023
@github-actions github-actions bot added the external Issues created by non node team members label Aug 12, 2023
rootulp
rootulp commented Aug 12, 2023
View reviewed changes
share/namespace.go
@@ -106,6 +107,7 @@ func (n Namespace) Validate() error {
}

// ValidateForData checks if the Namespace is of real/useful data.
// TODO: it's not clear what "real/useful" data means.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Wondertan @vgonkivs can you please help me understand what "real/useful" data means? Do we need ValidateForData or is ValidateForBlob sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we need to maintain this function, it may need to be updated in this PR.

Copy link
Member

@Wondertan Wondertan Aug 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not parity or padding data

@codecov-commenter
Copy link

Codecov Report

Merging #2562 (c56d1de) into main (87e9500) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2562      +/-   ##
==========================================
+ Coverage   51.05%   51.23%   +0.17%     
==========================================
  Files         158      158              
  Lines       10398    10398              
==========================================
+ Hits         5309     5327      +18     
+ Misses       4622     4607      -15     
+ Partials      467      464       -3
Files Changed Coverage Δ
share/namespace.go 61.25% <100.00%> (+6.25%) ⬆️

... and 4 files with indirect coverage changes

@rootulp
Copy link
Contributor Author

rootulp commented Aug 21, 2023

Closing b/c #2581 has incorporated most of these changes.

@rootulp rootulp closed this Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan left review comments

@renaynay renaynay Awaiting requested review from renaynay renaynay will be requested when the pull request is marked ready for review renaynay is a code owner

@vgonkivs vgonkivs Awaiting requested review from vgonkivs vgonkivs will be requested when the pull request is marked ready for review vgonkivs is a code owner

@walldiss walldiss Awaiting requested review from walldiss walldiss will be requested when the pull request is marked ready for review walldiss is a code owner

@cristaloleg cristaloleg Awaiting requested review from cristaloleg cristaloleg will be requested when the pull request is marked ready for review cristaloleg is a code owner

Assignees

@rootulp rootulp

Labels
area:core_and_app Relationship with Core node and Celestia-App external Issues created by non node team members
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Account for secondary reserved namespace range
3 participants
@rootulp @codecov-commenter @Wondertan