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

deps!: bumping nmt to v0.19.0, breaking rpc response fields #2643

Closed
wants to merge 1 commit into from

Conversation

distractedm1nd
Copy link
Collaborator

Related: #2633

@distractedm1nd distractedm1nd added area:shares Shares and samples kind:deps Pull requests that update a dependency file labels Aug 31, 2023
@distractedm1nd distractedm1nd self-assigned this Aug 31, 2023
@Wondertan
Copy link
Member

Changes of the name do not break protos. Changes of the type and fields reorder do.

@codecov-commenter
Copy link

Codecov Report

Merging #2643 (97a3881) into main (47047f3) will decrease coverage by 0.28%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2643      +/-   ##
==========================================
- Coverage   51.34%   51.06%   -0.28%     
==========================================
  Files         158      158              
  Lines       10559    10559              
==========================================
- Hits         5421     5392      -29     
- Misses       4669     4693      +24     
- Partials      469      474       +5     

see 7 files with indirect coverage changes

@distractedm1nd
Copy link
Collaborator Author

@Wondertan, interesting, assumed it is breaking since older versions will use different names? Or does the way it work not care about naming and just uses data types and offsets. The JSON representation is definitely breaking, it is at least breaking our RPC return types that embed nmt.Proof

@Wondertan
Copy link
Member

Or does the way it work not care about naming and just uses data types and offsets.

Yes

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Can we bump when we bump core?

@distractedm1nd distractedm1nd changed the title deps!: bumping nmt to v0.19.0, breaking protobuf deps!: bumping nmt to v0.19.0, breaking rpc response fields Sep 4, 2023
@distractedm1nd
Copy link
Collaborator Author

sure, but then we should mark the core bump as breaking, since it breaks rpc response types

@distractedm1nd
Copy link
Collaborator Author

closing in favor of core bump #2646

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:shares Shares and samples kind:deps Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants