-
Notifications
You must be signed in to change notification settings - Fork 103
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(x/data): remove oneof and update validation #946
Conversation
Codecov Report
@@ Coverage Diff @@
## master #946 +/- ##
==========================================
+ Coverage 73.15% 73.21% +0.06%
==========================================
Files 196 196
Lines 23155 23116 -39
==========================================
- Hits 16938 16924 -14
+ Misses 4934 4922 -12
+ Partials 1283 1270 -13
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we are removing oneof
? Can we use blake3 instead?
PS: I thought we want to update proto first (before updating code) ;)
We are removing oneof because it breaks amino signing, i.e. "This pull request updates ContentHash to no longer use oneof, which breaks amino signing".
That's being discussed in #248 and would be a separate pull request.
That's the process for new features that are in the design phase. I don't think it's necessary when fixing an existing feature but maybe that's something we can further discuss. Seems unnecessary for the change here or adding queries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
Description
Closes: #872
This pull request updates ContentHash to no longer use oneof, which breaks amino signing. This pull request also improves validation checks and updates MediaType to RawMediaType, which is specific to raw content hashes and aligns with the namespace used for graph content hashes.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change