search

cosmos / ibc-rs

Rust implementation of the Inter-Blockchain Communication (IBC) protocol.
Apache License 2.0
181 stars 73 forks source link

fix(ics-23): Auto-derive `serde::Serialize` for `CommitmentPrefix` #1230

Closed seanchen1991 closed 1 month ago

seanchen1991 commented 1 month ago

Closes: #1229

Description

PR author checklist:

Reviewer checklist:

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 67.33%. Comparing base (86a0ee0) to head (c3cbca4).

Files Patch % Lines
ibc-core/ics23-commitment/types/src/commitment.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

soareschen commented 1 month ago

I think this is a reasonable fix. However, we may want to consider whether it is fine for breaking compatibility in this case. Let's wait for @rnbguy and @romac to review and discuss the changes.

romac commented 1 month ago

I think that's fine, better to break compatibility now and have consistent serialization than the reverse.

soareschen commented 1 month ago

Sounds good. @seanchen1991 let's add a changelog to indicate the breaking change?

seanchen1991 commented 1 month ago

@soareschen, you mentioned that you opened a fork that manually implements deserialization from String. Would that route be preferable than opting to auto-derive Serialize?

seanchen1991 commented 1 month ago

Oh, I just saw this comment, which makes me think we should just opt to auto-derive Serialize instead of manually implement Deserialize.

soareschen commented 1 month ago

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.