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!: max namespace version for parity namespace #1627

Merged
merged 3 commits into from
Apr 17, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Apr 13, 2023

Re-apply #1590 against main

Closes celestiaorg#1587

Motivated by an effort to bump celestia-node's dependency on
celestia-app. Will cut a new v0.14.x RC after this merges and also
cherry-pick this to main. Can split this into multiple PRs if requested.

1. Export `NewShare`
2. Modify parity namespace to use the max namespace version so that the
NMT feature `ignoreMaxNamespace` works as expected

Follow-ups:
- celestiaorg#1610 (not blocking
the upgrade so spinning out)
@rootulp rootulp added the warn:api breaking item will be break an API and require a major bump label Apr 13, 2023
@rootulp rootulp requested a review from cmwaters April 13, 2023 16:24
@rootulp rootulp self-assigned this Apr 13, 2023
@rootulp rootulp changed the title feat!: max namespace version for parity namespace (#1590) feat!: max namespace version for parity namespace Apr 13, 2023
@rootulp rootulp marked this pull request as ready for review April 13, 2023 21:10
@MSevey MSevey requested a review from a team April 14, 2023 14:48
@codecov-commenter
Copy link

Codecov Report

Merging #1627 (156577a) into main (afc29fe) will decrease coverage by 0.14%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##             main    #1627      +/-   ##
==========================================
- Coverage   49.18%   49.04%   -0.14%     
==========================================
  Files          84       85       +1     
  Lines        4949     4963      +14     
==========================================
  Hits         2434     2434              
- Misses       2277     2290      +13     
- Partials      238      239       +1     
Impacted Files Coverage Δ
pkg/namespace/random_namespace.go 0.00% <0.00%> (ø)
pkg/shares/shares.go 74.28% <11.11%> (-1.08%) ⬇️
pkg/proof/proof.go 64.12% <40.00%> (-1.51%) ⬇️
pkg/shares/share_builder.go 76.31% <100.00%> (ø)
x/blob/types/payforblob.go 71.84% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Other than my comment and the failing linter, everything else looks good

x/blob/types/payforblob_test.go Show resolved Hide resolved
@MSevey MSevey requested a review from a team April 17, 2023 15:01
@rootulp rootulp merged commit d5c20a0 into celestiaorg:main Apr 17, 2023
13 of 21 checks passed
@rootulp rootulp deleted the rp/cherry-pick-max-version-parity branch April 17, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
warn:api breaking item will be break an API and require a major bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants