From 76dd6100a27c2fcc4d0ae94d689eb6bf06a9ad61 Mon Sep 17 00:00:00 2001 From: jamillambert Date: Tue, 3 Jun 2025 10:58:54 +0100 Subject: [PATCH 1/7] Split derivation path test into valid and invalid Make the tests separate to make tests smaller and easier to read. There are no logic changes. --- bitcoin/src/bip32.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 22ed79d7a..8854e28d5 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -1098,7 +1098,7 @@ mod tests { use super::*; #[test] - fn parse_derivation_path() { + fn parse_derivation_path_invalid() { assert!(matches!( "n/0'/0".parse::(), Err(ParseChildNumberError::ParseInt(..)), @@ -1119,7 +1119,10 @@ mod tests { "2147483648".parse::(), Err(ParseChildNumberError::IndexOutOfRange(IndexOutOfRangeError { index: 2147483648 })), ); + } + #[test] + fn parse_derivation_path_valid() { assert_eq!(DerivationPath::master(), "".parse::().unwrap()); assert_eq!(DerivationPath::master(), DerivationPath::default()); From 015fb1be3b74c406ed7b7ff158634f4e6fa6dcef Mon Sep 17 00:00:00 2001 From: jamillambert Date: Tue, 3 Jun 2025 11:11:29 +0100 Subject: [PATCH 2/7] Split invalid derivation path test The test tests two sperate things. Split the test into invalid path and out of range. --- bitcoin/src/bip32.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 8854e28d5..316419ed3 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -1098,7 +1098,7 @@ mod tests { use super::*; #[test] - fn parse_derivation_path_invalid() { + fn parse_derivation_path_invalid_format() { assert!(matches!( "n/0'/0".parse::(), Err(ParseChildNumberError::ParseInt(..)), @@ -1115,6 +1115,10 @@ mod tests { "0h/0x".parse::(), Err(ParseChildNumberError::ParseInt(..)), )); + } + + #[test] + fn parse_derivation_path_out_of_range() { assert_eq!( "2147483648".parse::(), Err(ParseChildNumberError::IndexOutOfRange(IndexOutOfRangeError { index: 2147483648 })), From ed36a980f8270db77fd8226037889328a74e1eae Mon Sep 17 00:00:00 2001 From: jamillambert Date: Tue, 3 Jun 2025 11:13:53 +0100 Subject: [PATCH 3/7] Refactor invalid derivation path tests Refactor the tests to have the invalid paths in a list, or string. --- bitcoin/src/bip32.rs | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 316419ed3..61aacbd3e 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -1099,28 +1099,25 @@ mod tests { #[test] fn parse_derivation_path_invalid_format() { - assert!(matches!( - "n/0'/0".parse::(), - Err(ParseChildNumberError::ParseInt(..)), - )); - assert!(matches!( - "4/m/5".parse::(), - Err(ParseChildNumberError::ParseInt(..)), - )); - assert!(matches!( - "//3/0'".parse::(), - Err(ParseChildNumberError::ParseInt(..)), - )); - assert!(matches!( - "0h/0x".parse::(), - Err(ParseChildNumberError::ParseInt(..)), - )); + let invalid_paths = [ + "n/0'/0", + "4/m/5", + "//3/0'", + "0h/0x", + ]; + for path in &invalid_paths { + assert!(matches!( + path.parse::(), + Err(ParseChildNumberError::ParseInt(..)), + )); + } } #[test] fn parse_derivation_path_out_of_range() { + let invalid_path = "2147483648"; assert_eq!( - "2147483648".parse::(), + invalid_path.parse::(), Err(ParseChildNumberError::IndexOutOfRange(IndexOutOfRangeError { index: 2147483648 })), ); } From 3e7fdad5fd542a8c959039d36a6b09d541982d89 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 3 Jun 2025 11:36:01 +0100 Subject: [PATCH 4/7] Split empty master test out The valid derivation test is doing a whole bunch of things. Split out the empty master path assertions into a separate test. --- bitcoin/src/bip32.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 61aacbd3e..75d460edc 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -1123,15 +1123,20 @@ mod tests { } #[test] - fn parse_derivation_path_valid() { + fn parse_derivation_path_valid_empty_master() { + // Sanity checks. + assert_eq!(DerivationPath::master(), DerivationPath(vec![])); assert_eq!(DerivationPath::master(), "".parse::().unwrap()); assert_eq!(DerivationPath::master(), DerivationPath::default()); - // Acceptable forms for a master path. + // Empty is the same as with an `m`. + assert_eq!("".parse::().unwrap(), DerivationPath(vec![])); assert_eq!("m".parse::().unwrap(), DerivationPath(vec![])); assert_eq!("m/".parse::().unwrap(), DerivationPath(vec![])); - assert_eq!("".parse::().unwrap(), DerivationPath(vec![])); + } + #[test] + fn parse_derivation_path_valid() { assert_eq!("0'".parse::(), Ok(vec![ChildNumber::ZERO_HARDENED].into())); assert_eq!( "0'/1".parse::(), From c5073f4c513636d4feff217986853d6b296524d4 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 3 Jun 2025 11:51:48 +0100 Subject: [PATCH 5/7] Refactor simple valid path tests into a loop The test is still doing a bunch of stuff. Pull the simple test cases into a loop. --- bitcoin/src/bip32.rs | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 75d460edc..e5e1af62a 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -1137,30 +1137,25 @@ mod tests { #[test] fn parse_derivation_path_valid() { - assert_eq!("0'".parse::(), Ok(vec![ChildNumber::ZERO_HARDENED].into())); - assert_eq!( - "0'/1".parse::(), - Ok(vec![ChildNumber::ZERO_HARDENED, ChildNumber::ONE_NORMAL].into()) - ); - assert_eq!( - "0h/1/2'".parse::(), - Ok(vec![ + let valid_paths = [ + ("0'", vec![ChildNumber::ZERO_HARDENED]), + ("0'/1", vec![ChildNumber::ZERO_HARDENED, ChildNumber::ONE_NORMAL]), + ("0h/1/2'", vec![ ChildNumber::ZERO_HARDENED, ChildNumber::ONE_NORMAL, ChildNumber::from_hardened_idx(2).unwrap(), - ] - .into()) - ); - assert_eq!( - "0'/1/2h/2".parse::(), - Ok(vec![ + ]), + ("0'/1/2h/2", vec![ ChildNumber::ZERO_HARDENED, ChildNumber::ONE_NORMAL, ChildNumber::from_hardened_idx(2).unwrap(), ChildNumber::from_normal_idx(2).unwrap(), - ] - .into()) - ); + ]), + ]; + for (path, expected) in valid_paths { + assert_eq!(path.parse::().unwrap(), expected.into()); + } + let want = DerivationPath::from(vec![ ChildNumber::ZERO_HARDENED, ChildNumber::ONE_NORMAL, From 0c9dd31f53fe1b695bcd8322b2fee67cd3bff61f Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 3 Jun 2025 11:55:14 +0100 Subject: [PATCH 6/7] Test with m prefix We have a single test case that tests for the m prefix while all the others do not. Move one test case into the loop and then test on each iteration the path with m prefix added. --- bitcoin/src/bip32.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index e5e1af62a..cce4c999f 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -1151,21 +1151,22 @@ mod tests { ChildNumber::from_hardened_idx(2).unwrap(), ChildNumber::from_normal_idx(2).unwrap(), ]), + ("0'/1/2'/2/1000000000", vec![ + ChildNumber::ZERO_HARDENED, + ChildNumber::ONE_NORMAL, + ChildNumber::from_hardened_idx(2).unwrap(), + ChildNumber::from_normal_idx(2).unwrap(), + ChildNumber::from_normal_idx(1000000000).unwrap(), + ]), ]; for (path, expected) in valid_paths { - assert_eq!(path.parse::().unwrap(), expected.into()); + // Access the inner private field so we don't have to clone expected. + assert_eq!(path.parse::().unwrap().0, expected); + // Test with the leading `m` for good measure. + let prefixed = format!("m/{}", path); + assert_eq!(prefixed.parse::().unwrap().0, expected); } - let want = DerivationPath::from(vec![ - ChildNumber::ZERO_HARDENED, - ChildNumber::ONE_NORMAL, - ChildNumber::from_hardened_idx(2).unwrap(), - ChildNumber::from_normal_idx(2).unwrap(), - ChildNumber::from_normal_idx(1000000000).unwrap(), - ]); - assert_eq!("0'/1/2'/2/1000000000".parse::().unwrap(), want); - assert_eq!("m/0'/1/2'/2/1000000000".parse::().unwrap(), want); - let s = "0'/50/3'/5/545456"; assert_eq!(s.parse::(), s.into_derivation_path()); assert_eq!(s.parse::(), s.to_string().into_derivation_path()); From dd3f3e44bc3329830aacacb252162c57858b0029 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 3 Jun 2025 12:05:03 +0100 Subject: [PATCH 7/7] Split into_derivation_path tests out The test _still_ tests multiple things. Move the `into_derivation_path` calls into a separate test. --- bitcoin/src/bip32.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index cce4c999f..3e7aae74a 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -1166,7 +1166,10 @@ mod tests { let prefixed = format!("m/{}", path); assert_eq!(prefixed.parse::().unwrap().0, expected); } + } + #[test] + fn parse_derivation_path_same_as_into_derivation_path() { let s = "0'/50/3'/5/545456"; assert_eq!(s.parse::(), s.into_derivation_path()); assert_eq!(s.parse::(), s.to_string().into_derivation_path());