-
Notifications
You must be signed in to change notification settings - Fork 96
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
fix(ics-23): Auto-derive serde::Serialize
for CommitmentPrefix
#1230
Conversation
serde::Serialize
for CommitmentPrefix
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1230 +/- ##
==========================================
+ Coverage 67.31% 67.33% +0.01%
==========================================
Files 234 234
Lines 23553 23547 -6
==========================================
Hits 15855 15855
+ Misses 7698 7692 -6 ☔ View full report in Codecov by Sentry. |
I think that's fine, better to break compatibility now and have consistent serialization than the reverse. |
Sounds good. @seanchen1991 let's add a changelog to indicate the breaking change? |
@soareschen, you mentioned that you opened a fork that manually implements deserialization from |
Oh, I just saw this comment, which makes me think we should just opt to auto-derive |
Yes, the custom fix is at https://github.com/cosmos/ibc-rs/tree/soares/fix-commitment-prefix-deserialize. But we should serialize and deserialize it as bytes, as the current behavior for serializing non-UTF8 commitment prefix is incorrect. |
…1230) * Auto-derive serde::Serialize for CommitmentPrefix * Add changelog entry
Closes: #1229
Description
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.