pubky / pkarr

Public Key Addressable Resource Records (sovereign TLDs)
https://app.pkarr.org
MIT License
153 stars 17 forks source link

fix: signed packet from bytes #57

Closed Frando closed 6 months ago

Frando commented 6 months ago

56 had a mistake in the from_bytes method... Here's the fix, this time with the test to prevent mistakes like this again.

Nuhvi commented 6 months ago

@Frando Irrelevant but do you think the naming of the methods need changing to be idiomatic? like try_from_bytes instead? Similarly, is the as_ methods naming appropriate because cloning Bytes is cheap?

Sorry for the newbie questions, just want to leverage the V2 to most :)

Frando commented 6 months ago

Yeah I am a bit unsure there usually myself... The general convention is: as_ -> gives you a reference to some inner data, always cheap to_ -> gives you an owned copy of some inner data, always cheap into_ -> gives you an owned value, can be expensive

So it should be to_bytes and not as_bytes because you get an owned Bytes and not a reference.

not sure when it's idiomatic to do try_ prefixes to be honest. I think in std it's try_from to not conflict with from trait, and for regular methods it's try_into_foo if there's also a into_foo which would panic in case of error. E.g currently a lot of try_with_capacity etc methods are being added to Vec etc to fail with error and not panic if allocation fails.

Nuhvi commented 6 months ago

Hmm I thought try_ should be anything that returns Result. Will read more and update it in v2.

Frando commented 6 months ago

Yea I've seen different things in different codebases. E.g. in databases or so where each operation involves IO it's not common to add try_ prefixes because all methods would have try_. For fallible conversions I've seen both. Maybe there try_ is more idiomatic, not sure.