search

rust-bitcoin / rust-secp256k1

Rust language bindings for Bitcoin secp256k1 library.
Creative Commons Zero v1.0 Universal
350 stars 270 forks source link

chore: add back to/from i32 fns #765

Open mattsse opened 3 days ago

mattsse commented 3 days ago

in #743 these have been removed in favor of tryfrom/from traits.

this was a breaking change, the into 32 makes it a bit weirder to convert it to an u8. id.to_i32() as u8 is now i32::from(id) as u8.

imo it's reasonable to add them back, so this doesn't break for everyone that hasn't updated yet.

tcharding commented 3 days ago

Thanks for the contribution. Rust convention is to use From and TryFrom for conversions. I do not think we should add from_i32 back in.

to_i32() however seems useful and IMO probably should not have been removed, we could also add to_u8 if going to a u8 is common.

Both functions can be backported to v0.30.0.

apoelstra commented 16 hours ago

Agreed re from_i32 -- I think we should add it back but deprecate it.

Also agreed that we should add a to_u8 alongside to_i32. This value fits into a u8 and you probably want it in a u8 if your intent is to serialize it.

apoelstra commented 16 hours ago

Indeed, in rust-bitcoin where we use this function we have casts in both directions...when serializing, we do to_i32() as u8 which sucks, and also when deserializing we use as i32 to convert a u8 to a i32, which we shouldn't be doing.