From 1431acbdbafaeb43de644c5b1b9bc7b0a5a33cb2 Mon Sep 17 00:00:00 2001 From: Christian Gorenflo Date: Mon, 18 Sep 2023 22:56:10 -0400 Subject: [PATCH 1/6] refactor: introduce new Message type that can be deserialized safely --- Cargo.lock | 1 + contracts/connection-router/Cargo.toml | 1 + contracts/connection-router/src/error.rs | 4 +- contracts/connection-router/src/msg.rs | 1 + contracts/connection-router/src/state.rs | 84 ++++++++++++++++++- contracts/connection-router/src/types.rs | 30 +++++-- contracts/connection-router/tests/test.rs | 10 +-- packages/axelar-wasm-std/src/nonempty/mod.rs | 2 + .../axelar-wasm-std/src/nonempty/string.rs | 64 ++++++++++++++ 9 files changed, 180 insertions(+), 17 deletions(-) create mode 100644 packages/axelar-wasm-std/src/nonempty/string.rs diff --git a/Cargo.lock b/Cargo.lock index 0598a4119..854cdeaa1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -752,6 +752,7 @@ dependencies = [ "cw-multi-test", "cw-storage-plus 1.1.0", "cw2 0.15.1", + "error-stack", "flagset", "hex", "rand", diff --git a/contracts/connection-router/Cargo.toml b/contracts/connection-router/Cargo.toml index c8931803f..818593a45 100644 --- a/contracts/connection-router/Cargo.toml +++ b/contracts/connection-router/Cargo.toml @@ -39,6 +39,7 @@ schemars = "0.8.10" serde = { version = "1.0.145", default-features = false, features = ["derive"] } serde_json = "1.0.89" thiserror = { workspace = true } +error-stack = {workspace = true} [dev-dependencies] cw-multi-test = "0.15.1" diff --git a/contracts/connection-router/src/error.rs b/contracts/connection-router/src/error.rs index abbe41e2b..b12c0cbfc 100644 --- a/contracts/connection-router/src/error.rs +++ b/contracts/connection-router/src/error.rs @@ -32,8 +32,8 @@ pub enum ContractError { #[error("chain is frozen")] ChainFrozen { chain: ChainName }, - #[error("address of {0} is invalid")] - InvalidAddress(String), + #[error("address of is invalid")] + InvalidAddress, #[error("source chain does not match registered gateway")] WrongSourceChain, diff --git a/contracts/connection-router/src/msg.rs b/contracts/connection-router/src/msg.rs index ae495e16d..c77dcd152 100644 --- a/contracts/connection-router/src/msg.rs +++ b/contracts/connection-router/src/msg.rs @@ -6,6 +6,7 @@ use crate::types::GatewayDirection; // Message is a type meant to be used in interfaces where the data can be provided by the user. // The fields have not necessarily been validated, and should be checked prior to further processing. #[cw_serde] +#[deprecated(note = "use NewMessage instead")] pub struct Message { pub id: String, // should be globally unique pub source_address: String, diff --git a/contracts/connection-router/src/state.rs b/contracts/connection-router/src/state.rs index 32433c2a0..2c82ec006 100644 --- a/contracts/connection-router/src/state.rs +++ b/contracts/connection-router/src/state.rs @@ -1,8 +1,15 @@ +#![allow(deprecated)] + use core::panic; +use std::ops::Deref; +use std::str::FromStr; use cosmwasm_schema::cw_serde; use cosmwasm_std::{Addr, DepsMut, HexBinary, Order, StdResult}; use cw_storage_plus::{Index, IndexList, IndexedMap, Item, MultiIndex}; +use error_stack::{Report, ResultExt}; + +use axelar_wasm_std::nonempty; use crate::{ msg, @@ -77,6 +84,30 @@ impl<'a> IndexList for ChainEndpointIndexes<'a> { // Message represents a message for which the fields have been successfully validated. // This should never be supplied by the user. #[cw_serde] +pub struct NewMessage { + pub id_on_chain: MessageID, // globally unique + pub destination_address: Address, + pub destination_chain: ChainName, + pub source_chain: ChainName, + pub source_address: Address, + pub payload_hash: HexBinary, +} + +impl NewMessage { + pub fn global_id(&self) -> MessageID { + format!( + "{}{}{}", + &self.source_chain, ID_SEPARATOR, &self.id_on_chain + ) + .try_into() + .expect("a valid source chain and valid id should always produce a valid global message id") + } +} + +// Message represents a message for which the fields have been successfully validated. +// This should never be supplied by the user. +#[cw_serde] +#[deprecated(note = "use NewMessage instead")] pub struct Message { pub id: MessageID, // globally unique pub destination_address: String, @@ -110,11 +141,11 @@ impl TryFrom for Message { type Error = ContractError; fn try_from(value: msg::Message) -> Result { if value.destination_address.is_empty() { - return Err(ContractError::InvalidAddress(value.destination_address)); + return Err(ContractError::InvalidAddress); } if value.source_address.is_empty() { - return Err(ContractError::InvalidAddress(value.source_address)); + return Err(ContractError::InvalidAddress); } if !value @@ -148,14 +179,63 @@ impl From for msg::Message { } } +#[cw_serde] +pub struct Address(nonempty::String); + +impl Deref for Address { + type Target = String; + + fn deref(&self) -> &Self::Target { + self.0.deref() + } +} + +impl FromStr for Address { + type Err = Report; + + fn from_str(s: &str) -> Result { + Address::try_from(s.to_string()) + } +} + +impl TryFrom for Address { + type Error = Report; + + fn try_from(value: String) -> Result { + Ok(Address( + value + .parse::() + .change_context(ContractError::InvalidAddress)?, + )) + } +} + #[cfg(test)] mod tests { + use crate::state::NewMessage; use cosmwasm_std::to_vec; use hex; use sha3::{Digest, Sha3_256}; use super::Message; + #[test] + fn create_correct_global_message_id() { + let msg = NewMessage { + id_on_chain: "hash:id".to_string().parse().unwrap(), + source_chain: "source_chain".to_string().parse().unwrap(), + source_address: "source_address".parse().unwrap(), + destination_chain: "destination_chain".parse().unwrap(), + destination_address: "destination_address".parse().unwrap(), + payload_hash: [1; 32].into(), + }; + + assert_eq!( + msg.global_id().to_string(), + "source_chain:hash:id".to_string() + ); + } + #[test] // Any modifications to the Message struct fields or their types // will cause this test to fail, indicating that a migration is needed. diff --git a/contracts/connection-router/src/types.rs b/contracts/connection-router/src/types.rs index 7b98d2432..2c91fcafa 100644 --- a/contracts/connection-router/src/types.rs +++ b/contracts/connection-router/src/types.rs @@ -1,7 +1,7 @@ use std::fmt; +use std::ops::Deref; use std::str::FromStr; -use axelar_wasm_std::flagset::FlagSet; use cosmwasm_schema::cw_serde; use cosmwasm_std::{Addr, StdError, StdResult}; use cw_storage_plus::{Key, KeyDeserialize, Prefixer, PrimaryKey}; @@ -9,6 +9,8 @@ use flagset::flags; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use axelar_wasm_std::flagset::FlagSet; + use crate::ContractError; pub const ID_SEPARATOR: char = ':'; @@ -21,7 +23,8 @@ impl FromStr for MessageID { type Err = ContractError; fn from_str(s: &str) -> Result { - if !s.contains(ID_SEPARATOR) || s.is_empty() { + // todo: should be exactly 1 when migrated to state::NewMessage + if s.matches(ID_SEPARATOR).count() < 1 { return Err(ContractError::InvalidMessageID); } Ok(MessageID(s.to_lowercase())) @@ -41,9 +44,11 @@ impl From for String { } } -impl<'a> MessageID { - pub fn as_str(&'a self) -> &'a str { - self.0.as_str() +impl Deref for MessageID { + type Target = String; + + fn deref(&self) -> &Self::Target { + &self.0 } } @@ -218,4 +223,19 @@ mod tests { .to_string() ); } + + #[test] + fn message_id_must_have_at_least_one_separator() { + assert!(MessageID::from_str("source_chain:hash:id").is_ok()); + assert!(serde_json::from_str::("\"source_chain:hash:id\"").is_ok()); + + assert!(MessageID::from_str("invalid_hash").is_err()); + assert!(serde_json::from_str::("\"invalid_hash\"").is_err()); + } + + #[test] + fn message_id_is_lower_case() { + let msg_id = "HaSH:iD".parse::().unwrap(); + assert_eq!(msg_id.to_string(), "hash:id"); + } } diff --git a/contracts/connection-router/tests/test.rs b/contracts/connection-router/tests/test.rs index 89cbd59df..18f7f6b1e 100644 --- a/contracts/connection-router/tests/test.rs +++ b/contracts/connection-router/tests/test.rs @@ -262,10 +262,7 @@ fn invalid_address() { &[], ) .unwrap_err(); - assert_eq!( - ContractError::InvalidAddress("".to_string()), - res.downcast().unwrap() - ); + assert_eq!(ContractError::InvalidAddress, res.downcast().unwrap()); let res = config .app @@ -279,10 +276,7 @@ fn invalid_address() { &[], ) .unwrap_err(); - assert_eq!( - ContractError::InvalidAddress("".to_string()), - res.downcast().unwrap() - ); + assert_eq!(ContractError::InvalidAddress, res.downcast().unwrap()); } #[test] diff --git a/packages/axelar-wasm-std/src/nonempty/mod.rs b/packages/axelar-wasm-std/src/nonempty/mod.rs index 8a3a4a8b0..0001b7646 100644 --- a/packages/axelar-wasm-std/src/nonempty/mod.rs +++ b/packages/axelar-wasm-std/src/nonempty/mod.rs @@ -1,9 +1,11 @@ mod error; +mod string; mod timestamp; mod uint; mod vec; pub use error::Error; +pub use string::String; pub use timestamp::Timestamp; pub use uint::{Uint256, Uint64}; pub use vec::Vec; diff --git a/packages/axelar-wasm-std/src/nonempty/string.rs b/packages/axelar-wasm-std/src/nonempty/string.rs new file mode 100644 index 000000000..78e3c9e07 --- /dev/null +++ b/packages/axelar-wasm-std/src/nonempty/string.rs @@ -0,0 +1,64 @@ +use crate::nonempty::Error; +use cosmwasm_schema::cw_serde; +use std::ops::Deref; +use std::str::FromStr; + +#[cw_serde] +#[serde(try_from = "std::string::String")] +pub struct String(std::string::String); + +impl TryFrom for String { + type Error = Error; + + fn try_from(value: std::string::String) -> Result { + if value.is_empty() { + Err(Error::InvalidValue("empty".to_string())) + } else { + Ok(String(value)) + } + } +} + +impl TryFrom<&str> for String { + type Error = Error; + + fn try_from(value: &str) -> Result { + String::try_from(value.to_string()) + } +} + +impl FromStr for String { + type Err = Error; + + fn from_str(s: &str) -> Result { + s.try_into() + } +} + +impl From for std::string::String { + fn from(d: String) -> Self { + d.0 + } +} + +impl Deref for String { + type Target = std::string::String; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +#[cfg(test)] +mod tests { + use crate::nonempty; + + #[test] + fn cannot_convert_from_empty_string() { + assert!(nonempty::String::try_from("").is_err()); + assert!(serde_json::from_str::("\"\"").is_err()); + + assert!(nonempty::String::try_from("some string").is_ok()); + assert!(serde_json::from_str::("\"some string\"").is_ok()); + } +} From 1df3230b86a9d7edaac754b1d29c38479f731b32 Mon Sep 17 00:00:00 2001 From: Christian Gorenflo Date: Tue, 19 Sep 2023 00:17:59 -0400 Subject: [PATCH 2/6] fix formatting --- contracts/connection-router/Cargo.toml | 1 - contracts/connection-router/tests/test.rs | 8 ++------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/contracts/connection-router/Cargo.toml b/contracts/connection-router/Cargo.toml index 602247e7a..699601550 100644 --- a/contracts/connection-router/Cargo.toml +++ b/contracts/connection-router/Cargo.toml @@ -42,7 +42,6 @@ schemars = "0.8.10" serde = { version = "1.0.145", default-features = false, features = ["derive"] } serde_json = "1.0.89" thiserror = { workspace = true } -error-stack = {workspace = true} [dev-dependencies] cw-multi-test = "0.15.1" diff --git a/contracts/connection-router/tests/test.rs b/contracts/connection-router/tests/test.rs index df3959e1b..6bca62424 100644 --- a/contracts/connection-router/tests/test.rs +++ b/contracts/connection-router/tests/test.rs @@ -293,11 +293,9 @@ fn invalid_address() { res.downcast::() .unwrap() .to_string(), - axelar_wasm_std::ContractError::from(ContractError::InvalidAddress) - .to_string() + axelar_wasm_std::ContractError::from(ContractError::InvalidAddress).to_string() ); - let res = config .app .execute_contract( @@ -315,10 +313,8 @@ fn invalid_address() { res.downcast::() .unwrap() .to_string(), - axelar_wasm_std::ContractError::from(ContractError::InvalidAddress) - .to_string() + axelar_wasm_std::ContractError::from(ContractError::InvalidAddress).to_string() ); - } #[test] From ed9e57ec3e63255901b35334ea28d4f2ab757c57 Mon Sep 17 00:00:00 2001 From: Christian Gorenflo Date: Tue, 19 Sep 2023 04:20:05 -0400 Subject: [PATCH 3/6] lower linting severity of deprecated code --- .github/workflows/basic.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/basic.yaml b/.github/workflows/basic.yaml index 511048c4f..a4684573f 100644 --- a/.github/workflows/basic.yaml +++ b/.github/workflows/basic.yaml @@ -8,7 +8,7 @@ on: - releases/** env: - RUSTFLAGS: -D warnings --cfg tracing_unstable + RUSTFLAGS: -D warnings -W deprecated --cfg tracing_unstable name: Basic From c1b054090eaeb03191429f73e899c4474fe4b7c6 Mon Sep 17 00:00:00 2001 From: Christian Gorenflo Date: Tue, 19 Sep 2023 04:25:23 -0400 Subject: [PATCH 4/6] Update .github/workflows/basic.yaml --- .github/workflows/basic.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/basic.yaml b/.github/workflows/basic.yaml index a4684573f..564094491 100644 --- a/.github/workflows/basic.yaml +++ b/.github/workflows/basic.yaml @@ -8,7 +8,7 @@ on: - releases/** env: - RUSTFLAGS: -D warnings -W deprecated --cfg tracing_unstable + RUSTFLAGS: -D warnings -A deprecated --cfg tracing_unstable name: Basic From b147a83dd206be166edc72484fe6ea146f664099 Mon Sep 17 00:00:00 2001 From: Christian Gorenflo Date: Tue, 19 Sep 2023 04:32:19 -0400 Subject: [PATCH 5/6] fix lint --- .github/workflows/basic.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/basic.yaml b/.github/workflows/basic.yaml index a4684573f..e503cd041 100644 --- a/.github/workflows/basic.yaml +++ b/.github/workflows/basic.yaml @@ -67,7 +67,7 @@ jobs: command: wasm args: --locked --workspace --exclude ampd env: - RUSTFLAGS: -D warnings --cfg tracing_unstable -C link-arg=-s + RUSTFLAGS: -D warnings -A deprecated --cfg tracing_unstable -C link-arg=-s lints: name: Lints From 6f4126f9a469a5e5dbb5f27d3eba27f3a21e7d09 Mon Sep 17 00:00:00 2001 From: Christian Gorenflo Date: Tue, 19 Sep 2023 04:33:23 -0400 Subject: [PATCH 6/6] Update contracts/connection-router/src/error.rs --- contracts/connection-router/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/connection-router/src/error.rs b/contracts/connection-router/src/error.rs index 819a81f09..84914bf29 100644 --- a/contracts/connection-router/src/error.rs +++ b/contracts/connection-router/src/error.rs @@ -33,7 +33,7 @@ pub enum ContractError { #[error("chain is frozen")] ChainFrozen { chain: ChainName }, - #[error("address of is invalid")] + #[error("address is invalid")] InvalidAddress, #[error("source chain does not match registered gateway")]