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

refactor!: un-export namespace.ValidateBlobNamespace #2145

Merged
merged 7 commits into from
Jul 27, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jul 21, 2023

Closes #2143

De-duplicate ValidateBlobNamespace and export it from x/blob/types package. The namespace package needs to define its own isBlobNamespace because it can't import ValidateBlobNamespace from x/blob/types as that would introduce a circular dependency.

@rootulp rootulp added the refactor optional label for items that are related to implementation work and do not change functionality label Jul 21, 2023
@rootulp rootulp added this to the Post-mainnet milestone Jul 21, 2023
@rootulp rootulp self-assigned this Jul 21, 2023
@MSevey MSevey requested a review from a team July 21, 2023 19:54
@rootulp rootulp marked this pull request as draft July 21, 2023 20:06
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2023

Codecov Report

Merging #2145 (fd3c36f) into main (af1a07a) will decrease coverage by 0.01%.
The diff coverage is 23.80%.

@@            Coverage Diff             @@
##             main    #2145      +/-   ##
==========================================
- Coverage   24.41%   24.40%   -0.01%     
==========================================
  Files         126      126              
  Lines       14300    14302       +2     
==========================================
  Hits         3491     3491              
- Misses      10448    10450       +2     
  Partials      361      361              
Files Changed Coverage Δ
pkg/namespace/random_blob.go 0.00% <0.00%> (ø)
pkg/namespace/namespace.go 66.34% <100.00%> (+6.34%) ⬆️
x/blob/types/blob_tx.go 14.73% <100.00%> (ø)
x/blob/types/payforblob.go 69.54% <100.00%> (ø)

@rootulp rootulp marked this pull request as ready for review July 21, 2023 20:28
cmwaters
cmwaters previously approved these changes Jul 24, 2023
@Wondertan
Copy link
Member

@rootulp, could you open similar PR to celeria-node? I think we may have similar issue, as our code is mostly a port.

// validateVersion returns an error if the version is not supported.
func validateVersion(version uint8) error {
// validateVersionSupported returns an error if the version is not supported.
func validateVersionSupported(version uint8) error {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[note to reviewers] this function was renamed per the comment in #2143

I thought the original name was clear but I'm open to alternative naming suggestions.

@rootulp rootulp changed the title refactor: de-duplicate validate blob namespace refactor: un-export namespace.ValidateBlobNamespace Jul 24, 2023
@rootulp rootulp requested a review from evan-forbes July 24, 2023 20:17
@rootulp
Copy link
Collaborator Author

rootulp commented Jul 24, 2023

@rootulp, could you open similar PR to celeria-node? I think we may have similar issue, as our code is mostly a port.

As far as I can see, celestia-node only has one reference to ValidateBlobNamespace here which is the ValidateBlobNamespace that remains exported in this PR so I don't think any celestia-node changes are needed.

@Wondertan
Copy link
Member

@rootulp, I mean the slice.Contains if should also be added to our validate function

@rootulp rootulp changed the title refactor: un-export namespace.ValidateBlobNamespace refactor!: un-export namespace.ValidateBlobNamespace Jul 25, 2023
@rootulp rootulp enabled auto-merge (squash) July 26, 2023 13:34
@MSevey MSevey requested a review from a team July 26, 2023 17:56
@@ -36,3 +35,25 @@ func RandomBlobNamespaces(rand *tmrand.Rand, count int) (namespaces []Namespace)
}
return namespaces
}

// isBlobNamespace returns an true if this namespace is a valid user-specifiable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// isBlobNamespace returns an true if this namespace is a valid user-specifiable
// isBlobNamespace returns true if this namespace is a valid user-specifiable

@rootulp rootulp merged commit 53e7b57 into celestiaorg:main Jul 27, 2023
20 checks passed
rootulp added a commit that referenced this pull request Aug 14, 2023
Closes #2143

De-duplicate `ValidateBlobNamespace` and export it from `x/blob/types`
package. The namespace package needs to define its own `isBlobNamespace`
because it can't import `ValidateBlobNamespace` from `x/blob/types` as
that would introduce a circular dependency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor optional label for items that are related to implementation work and do not change functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ValidateBlobNamespace: cleaning up and future-proofing
5 participants