Skip to content
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

Remove regex requirement #876

Merged
merged 23 commits into from
Sep 27, 2023
Merged
Show file tree
Hide file tree
Changes from 18 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: 2 additions & 0 deletions .changelog/unreleased/improvements/875-remove-regex-dep.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Remove `safe-regex` dependency
([\#875](https://github.com/cosmos/ibc-rs/issues/875))
1 change: 0 additions & 1 deletion crates/ibc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ serde_json = { version = "1", default-features = false, 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 }
safe-regex = { version = "0.2.5", default-features = false }
subtle-encoding = { version = "0.5", default-features = false }
sha2 = { version = "0.10.6", default-features = false }
displaydoc = { version = "0.2", default-features = false }
Expand Down
144 changes: 84 additions & 60 deletions crates/ibc/src/applications/transfer/coin.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
//! Defines coin types; the objects that are being transferred.

use core::fmt::{Display, Error as FmtError, Formatter};
use core::str::{from_utf8, FromStr};
use core::str::FromStr;

use ibc_proto::cosmos::base::v1beta1::Coin as ProtoCoin;
use safe_regex::regex;

use super::amount::Amount;
use super::denom::{BaseDenom, PrefixedDenom};
Expand Down Expand Up @@ -38,6 +37,19 @@ pub struct Coin<D> {
pub amount: Amount,
}

impl<D> Coin<D> {
pub fn new<T, DT>(amount: T, denom: DT) -> Self
where
T: Into<Amount>,
DT: Into<D>,
Farhad-Shabani marked this conversation as resolved.
Show resolved Hide resolved
{
Self {
denom: denom.into(),
amount: amount.into(),
}
}
}

impl<D: FromStr> Coin<D>
where
D::Err: Into<TokenTransferError>,
Expand All @@ -59,24 +71,30 @@ where
// a letter, a number or a separator ('/', ':', '.', '_' or '-').
// Loosely copy the regex from here:
// https://github.com/cosmos/cosmos-sdk/blob/v0.45.5/types/coin.go#L760-L762
rnbguy marked this conversation as resolved.
Show resolved Hide resolved
let matcher = regex!(br"([0-9]+)([a-zA-Z0-9/:\\._\x2d]+)");

let (m1, m2) = matcher.match_slices(coin_str.as_bytes()).ok_or_else(|| {
TokenTransferError::InvalidCoin {
//
// equivalent regex code in rust:
// let re = Regex::new(r"^(?<amount>[0-9]+)(?<denom>[a-zA-Z0-9/:._-]+)$")?;
// let cap = re.captures("123stake")?;
// let (amount, denom) = (cap.name("amount")?.as_str(), cap.name("denom")?.as_str());

let (amount, denom) = coin_str
.chars()
.position(|x| !x.is_numeric())
.map(|index| coin_str.split_at(index))
.filter(|(amount, denom)| !amount.is_empty() && !denom.is_empty())
.filter(|(_, denom)| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think filter means that you will exclude the invalid characters, but the result would be valid. In the original case, invalid characters will cause the method to fail.

denom.chars().all(|x| {
matches!(x, 'a'..='z' | 'A'..='Z' | '0'..='9' | '/' | ':' | '.' | '_' | '-')
})
})
Farhad-Shabani marked this conversation as resolved.
Show resolved Hide resolved
.ok_or_else(|| TokenTransferError::InvalidCoin {
coin: coin_str.to_string(),
}
})?;

let amount = from_utf8(m1)
.map_err(TokenTransferError::Utf8Decode)?
.parse()?;
})?;

let denom = from_utf8(m2)
.map_err(TokenTransferError::Utf8Decode)?
.parse()
.map_err(Into::into)?;

Ok(Coin { amount, denom })
Ok(Coin {
amount: amount.parse()?,
denom: denom.parse().map_err(Into::into)?,
})
}
}

