Skip to content

Commit 5784770

Browse files
authored
Refactor Height parsing logic and add test cases (#758)
* Refactor Height parsing logic and add test cases * Create 752-fix-752.md * Refactor error message for invalid height format * Update height.rs * Refactored height parsing logic and added tests * Refactor error message for invalid height format * Refactor variable name for consistency in `Height` conversion
1 parent bd2c1e6 commit 5784770

File tree

2 files changed

+59
-4
lines changed

2 files changed

+59
-4
lines changed
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
- Height::from_str accepts invalid heights
2+
([#752](https://github.com/cosmos/ibc-rs/issues/752))

crates/ibc/src/core/ics02_client/height.rs

Lines changed: 57 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ impl core::fmt::Display for Height {
150150
}
151151

152152
/// Encodes all errors related to chain heights
153-
#[derive(Debug, Display)]
153+
#[derive(Debug, Display, PartialEq)]
154154
pub enum HeightError {
155155
/// cannot convert into a `Height` type from string `{height}`
156156
HeightConversion {
@@ -159,6 +159,8 @@ pub enum HeightError {
159159
},
160160
/// attempted to parse an invalid zero height
161161
ZeroHeight,
162+
/// the height(`{raw_height}`) is not valid format, this format must be used: \[revision_number\]-\[revision_height\]
163+
InvalidFormat { raw_height: String },
162164
}
163165

164166
#[cfg(feature = "std")]
@@ -167,6 +169,7 @@ impl std::error::Error for HeightError {
167169
match &self {
168170
HeightError::HeightConversion { error: e, .. } => Some(e),
169171
HeightError::ZeroHeight => None,
172+
HeightError::InvalidFormat { .. } => None,
170173
}
171174
}
172175
}
@@ -175,17 +178,25 @@ impl TryFrom<&str> for Height {
175178
type Error = HeightError;
176179

177180
fn try_from(value: &str) -> Result<Self, Self::Error> {
178-
let split: Vec<&str> = value.split('-').collect();
181+
let (rev_number_str, rev_height_str) = match value.split_once('-') {
182+
Some((rev_number_str, rev_height_str)) => (rev_number_str, rev_height_str),
183+
None => {
184+
return Err(HeightError::InvalidFormat {
185+
raw_height: value.to_owned(),
186+
})
187+
}
188+
};
179189

180190
let revision_number =
181-
split[0]
191+
rev_number_str
182192
.parse::<u64>()
183193
.map_err(|e| HeightError::HeightConversion {
184194
height: value.to_owned(),
185195
error: e,
186196
})?;
197+
187198
let revision_height =
188-
split[1]
199+
rev_height_str
189200
.parse::<u64>()
190201
.map_err(|e| HeightError::HeightConversion {
191202
height: value.to_owned(),
@@ -209,3 +220,45 @@ impl FromStr for Height {
209220
Height::try_from(s)
210221
}
211222
}
223+
224+
#[test]
225+
fn test_valid_height() {
226+
assert_eq!(
227+
"1-1".parse::<Height>(),
228+
Ok(Height {
229+
revision_number: 1,
230+
revision_height: 1
231+
})
232+
);
233+
assert_eq!(
234+
"1-10".parse::<Height>(),
235+
Ok(Height {
236+
revision_number: 1,
237+
revision_height: 10
238+
})
239+
);
240+
}
241+
242+
#[test]
243+
fn test_invalid_height() {
244+
assert_eq!(
245+
HeightError::ZeroHeight,
246+
"0-0".parse::<Height>().unwrap_err()
247+
);
248+
assert!("0-".parse::<Height>().is_err());
249+
assert!("-0".parse::<Height>().is_err());
250+
assert!("-".parse::<Height>().is_err());
251+
assert!("1-1-1".parse::<Height>().is_err());
252+
assert_eq!(
253+
"1".parse::<Height>(),
254+
Err(HeightError::InvalidFormat {
255+
raw_height: "1".to_owned()
256+
})
257+
);
258+
assert_eq!(
259+
"".parse::<Height>(),
260+
Err(HeightError::InvalidFormat {
261+
raw_height: "".to_owned()
262+
})
263+
);
264+
}

0 commit comments

Comments
 (0)