Update existing Id::try_from to return a u32 and impl new ones to convert to and from u64 #447
Conversation
.pre-commit-config.yaml
Outdated
There was a problem hiding this comment.
Yeah this is for the pre commit checks, I need to make it easier for other contributors. But for now can you just use the one from main.
Curious are you using the nix flake and it changed the file?
There was a problem hiding this comment.
Yeah I used nix develop as usual and it auto-updated this file.
crates/cdk/src/nuts/nut02.rs
Outdated
| /// [`Id`] to u64 | ||
| pub fn to_u64(&self) -> u64 { | ||
| let bytes = self.to_bytes(); | ||
| let mut array = [0u8; 8]; | ||
| array[..bytes.len()].copy_from_slice(&bytes); | ||
| u64::from_be_bytes(array) | ||
| } |
There was a problem hiding this comment.
I don't think this matches the nut here https://github.com/cashubtc/nuts/blob/main/13.md#keyset-id?
Can we just reuse
cdk/crates/cdk/src/nuts/nut02.rs
Lines 117 to 126 in cc5b267
There was a problem hiding this comment.
Ah, this is a much better idea! Sorry for the garbage PR. I can take care of that test tho.
There was a problem hiding this comment.
Actually, I take it back. The existing TryFrom implementation returns a u64 compressed to a u32 with the modulo function. Shouldn't it return a u32 instead of a u64?
I need functions to convert the 8 byte array to a u64 and back without data loss.
There was a problem hiding this comment.
I'm not sure what the existing TryFrom is used for, or if it is used. I think the simplest fix would be to update it to return a u32 and then write two new impls. Is this a good plan?
impl TryFrom<u64> for Id
impl TryFrom<Id> for u64
There was a problem hiding this comment.
I opened a PR in the nuts repo to clarify the language. cashubtc/nuts#189
crates/cdk/src/nuts/nut02.rs
Outdated
| fn test_id_u64_conversion() { | ||
| let id = generate_random_id(); | ||
| let u64_value = id.to_u64(); | ||
| let converted_id = Id::from_u64(u64_value).unwrap(); | ||
|
|
||
| assert_eq!(id, converted_id); | ||
| assert_eq!(id.version, converted_id.version); | ||
| assert_eq!(id.id, converted_id.id); | ||
| } |
There was a problem hiding this comment.
Can we add a test that it matches the try from
cdk/crates/cdk/src/nuts/nut02.rs
Lines 117 to 126 in cc5b267
vnprc
changed the title
crates/cdk/src/nuts/nut02.rs
Outdated
| assert_eq!(864559728, id_int) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_u64_to_id_and_back_conversion() { |
There was a problem hiding this comment.
This function name is innaccurate and too long. Change it to something like this:
| fn test_u64_to_id_and_back_conversion() { | |
| fn test_to_u64_and_back() { |
|
Crap I merged the latest from master and now the PR shows unrelated file changes. Is there a good way to fix this? |
These functions are very useful integrating cdk into the SRI codebase. Probably other codebases as well.