Expand Down Expand Up @@ -119,53 +137,59 @@ impl<D: Display> Display for Coin<D> {

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_parse_raw_coin() -> Result<(), TokenTransferError> {
{
let coin = RawCoin::from_str("123stake")?;
assert_eq!(coin.denom, "stake");
assert_eq!(coin.amount, 123u64.into());
}

{
let coin = RawCoin::from_str("1a1")?;
assert_eq!(coin.denom, "a1");
assert_eq!(coin.amount, 1u64.into());
}
use rstest::rstest;

{
let coin = RawCoin::from_str("0x1/:.\\_-")?;
assert_eq!(coin.denom, "x1/:.\\_-");
assert_eq!(coin.amount, 0u64.into());
}
use super::*;

{
// `!` is not allowed
let res = RawCoin::from_str("0x!");
assert!(res.is_err());
}
#[rstest]
rnbguy marked this conversation as resolved.
Show resolved Hide resolved
#[case::nat("123stake", RawCoin::new(123, "stake"))]
#[case::zero("0stake", RawCoin::new(0, "stake"))]
#[case::u256_max(
"115792089237316195423570985008687907853269984665640564039457584007913129639935stake",
RawCoin::new(
[u64::MAX; 4],
"stake"
)
)]
#[case::digit_in_denom("1a1", RawCoin::new(1, "a1"))]
#[case::chars_in_denom("0x1/:._-", RawCoin::new(0, "x1/:._-"))]
#[case::ibc_denom("1234ibc/a0B1C", RawCoin::new(1234, "ibc/a0B1C"))]
fn test_parse_raw_coin(#[case] parsed: RawCoin, #[case] expected: RawCoin) {
assert_eq!(parsed, expected);
}

#[rstest]
#[case::pos("+123stake")]
#[case::pos_zero("+0stake")]
#[case::neg("-123stake")]
#[case::neg_zero("-0stake")]
#[case::u256_max_plus_1(
"115792089237316195423570985008687907853269984665640564039457584007913129639936stake"
)]
#[case::invalid_char_in_denom("0x!")]
#[case::blackslash_in_denom("0x1/:.\\_-")]
#[should_panic]
fn test_failed_parse_raw_coin(#[case] _raw: RawCoin) {}

#[rstest]
#[case::nomal("123stake,1a1,999den0m", &[RawCoin::new(123, "stake"), RawCoin::new(1, "a1"), RawCoin::new(999, "den0m")])]
#[case::tricky("123stake,1a1-999den0m", &[RawCoin::new(123, "stake"), RawCoin::new(1, "a1-999den0m")])]
#[case::colon_delimiter("123stake:1a1:999den0m", &[RawCoin::new(123, "stake:1a1:999den0m")])]
#[case::dash_delimiter("123stake-1a1-999den0m", &[RawCoin::new(123, "stake-1a1-999den0m")])]
#[case::slash_delimiter("123stake/1a1/999den0m", &[RawCoin::new(123, "stake/1a1/999den0m")])]
fn test_parse_raw_coin_list(
#[case] coins_str: &str,
#[case] coins: &[RawCoin],
) -> Result<(), TokenTransferError> {
assert_eq!(RawCoin::from_string_list(coins_str)?, coins);
Ok(())
}

#[test]
fn test_parse_raw_coin_list() -> Result<(), TokenTransferError> {
{
let coins = RawCoin::from_string_list("123stake,1a1,999den0m")?;
assert_eq!(coins.len(), 3);

assert_eq!(coins[0].denom, "stake");
assert_eq!(coins[0].amount, 123u64.into());

assert_eq!(coins[1].denom, "a1");
assert_eq!(coins[1].amount, 1u64.into());

assert_eq!(coins[2].denom, "den0m");
assert_eq!(coins[2].amount, 999u64.into());
}

Ok(())
#[rstest]
#[case::semicolon_delimiter("123stake;1a1;999den0m")]
#[case::mixed_delimiter("123stake,1a1;999den0m")]
#[should_panic(expected = "parsing failure in test")]
fn test_failed_parse_raw_coin_list(#[case] coins_str: &str) {
RawCoin::from_string_list(coins_str).expect("parsing failure in test");
}
}