Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update existing Id::try_from to return a u32 and impl new ones to convert to and from u64 #447

Closed
wants to merge 9 commits into from
Closed

Update existing Id::try_from to return a u32 and impl new ones to convert to and from u64 #447

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
09a8bd3
feat: functions to convert keyset ID to and from u64
vnprc Sep 11, 2024
9f0951b
test: ID::to_u64 and ID::from_u64
vnprc Nov 10, 2024
a34d93b
fix: return u32 from existing Id::TryFrom and add lossless u64 versions
vnprc Nov 12, 2024
91208e6
fix: revert change to .pre-commit-config.yaml
vnprc Nov 12, 2024
d23733c
fix: Send current state of the subscriptions (#444)
crodas Nov 10, 2024
072dd71
chore: rust fmt
thesimplekid Nov 10, 2024
9801741
chore: nut17 import oordering
thesimplekid Nov 10, 2024
1012f46
multiple active keysets and return active keyset with lowest fee (#448)
mubarak23 Nov 12, 2024
efda2aa
rename test function to be more concise and accurate
vnprc Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I used nix develop as usual and it auto-updated this file.

Original file line number Diff line number Diff line change
@@ -1 +1 @@
/nix/store/9bf8g8scpkrma0rwv05b4bd1qc81gihg-pre-commit-config.json
/nix/store/v35hg96mpn8sa4i4vk9qm1f4jdyb59yj-pre-commit-config.json
25 changes: 25 additions & 0 deletions crates/cdk/src/nuts/nut02.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,20 @@ impl Id {
id: bytes[1..].try_into()?,
})
}

/// [`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)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

impl TryFrom<Id> for u64 {
type Error = Error;
fn try_from(value: Id) -> Result<Self, Self::Error> {
let hex_bytes: [u8; 8] = value.to_bytes().try_into().map_err(|_| Error::Length)?;
let int = u64::from_be_bytes(hex_bytes);
Ok(int % (2_u64.pow(31) - 1))
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is a much better idea! Sorry for the garbage PR. I can take care of that test tho.

Copy link
Contributor Author

@vnprc vnprc Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a PR in the nuts repo to clarify the language. cashubtc/nuts#189


/// [`Id`] from u64
pub fn from_u64(value: u64) -> Result<Self, Error> {
let bytes = value.to_be_bytes();
Self::from_bytes(&bytes)
}
}

impl TryFrom<Id> for u64 {
Expand Down Expand Up @@ -547,4 +561,15 @@ mod test {
let id_from_uppercase = Id::from_str(&SHORT_KEYSET_ID.to_uppercase());
assert!(id_from_uppercase.is_ok());
}

#[test]
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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test that it matches the try from

cdk/crates/cdk/src/nuts/nut02.rs

Lines 117 to 126 in cc5b267

impl TryFrom<Id> for u64 {
type Error = Error;
fn try_from(value: Id) -> Result<Self, Self::Error> {
let hex_bytes: [u8; 8] = value.to_bytes().try_into().map_err(|_| Error::Length)?;
let int = u64::from_be_bytes(hex_bytes);
Ok(int % (2_u64.pow(31) - 1))
}
}

}
Loading