-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat: Rewrite request ID calculator #478
Conversation
This PR removes a lot of comments - are they no longer applicable with the new changes? |
The Request ID example in interface-spec has changed. (Two extra fields) #[test]
fn public_spec_example() {
#[derive(Serialize)]
struct PublicSpecExampleStruct {
request_type: &'static str,
sender: Principal,
ingress_expiry: u64,
canister_id: Principal,
method_name: &'static str,
#[serde(with = "serde_bytes")]
arg: Vec<u8>,
}
let data = PublicSpecExampleStruct {
request_type: "call",
sender: Principal::anonymous(),
ingress_expiry: 1685570400000000000u64,
canister_id: Principal::try_from(&vec![0, 0, 0, 0, 0, 0, 0x04, 0xD2]).unwrap(), // 1234 in u64
method_name: "hello",
arg: b"DIDL\x00\xFD*".to_vec(),
};
// Hash taken from the example on the public spec.
let request_id = to_request_id(&data).unwrap();
assert_eq!(
hex::encode(request_id.0),
"1d1091364d6bb8a6c16b203ee75467d59ead468f523eb058880ae8ec80e2b101"
);
} The implementation looks great. And the comments are sufficient. As Eric suggested, more tests will be great for coverage. The existing test vectors were generated with |
@krpeacock Either no longer applicable, capable of being made more concise by locating the information closer to the code they actually document, or were originally copied verbatim from Serializer's docs and thus pointless. |
This PR allows request IDs to include nested maps, which necessitated a rewrite of the underlying algorithm. Required for replica-signed queries.