Add serde feature

[hu55a1n1]

Closes: #293

Description

This PR builds on top of @DaviRain-Su's PR #301 - I was unable to push on their fork so opened this PR.

---

PR author checklist:

• Added changelog entry, using unclog.
• Added tests.
• Linked to GitHub issue.
• Updated code comments and documentation (e.g., docs/).
• Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

• Reviewed Files changed in the GitHub PR explorer.
• Manually tested (in case integration/unit/mock tests are absent).

[codecov]

Codecov Report

Base: 62.44% // Head: 62.23% // Decreases project coverage by -0.20% ⚠️

Coverage data is based on head (a2a3a3f) compared to base (7a7dad7).
Patch coverage: 41.80% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #331      +/-   ##
==========================================
- Coverage   62.44%   62.23%   -0.21%     
==========================================
  Files         124      124              
  Lines       14072    14134      +62     
==========================================
+ Hits         8787     8797      +10     
- Misses       5285     5337      +52     

Impacted Files	Coverage Δ
crates/ibc/src/core/ics02_client/client_state.rs	75.00% <ø> (ø)
...rates/ibc/src/core/ics02_client/consensus_state.rs	42.85% <ø> (ø)
crates/ibc/src/core/ics02_client/header.rs	50.00% <ø> (ø)
crates/ibc/src/core/ics03_connection/events.rs	26.98% <ø> (ø)
.../src/core/ics04_channel/handler/acknowledgement.rs	79.82% <ø> (ø)
.../ibc/src/core/ics04_channel/handler/recv_packet.rs	80.14% <ø> (ø)
.../ibc/src/core/ics04_channel/handler/send_packet.rs	80.51% <ø> (ø)
...ates/ibc/src/core/ics04_channel/handler/timeout.rs	81.75% <ø> (ø)
...src/core/ics04_channel/handler/timeout_on_close.rs	81.50% <ø> (ø)
...ore/ics04_channel/handler/write_acknowledgement.rs	84.21% <ø> (ø)
... and 35 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[hu55a1n1]

I managed to fix the CI but there are a couple of things to consider ->

• The ICS20 impl encodes PacketData and Acknowledgement as JSON and therefore depends on serde. This means the build will fail with --no-default-features unless there's an accompanying --features=clock,serde (I updated the CI to test for this). Not sure what's the best way to fix this - maybe an app-transfer = ["serde"] feature or moving the transfer app into a separate crate?
• All light client traits use ErasedSerialize as a supertrait. This is required for raw-/domain-type conversions. I get around this by defining a trait ErasedSerialize {} with a blanket impl for all types if the serde feature is disabled.

[plafer]

cargo check --no-default-features currently fails (I'll add the check to the CI). Seems like one of the fixes is to put the pub mod transfer under the serde feature (since it uses serde_json).

Also I noticed today that ibc-proto-rs depends on serde (unsurprisingly), so we still implicitly depend on serde even when the feature is off. However, this is a step in the right direction.

[plafer]

Why is the serde_string module necessary? Also the serialization/deserialization is based on the Display implementation, which seems brittle and easy to break

[hu55a1n1]

The Coin::amount is serialized as a decimal string so we use the display impl for easy serde. I think this is a popular pattern and there are crates that provide this functionality e.g. serde_with. Can you please expand on why you think this is brittle?

[plafer]

I find it brittle because in general, Display is for human-readable output; it's easy to forget that it's also used for serialization. If we change it for a different output, we just made a breaking serialization change, which is pretty bad. I find it unfortunate that it's a common pattern... We can leave it as is though

[plafer]

I don't think we should change the CI like this; it should be possible to use the library as no_std, without wanting serde. There are currently real errors to fix in the no_std case (see previous review).

Should we add a cargo check --no-default-features to the CI? Or is clippy a superset of check?

[hu55a1n1]

Should we add a cargo check --no-default-features to the CI? Or is clippy a superset of check?

Good point! I think a clippy --all should cover everything and therefore there is no need to run clippy with --no-default-features - what we really need is just a check --no-default-features. WDYT?

it should be possible to use the library as no_std, without wanting serde

The ICS20 impl relies on serde for correctness (to properly deal with PacketData and Acknowledgement) in the module callbacks such as on_recv_packet. Should we also feature-gate them? (Does it make sense to feature gate individual callbacks or should we simply feature gate the whole app itself?)

[hu55a1n1]

Note that encoding to JSON is required by the spec ->

Note that both the FungibleTokenPacketData as well as FungibleTokenPacketAcknowledgement must be JSON-encoded (not Protobuf encoded) when they serialized into packet data. Also note that uint256 is string encoded when converted to JSON, but must be a valid decimal number of the form [0-9]+.

[plafer]

Right... Here is my solution: a2a3a3f. Basically it puts the transfer application behind the serde feature. WDYT?

Also note that since #341, this workflow has changed to

args: --no-default-features --lib

[plafer]

Please merge it if you agree with my latest changes