Update Id::try_from to return a u32 and remove redundant logic in nut13::derive_path_from_keyset_id #452

vnprc · 2024-11-13T02:58:39Z

Recreating this PR without including other contributors' changes in the diff.

The existing TryFrom<Id> trait implementation returns a u64 but can only contain values that fit within a u32 according to the nut13 protocol definition. This PR changes the existing implementation to return a u32 and defines two new TryFrom trait implementations that can be used to convert a keyset ID to and from a u64 with no data loss.

I have also opened a PR to clarify the language in NUT13 here.

thesimplekid · 2024-11-13T15:21:58Z

crates/cdk/src/nuts/nut02.rs

+impl TryFrom<u64> for Id {
+    type Error = Error;
+    fn try_from(value: u64) -> Result<Self, Self::Error> {
+        let bytes = value.to_be_bytes();
+        Self::from_bytes(&bytes)
+    }
+}
+
+impl TryFrom<Id> for u64 {
+    type Error = Error;
+
+    fn try_from(value: Id) -> Result<Self, Self::Error> {
+        let bytes = value.to_bytes();
+        let byte_array: [u8; 8] = bytes.try_into().map_err(|_| Error::Length)?;
+        Ok(u64::from_be_bytes(byte_array))


I'm hesitant to put in these u64 conversion and have the u32 one that does the module as defined in the nut. I think it maybe best to change the current TryFrom<Id> for u64 to TryFrom<Id> for u32 as you've done but to leave the u64 conversion out. Converting to and from u64 can be done outside cdk if thats needed for something as cdk should be kept to whats defined in the nuts.

I see now I broke some other stuff with this change to u32. It's definitely good to isolate this change.

don't merge, nut13 unit test incoming...

vnprc · 2024-11-13T17:07:31Z

I wrote a test for the nut13 functionality and ran it on the commit where it was broken...and it passed! Turns out we were reducing the keyset ID to 32 bits twice, once in the TryFrom impl and again in derive_path_from_keyset_id. Now with a unit test we can be sure that deriving the path works correctly and only reduce the keyset to u32 once.

PS I'm having to use --no-verify to commit. Not sure how to fix it but I can run all the checks using just.

vnprc · 2024-11-13T17:12:15Z

Not sure I agree with you about leaving out the u64 TryFrom impl but you are the captain. Disagree and commit. 🫡
https://en.wikipedia.org/wiki/Disagree_and_commit

thesimplekid · 2024-11-13T18:11:53Z

Turns out we were reducing the keyset ID to 32 bits twice, once in the TryFrom impl and again in derive_path_from_keyset_id

~~Yeah I noticed that, I'm gonna write some full integration tests deriving from seed to make sure we haven't changed that before we merge this.~~

Already a test for this.

Not sure I agree with you about leaving out the u64 TryFrom impl but you are the captain. Disagree and commit. 🫡

Happy to hear a counter argument.

PS I'm having to use --no-verify to commit. Not sure how to fix it but I can run all the checks using just.

I know precommit still giving issues going to fix that here #451

thesimplekid · 2024-11-14T12:46:44Z

Noticed while we're here we can actually make it a From instead of a TryFrom if we add that

    pub fn as_bytes(&self) -> [u8; Self::BYTELEN + 1] {
        let mut bytes = [0u8; Self::BYTELEN + 1];
        bytes[0] = self.version.to_byte();
        bytes[1..].copy_from_slice(&self.id);
        bytes
    }

to

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

Line 107 in f3ae4f4

.

Can do this as a different pr if you don't want to include it here.

vnprc · 2024-11-14T22:19:18Z

Nice!

Happy to hear a counter argument.

If we strictly limit cdk to what is in the specs then we leave out nice-to-have features like this because this is an implementation detail that doesn't belong in the spec.

I think converting 8 bytes to a u64 is a useful little helper function. You are correct that it's not a big deal to implement outside of cdk but this decision pushes more complexity up the stack. I think the point of this project is to push as much implementation complexity as we can down stack to free up higher level devs to focus on other problems, enabling them to build more cool shit. To me, it feels like it belongs in a 'dev kit' library.

OTOH I'm new to cdk, new to rust, and I've never contributed to a low level library, so what do I know? I just have my engineering intuition to rely on. It's a really minor thing at this point, not worth a drawn out discussion.

Another potential reason not to implement From<Id> for u64 and it's inverse is that it could create confusion with From<Id> for u32. This is best solved with code documentation, IMO.

thesimplekid · 2024-11-15T13:01:17Z

Another potential reason not to implement From for u64 and it's inverse is that it could create confusion with From for u32. This is best solved with code documentation, IMO.

This is my issue with it - not that it's not in the spec but that it's potentially in conflict with what is. If someone has a u32 they may expect they can safely convert it to u64 to do From for Id, but that conversion path isn't actually safe since they can't know if data was lost when converting from Id to u32 via From for u32. Having these functions is too likely to lead to potential bugs, and adding docs isn't a reliable safeguard as documentation can be easily overlooked.

While I understand the desire to reduce complexity for higher-level developers and that is the goal of cdk, in this case the risk of subtle bugs outweighs the benefit. Even though implementing this trivial conversion outside cdk requires more work, it's preferable to having potentially unsafe implicit conversions in the library itself. This is especially important for a low-level library where predictability and safety should take precedence over convenience.

vnprc · 2024-11-15T15:09:03Z

Gotcha, makes sense.

If someone has a u32 they may expect they can safely convert it to u64 to do From for Id

Yeah I thought that was where you were gonna go initially. What do you think about this?

impl From<u32> for Id {
    fn from(value: Id) -> Self {
        panic!("Conversion from u32 to Id is not allowed. This operation is invalid and should never be performed.");
    }
}

thesimplekid · 2024-11-15T15:31:50Z

In general, we don't use panic in cdk other then tests always return an error.

Here specifically, I don't think there is any need to add a function that will always panic or return an error the omission of a function supporting it implicitly signals it can't/shouldn't be done. This also introduces a runtime panic instead of a developer trying to do the conversion and realizing there is no function for it when developing a much preferable time for this realization.

If we want to be more explicit about it being a one way function and why we can add to that documentation on the From<Id> for u32

fix: return u32 from existing Id::TryFrom and add lossless u64 versions

8791109

thesimplekid reviewed Nov 13, 2024

View reviewed changes

vnprc added 3 commits November 13, 2024 10:33

remove TryFrom<u64> for Id and it's inverse

67fc241

fix: remove unit test and fix nut13::derive_path_from_keyset_id

2cc8618

test: derive_path_from_keyset_id

f3ae4f4

vnprc changed the title ~~Update existing Id::try_from to return a u32 and impl new ones to convert to and from u64~~ Update Id::try_from to return a u32 and remove redundant logic in nut13::derive_path_from_keyset_id Nov 13, 2024

fix: convert Id::TryFrom to Id::From

507cadb

docs: comment calling out From<Id> for u32 as a one-way function

bb4b49e

vnprc force-pushed the u64_keyset_id_2 branch from 11c725b to bb4b49e Compare November 14, 2024 22:41

thesimplekid approved these changes Nov 15, 2024

View reviewed changes

thesimplekid merged commit f9d500e into cashubtc:main Nov 15, 2024
43 checks passed

vnprc deleted the u64_keyset_id_2 branch November 17, 2024 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Id::try_from to return a u32 and remove redundant logic in nut13::derive_path_from_keyset_id #452

Update Id::try_from to return a u32 and remove redundant logic in nut13::derive_path_from_keyset_id #452

vnprc commented Nov 13, 2024

thesimplekid Nov 13, 2024

vnprc Nov 13, 2024

vnprc Nov 13, 2024

vnprc Nov 13, 2024

vnprc commented Nov 13, 2024

vnprc commented Nov 13, 2024

thesimplekid commented Nov 13, 2024 •

edited

Loading

thesimplekid commented Nov 14, 2024 •

edited

Loading

vnprc commented Nov 14, 2024 •

edited

Loading

thesimplekid commented Nov 15, 2024

vnprc commented Nov 15, 2024

thesimplekid commented Nov 15, 2024

Update Id::try_from to return a u32 and remove redundant logic in nut13::derive_path_from_keyset_id #452

Update Id::try_from to return a u32 and remove redundant logic in nut13::derive_path_from_keyset_id #452

Conversation

vnprc commented Nov 13, 2024

thesimplekid Nov 13, 2024

Choose a reason for hiding this comment

vnprc Nov 13, 2024

Choose a reason for hiding this comment

vnprc Nov 13, 2024

Choose a reason for hiding this comment

vnprc Nov 13, 2024

Choose a reason for hiding this comment

vnprc commented Nov 13, 2024

vnprc commented Nov 13, 2024

thesimplekid commented Nov 13, 2024 • edited Loading

thesimplekid commented Nov 14, 2024 • edited Loading

vnprc commented Nov 14, 2024 • edited Loading

thesimplekid commented Nov 15, 2024

vnprc commented Nov 15, 2024

thesimplekid commented Nov 15, 2024

thesimplekid commented Nov 13, 2024 •

edited

Loading

thesimplekid commented Nov 14, 2024 •

edited

Loading

vnprc commented Nov 14, 2024 •

edited

Loading