Compile errors of v0.41.0 while only serde feature enabled

[riversyang]

Problem Summary

During the integration of ibc-rs v0.41.0 into near-ibc, I got compile errors in the following places:

ibc-rs/crates/ibc/src/applications/transfer/packet.rs

Lines 13 to 25 in 349d7f5

	/// Defines the structure of token transfers' packet bytes
	#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
	#[cfg_attr(
	feature = "serde",
	serde(try_from = "RawPacketData", into = "RawPacketData")
	)]
	#[derive(Clone, Debug, PartialEq, Eq)]
	pub struct PacketData {
	pub token: PrefixedCoin,
	pub sender: Signer,
	pub receiver: Signer,
	pub memo: Memo,
	}

ibc-rs/crates/ibc/src/core/ics02_client/events.rs

Lines 135 to 151 in 349d7f5

	#[cfg_attr(
	feature = "parity-scale-codec",
	derive(
	parity_scale_codec::Encode,
	parity_scale_codec::Decode,
	scale_info::TypeInfo
	)
	)]
	#[cfg_attr(
	feature = "borsh",
	derive(borsh::BorshSerialize, borsh::BorshDeserialize)
	)]
	#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
	#[derive(Clone, Debug, From, PartialEq, Eq)]
	struct HeaderAttribute {
	header: Any,
	}

In near-ibc project, we only need to enable borsh and serde features of ibc-rs. So I checked source code and found that this is because the definition of ibc_proto::ibc::applications::transfer::v2::FungibleTokenPacketData and ibc_proto::google::protobuf::Any in ibc-proto-rs v0.32.0 were defined as:

#[cfg_attr(feature = "std", derive(::serde::Serialize, ::serde::Deserialize))]

I have removed the feature condition from the derive statement for now, which allowed the code to compile. But I'm not sure whether I should commit this issue to ibc-proto-rs.

Anyway, I believe it would be worth discussing the issue in the ibc-rs implementation. It is important to determine whether the current implementation of the two mentioned places is necessary and appropriate.

[Farhad-Shabani]

Thanks @riversyang for laying out the problem so clearly. Yes, it’s the right place to discuss!

For a bit of context

After implementing ProtoJSON-compatible Serialize and Deserialize on the ics23 Protobuf definitions using pbjson (which is not compatible with no-std envs) and then re-exporting the ics23 type in ibc-proto-rs, serde feature is no longer supported in no-std envs. And we put restoring the no-std compatibility on hold waiting to hear back before we dive into that.

Possible remedy

Yes! As you said and from what I see, those two impls look unnecessary:

1. The serde drive on IbcEvent (which required serde derive on Any) was added through this PR. Sounds like, initially, the intention was to derive only parity-scale-codec and borsh, but we ended up with an additional serde derive. Perhaps @DaviRain-Su can help us to clarify whether serde is needed there? If not, we can remove it.

2. Regarding PacketData, based on my checks, it seems safe to remove these lines. However, Should be noted we can't completely drop the serde since packet data is commonly serialized by JSON serde when sending over IBC.

Assuming no objections, this should work, and we can avoid changing ibc-proto-rs.

A side question

IIRC, you were also using the parity-scale-codec feature before. Noticed that you no longer activate it (checked this branch). Is it no longer needed?

[riversyang]

Thanks for the quick response, @Farhad-Shabani!

Regarding the PacketData struct, I agree with you that to remove the lines you mentioned while keeping the serde feature.

As for the HeaderAttribute struct, I believe it's necessary to keep the serde feature. JSON serialization is required for IbcEvent in certain cases, such as emitting the event via smart contract log text in near-ibc implementation. Since this struct is the only place where a type from ibc-proto-rs is used, would changing the type of the header field to Vec<u8> solve the issue more straightforwardly?

Lastly, to clarify, the parity-scale-codec feature is not required in near-ibc implementation and was mistakenly enabled before. It seems that it is for the substrate-ibc implementation.

[DaviRain-Su]

in near ibc maybe you can use borsh😃

[DaviRain-Su]

relate this relate no-std ： cosmos/ics23#155 i working this， aim to enable ics23 serde to support no -std

[DaviRain-Su]

about issue: #753

[Farhad-Shabani]

Regarding the PacketData struct, I agree with you that to remove the lines you mentioned while keeping the serde feature.

Check #763, please

Since this struct is the only place where a type from ibc-proto-rs is used, would changing the type of the header field to Vec solve the issue more straightforwardly?

Opened #764

I don't know any other ways in ibc-rs to get this work.
Sounds like we should follow it either in ibc-proto-rs or pbjson crate.
Mark this as blocked till #753 gets fixed.

[dzmitry-lahoda]

oh, so many issues. what issue to track when it will be fixed?

[dzmitry-lahoda]

last working commit 2403dc3 . why not to revert breaking change until it fixed?

[Farhad-Shabani]

oh, so many issues. what issue to track when it will be fixed?

Basically "restoring no_std compatibility" for ibc-proto-rs (reopened cosmos/ibc-proto-rs#98). By which, then the serde feature would work for no_std envs.

last working commit 2403dc3 . why not to revert breaking change until it fixed?

There are projects dependent on ibc-proto-rs >= v0.32