ralexstokes / ssz-rs

Implementation of ethereum's `ssz`
Apache License 2.0
102 stars 40 forks source link

feat(`uint`): widen accepted range of types on `try_from_bytes_le` #39

Closed Evalir closed 1 year ago

Evalir commented 1 year ago

(Feel free to close this PR if the change is not wanted/needed—I know there's no issue associated to this)

Was just tinkering with the crate for a project, and saw that the type definition for bytes could be widened a bit so it can also accepted full vectors rather than just slices.

ralexstokes commented 1 year ago

hello! thanks for opening the PR :)

can you give me an example you have in mind? I'm not opposed to making this more flexible but if you just want to use a vector v instead here, you can just &v

e.g.

fn foo(a: &[u8]) {
    dbg!(a);
}

fn main() {
    let b = vec![1,2,3];
    foo(&b);
}
Evalir commented 1 year ago

np sure! it's mainly to also be able to pass Strings as well, e.g

fn foo(a: impl AsRef<[u8]> + std::fmt::Debug) {
    dbg!(a);
}

fn main() {
    let b = String::from("0xcafe");
    foo(&b);
}

it's pretty minor for sure, just adds a little bit of extra ergonomics :).

ralexstokes commented 1 year ago

this is a personal preference but in this case I'd rather the caller just use String::as_bytes

I assume there is a slight compile-time penalty to using impl Foo over a more concrete type and don't feel that this change in ergonomics justifies the increased cost on everyone else using this method

Evalir commented 1 year ago

Actually, in this case the impl Trait syntax is zero cost, as it's just syntactic sugar for generics. That being said, happy to close if it's your personal preference to make the user explicitly cast the string, which i find reasonable imo!

ralexstokes commented 1 year ago

right and I would be very surprised if the compile-time cost for generics was the same as for the concrete type

I appreciate the contribution :) but I'm not seeing a compelling reason to merge this change yet

Evalir commented 1 year ago

haha makes sense—read too fast and didn't notice you were talking about compile time. np, thank you!