Skip to content

Add serde feature #331

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

Merged
merged 19 commits into from
Jan 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci/no-std-check/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ edition = "2021"
resolver = "2"

[dependencies]
ibc = { path = "../../crates/ibc", default-features = false, features = ["mocks-no-std"] }
ibc = { path = "../../crates/ibc", default-features = false, features = ["serde", "mocks-no-std"] }
ibc-proto = { version = "0.24.1", default-features = false, features = ["parity-scale-codec", "borsh"]}
tendermint = { version = "0.28.0", default-features = false }
tendermint-proto = { version = "0.28.0", default-features = false }
Expand Down
12 changes: 7 additions & 5 deletions crates/ibc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,24 +39,26 @@ std = [
parity-scale-codec = ["dep:parity-scale-codec", "dep:scale-info"]
borsh = ["dep:borsh"]

# This feature is required for token transfer (ICS-20)
serde = ["dep:serde", "dep:serde_derive", "serde_json", "erased-serde"]

# This feature guards the unfinished implementation of ADR 5.
val_exec_ctx = []

# This feature grants access to development-time mocking libraries, such as `MockContext` or `MockHeader`.
# Depends on the `testgen` suite for generating Tendermint light blocks.
mocks = ["tendermint-testgen", "tendermint/clock", "cfg-if", "parking_lot"]
mocks-no-std = ["cfg-if"]
serde = []

[dependencies]
# Proto definitions for all IBC-related interfaces, e.g., connections or channels.
ibc-proto = { version = "0.24.1", default-features = false, features = ["parity-scale-codec", "borsh"] }
ics23 = { version = "0.9.0", default-features = false, features = ["host-functions"] }
time = { version = ">=0.3.0, <0.3.18", default-features = false }
serde_derive = { version = "1.0.104", default-features = false }
serde = { version = "1.0", default-features = false }
serde_json = { version = "1", default-features = false }
erased-serde = { version = "0.3", default-features = false, features = ["alloc"] }
serde_derive = { version = "1.0.104", default-features = false, optional = true }
serde = { version = "1.0", default-features = false, optional = true }
serde_json = { version = "1", default-features = false, optional = true }
erased-serde = { version = "0.3", default-features = false, features = ["alloc"], optional = true }
tracing = { version = "0.1.36", default-features = false }
prost = { version = "0.11", default-features = false }
bytes = { version = "1.2.1", default-features = false }
Expand Down
1 change: 1 addition & 0 deletions crates/ibc/src/applications/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//! Various packet encoding semantics which underpin the various types of transactions.

#[cfg(feature = "serde")]
pub mod transfer;
14 changes: 7 additions & 7 deletions crates/ibc/src/applications/transfer/acknowledgement.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
use core::fmt::{Display, Error as FmtError, Formatter};

use serde::{Deserialize, Serialize};

use super::error::TokenTransferError;
use crate::core::ics26_routing::context::Acknowledgement as AckTrait;
use crate::prelude::*;
Expand All @@ -13,21 +11,23 @@ pub const ACK_ERR_STR: &str = "error handling packet on destination chain: see e
/// A successful acknowledgement, equivalent to `base64::encode(0x01)`.
pub const ACK_SUCCESS_B64: &str = "AQ==";

#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum ConstAckSuccess {
#[serde(rename = "AQ==")]
#[cfg_attr(feature = "serde", serde(rename = "AQ=="))]
Success,
}

#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum Acknowledgement {
/// Successful Acknowledgement
/// e.g. `{"result":"AQ=="}`
#[serde(rename = "result")]
#[cfg_attr(feature = "serde", serde(rename = "result"))]
Success(ConstAckSuccess),
/// Error Acknowledgement
/// e.g. `{"error":"cannot unmarshal ICS-20 transfer packet data"}`
#[serde(rename = "error")]
#[cfg_attr(feature = "serde", serde(rename = "error"))]
Error(String),
}

Expand Down
6 changes: 2 additions & 4 deletions crates/ibc/src/applications/transfer/amount.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
use core::str::FromStr;
use derive_more::{Display, From, Into};
use serde::{Deserialize, Serialize};

use super::error::TokenTransferError;
use primitive_types::U256;

/// A type for representing token transfer amounts.
#[derive(
Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Serialize, Deserialize, Display, From, Into,
)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Display, From, Into)]
pub struct Amount(U256);

impl Amount {
Expand Down
8 changes: 5 additions & 3 deletions crates/ibc/src/applications/transfer/coin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ use core::fmt::{Display, Error as FmtError, Formatter};
use core::str::{from_utf8, FromStr};
use ibc_proto::cosmos::base::v1beta1::Coin as ProtoCoin;
use safe_regex::regex;
use serde::{Deserialize, Serialize};

use super::amount::Amount;
use super::denom::{BaseDenom, PrefixedDenom};
use super::error::TokenTransferError;
use crate::prelude::*;

#[cfg(feature = "serde")]
use crate::serializers::serde_string;

/// A `Coin` type with fully qualified `PrefixedDenom`.
Expand All @@ -19,12 +20,13 @@ pub type BaseCoin = Coin<BaseDenom>;
pub type RawCoin = Coin<String>;

/// Coin defines a token with a denomination and an amount.
#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Serialize, Deserialize)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
pub struct Coin<D> {
/// Denomination
pub denom: D,
/// Amount
#[serde(with = "serde_string")]
#[cfg_attr(feature = "serde", serde(with = "serde_string"))]
pub amount: Amount,
}

Expand Down
28 changes: 14 additions & 14 deletions crates/ibc/src/applications/transfer/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,14 +178,14 @@ pub fn on_chan_open_init(
});
}

