From df2145592dd82c731b46896c8134145f6d303099 Mon Sep 17 00:00:00 2001 From: kpop-dfinity <125868903+kpop-dfinity@users.noreply.github.com> Date: Fri, 10 Jan 2025 13:11:17 +0100 Subject: [PATCH] test(consensus): move the `IngressPayload` serialization/deserialization unit test from `test_utilities/types/` to `rs/types/types` (#3384) It makes little sense to me to have unit tests for `IngressPayload` in a test utilities crate Note: to avoid cyclic dependency I'm not using `SignedIngressBuilder` to construct a `SignedIngress` --- Cargo.lock | 1 - rs/test_utilities/types/BUILD.bazel | 1 - rs/test_utilities/types/Cargo.toml | 1 - .../types/src/batch/ingress_payload.rs | 74 --------------- rs/types/types/src/batch/ingress.rs | 95 ++++++++++++++++--- 5 files changed, 82 insertions(+), 90 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 224642148bf..6adacd63a50 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13061,7 +13061,6 @@ dependencies = [ name = "ic-test-utilities-types" version = "0.9.0" dependencies = [ - "assert_matches", "bincode", "ic-canister-client-sender", "ic-crypto-ed25519", diff --git a/rs/test_utilities/types/BUILD.bazel b/rs/test_utilities/types/BUILD.bazel index 7a9748adc93..f5d2a16e788 100644 --- a/rs/test_utilities/types/BUILD.bazel +++ b/rs/test_utilities/types/BUILD.bazel @@ -25,7 +25,6 @@ rust_test( crate = ":types", deps = [ # Keep sorted. - "@crate_index//:assert_matches", "@crate_index//:bincode", "@crate_index//:serde_cbor", ], diff --git a/rs/test_utilities/types/Cargo.toml b/rs/test_utilities/types/Cargo.toml index 300abf61004..bbac97ba3da 100644 --- a/rs/test_utilities/types/Cargo.toml +++ b/rs/test_utilities/types/Cargo.toml @@ -14,6 +14,5 @@ ic-types-test-utils = { path = "../../types/types_test_utils" } rand = { workspace = true } [dev-dependencies] -assert_matches = { workspace = true } bincode = { workspace = true } serde_cbor = { workspace = true } diff --git a/rs/test_utilities/types/src/batch/ingress_payload.rs b/rs/test_utilities/types/src/batch/ingress_payload.rs index f858ae6542e..bef65d6638a 100644 --- a/rs/test_utilities/types/src/batch/ingress_payload.rs +++ b/rs/test_utilities/types/src/batch/ingress_payload.rs @@ -36,77 +36,3 @@ impl IngressPayloadBuilder { self.ingress_payload.into() } } - -#[cfg(test)] -mod tests { - use super::*; - use crate::messages::SignedIngressBuilder; - use assert_matches::assert_matches; - use ic_types::{batch::IngressPayloadError, time::expiry_time_from_now}; - use std::convert::TryFrom; - use std::time::Duration; - - #[test] - fn test_ingress_payload_deserialization() { - // serialization/deserialization of empty payload. - let payload = IngressPayload::default(); - let bytes = bincode::serialize(&payload).unwrap(); - assert_eq!( - bincode::deserialize::(&bytes).unwrap(), - payload - ); - let time = expiry_time_from_now(); - - // Some test messages. - let m1 = SignedIngressBuilder::new() - .method_name("m1".to_string()) - .expiry_time(time + Duration::from_secs(1)) - .build(); - let m1_id = m1.id(); - let m2 = SignedIngressBuilder::new() - .method_name("m2".to_string()) - .expiry_time(time + Duration::from_secs(2)) - .build(); - let m3 = SignedIngressBuilder::new() - .method_name("m3".to_string()) - .expiry_time(time + Duration::from_secs(3)) - .build(); - - let msgs = vec![m1, m2, m3]; - let payload = IngressPayload::from(msgs.clone()); - // Serialization/deserialization works. - let mut bytes = bincode::serialize(&payload).unwrap(); - assert_eq!( - bincode::deserialize::(&bytes).unwrap(), - payload - ); - // Individual lookup works. - assert_matches!(payload.get(0).unwrap(), (_, msg) if msg == msgs[0]); - assert_matches!(payload.get(1).unwrap(), (_, msg) if msg == msgs[1]); - assert_matches!(payload.get(2).unwrap(), (_, msg) if msg == msgs[2]); - // Test IndexOutOfBound. - assert_matches!(payload.get(3), Err(IngressPayloadError::IndexOutOfBound(3))); - // Converting back to messages should match original - assert_eq!(msgs, >::try_from(payload).unwrap()); - - // A sub-sequence search function - fn find(array: &[u8], subseq: &[u8]) -> Option { - (0..array.len() - subseq.len() + 1).find(|&i| array[i..i + subseq.len()] == subseq[..]) - } - - // Mutate some byte, deserialization works, but casting back to messages fail. - let pos = find(&bytes, m1_id.as_bytes()).unwrap(); - // `+= 1` may overflow in debug mode. - bytes[pos] ^= 1; - let payload = bincode::deserialize::(&bytes); - assert!(payload.is_ok()); - let payload = payload.unwrap(); - // get(0) should return error. - assert_matches!( - payload.get(0), - Err(IngressPayloadError::MismatchedMessageIdAtIndex(0)) - ); - // Conversion should also fail. - assert!(>::try_from(payload).is_err()); - } -} diff --git a/rs/types/types/src/batch/ingress.rs b/rs/types/types/src/batch/ingress.rs index 3bfcdafadaf..467136fe982 100644 --- a/rs/types/types/src/batch/ingress.rs +++ b/rs/types/types/src/batch/ingress.rs @@ -204,41 +204,45 @@ mod tests { }, time::expiry_time_from_now, }; + use assert_matches::assert_matches; + use std::convert::TryFrom; - /// Build a Vec. Convert to IngressPayload and then back to - /// Vec. Ensure that the two vectors are identical. - #[test] - fn into_ingress_payload_and_back() { - let ingress_expiry = expiry_time_from_now(); - let content = HttpCallContent::Call { + fn fake_http_call_content(method_name: &str) -> HttpCallContent { + HttpCallContent::Call { update: HttpCanisterUpdate { canister_id: Blob(vec![42; 8]), - method_name: "some_method".to_string(), + method_name: method_name.to_string(), arg: Blob(b"".to_vec()), sender: Blob(vec![0x05]), nonce: Some(Blob(vec![1, 2, 3, 4])), - ingress_expiry: ingress_expiry.as_nanos_since_unix_epoch(), + ingress_expiry: expiry_time_from_now().as_nanos_since_unix_epoch(), }, - }; + } + } + + /// Build a Vec. Convert to IngressPayload and then back to + /// Vec. Ensure that the two vectors are identical. + #[test] + fn into_ingress_payload_and_back() { let update_messages = vec![ HttpRequestEnvelope:: { - content: content.clone(), + content: fake_http_call_content("1"), sender_pubkey: Some(Blob(vec![2; 32])), sender_sig: Some(Blob(vec![1; 32])), sender_delegation: None, }, HttpRequestEnvelope:: { - content: content.clone(), + content: fake_http_call_content("2"), sender_pubkey: None, sender_sig: None, sender_delegation: None, }, HttpRequestEnvelope:: { - content, + content: fake_http_call_content("3"), sender_pubkey: Some(Blob(vec![2; 32])), sender_sig: Some(Blob(vec![1; 32])), sender_delegation: Some(vec![SignedDelegation::new( - Delegation::new(vec![1, 2], ingress_expiry), + Delegation::new(vec![1, 2], expiry_time_from_now()), vec![3, 4], )]), }, @@ -251,4 +255,69 @@ mod tests { let signed_ingresses1 = Vec::::try_from(ingress_payload).unwrap(); assert_eq!(signed_ingresses, signed_ingresses1); } + + #[test] + fn test_ingress_payload_deserialization() { + // serialization/deserialization of empty payload. + let payload = IngressPayload::default(); + let bytes = bincode::serialize(&payload).unwrap(); + assert_eq!( + bincode::deserialize::(&bytes).unwrap(), + payload + ); + + let fake_ingress_message = |method_name| { + let message = HttpRequestEnvelope:: { + content: fake_http_call_content(method_name), + sender_pubkey: None, + sender_sig: None, + sender_delegation: None, + }; + + SignedIngress::try_from(message).unwrap() + }; + + // Some test messages. + let m1 = fake_ingress_message("m1"); + let m1_id = m1.id(); + let m2 = fake_ingress_message("m2"); + let m3 = fake_ingress_message("m3"); + + let msgs = vec![m1, m2, m3]; + let payload = IngressPayload::from(msgs.clone()); + // Serialization/deserialization works. + let mut bytes = bincode::serialize(&payload).unwrap(); + assert_eq!( + bincode::deserialize::(&bytes).unwrap(), + payload + ); + // Individual lookup works. + assert_matches!(payload.get(0).unwrap(), (_, msg) if msg == msgs[0]); + assert_matches!(payload.get(1).unwrap(), (_, msg) if msg == msgs[1]); + assert_matches!(payload.get(2).unwrap(), (_, msg) if msg == msgs[2]); + // Test IndexOutOfBound. + assert_matches!(payload.get(3), Err(IngressPayloadError::IndexOutOfBound(3))); + // Converting back to messages should match original + assert_eq!(msgs, >::try_from(payload).unwrap()); + + // A sub-sequence search function + fn find(array: &[u8], subseq: &[u8]) -> Option { + (0..array.len() - subseq.len() + 1).find(|&i| array[i..i + subseq.len()] == subseq[..]) + } + + // Mutate some byte, deserialization works, but casting back to messages fail. + let pos = find(&bytes, m1_id.as_bytes()).unwrap(); + // `+= 1` may overflow in debug mode. + bytes[pos] ^= 1; + let payload = bincode::deserialize::(&bytes); + assert!(payload.is_ok()); + let payload = payload.unwrap(); + // get(0) should return error. + assert_matches!( + payload.get(0), + Err(IngressPayloadError::MismatchedMessageIdAtIndex(0)) + ); + // Conversion should also fail. + assert!(>::try_from(payload).is_err()); + } }