if !version.is_empty() && version != &Version::ics20() {
if !version.is_empty() && version != &Version::new(VERSION.to_string()) {
return Err(TokenTransferError::InvalidVersion {
expect_version: Version::ics20(),
expect_version: Version::new(VERSION.to_string()),
got_version: version.clone(),
});
}

Ok((ModuleExtras::empty(), Version::ics20()))
Ok((ModuleExtras::empty(), Version::new(VERSION.to_string())))
}

#[allow(clippy::too_many_arguments)]
Expand All @@ -204,14 +204,14 @@ pub fn on_chan_open_try(
got_order: order,
});
}
if counterparty_version != &Version::ics20() {
if counterparty_version != &Version::new(VERSION.to_string()) {
return Err(TokenTransferError::InvalidCounterpartyVersion {
expect_version: Version::ics20(),
expect_version: Version::new(VERSION.to_string()),
got_version: counterparty_version.clone(),
});
}

Ok((ModuleExtras::empty(), Version::ics20()))
Ok((ModuleExtras::empty(), Version::new(VERSION.to_string())))
}

pub fn on_chan_open_ack(
Expand All @@ -220,9 +220,9 @@ pub fn on_chan_open_ack(
_channel_id: &ChannelId,
counterparty_version: &Version,
) -> Result<ModuleExtras, TokenTransferError> {
if counterparty_version != &Version::ics20() {
if counterparty_version != &Version::new(VERSION.to_string()) {
return Err(TokenTransferError::InvalidCounterpartyVersion {
expect_version: Version::ics20(),
expect_version: Version::new(VERSION.to_string()),
got_version: counterparty_version.clone(),
});
}
Expand Down Expand Up @@ -335,6 +335,7 @@ pub fn on_timeout_packet(

#[cfg(test)]
pub(crate) mod test {
use super::*;
use subtle_encoding::bech32;

use crate::applications::transfer::context::{cosmos_adr028_escrow_address, on_chan_open_try};
Expand All @@ -347,7 +348,6 @@ pub(crate) mod test {
use crate::core::ics04_channel::Version;
use crate::core::ics24_host::identifier::{ChannelId, ConnectionId, PortId};
use crate::handler::HandlerOutputBuilder;
use crate::prelude::*;
use crate::test_utils::{get_dummy_transfer_module, DummyTransferModule};

use super::on_chan_open_init;
Expand Down Expand Up @@ -436,15 +436,15 @@ pub(crate) mod test {
)
.unwrap();

assert_eq!(out_version, Version::ics20());
assert_eq!(out_version, Version::new(VERSION.to_string()));
}

/// If the relayer passed in the only supported version (ics20), then return ics20
#[test]
fn test_on_chan_open_init_ics20_version() {
let (mut ctx, order, connection_hops, port_id, channel_id, counterparty) = get_defaults();

let in_version = Version::ics20();
let in_version = Version::new(VERSION.to_string());
let (_, out_version) = on_chan_open_init(
&mut ctx,
order,
Expand All @@ -456,7 +456,7 @@ pub(crate) mod test {
)
.unwrap();

assert_eq!(out_version, Version::ics20());
assert_eq!(out_version, Version::new(VERSION.to_string()));
}

/// If the relayer passed in an unsupported version, then fail
Expand All @@ -483,7 +483,7 @@ pub(crate) mod test {
fn test_on_chan_open_try_counterparty_correct_version() {
let (mut ctx, order, connection_hops, port_id, channel_id, counterparty) = get_defaults();

let counterparty_version = Version::ics20();
let counterparty_version = Version::new(VERSION.to_string());

let (_, out_version) = on_chan_open_try(
&mut ctx,
Expand All @@ -496,7 +496,7 @@ pub(crate) mod test {
)
.unwrap();

assert_eq!(out_version, Version::ics20());
assert_eq!(out_version, Version::new(VERSION.to_string()));
}

/// If the counterparty doesn't support ics20, then fail
Expand Down
13 changes: 8 additions & 5 deletions crates/ibc/src/applications/transfer/denom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,18 @@ use core::str::FromStr;

use derive_more::{Display, From};
use ibc_proto::ibc::applications::transfer::v1::DenomTrace as RawDenomTrace;
use serde::{Deserialize, Serialize};

use super::error::TokenTransferError;
use crate::core::ics24_host::identifier::{ChannelId, PortId};
use crate::prelude::*;

#[cfg(feature = "serde")]
use crate::serializers::serde_string;

/// Base denomination type
#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Serialize, Deserialize, Display)]
#[serde(transparent)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[cfg_attr(feature = "serde", serde(transparent))]
#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Display)]
pub struct BaseDenom(String);

impl BaseDenom {
Expand Down Expand Up @@ -147,10 +149,11 @@ impl Display for TracePath {
}

/// A type that contains the base denomination for ICS20 and the source tracing information path.
#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Serialize, Deserialize)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
pub struct PrefixedDenom {
/// A series of `{port-id}/{channel-id}`s for tracing the source of the token.
#[serde(with = "serde_string")]
#[cfg_attr(feature = "serde", serde(with = "serde_string"))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub trace_path: TracePath,
/// Base denomination of the relayed fungible token.
pub base_denom: BaseDenom,
Expand Down
9 changes: 6 additions & 3 deletions crates/ibc/src/applications/transfer/packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,17 @@ use core::convert::TryFrom;
use core::str::FromStr;

use ibc_proto::ibc::applications::transfer::v2::FungibleTokenPacketData as RawPacketData;
use serde::{Deserialize, Serialize};

use super::error::TokenTransferError;
use super::{Amount, PrefixedCoin, PrefixedDenom};
use crate::signer::Signer;

#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
#[serde(try_from = "RawPacketData", into = "RawPacketData")]
#[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,
Expand Down
Loading