ralexstokes / ssz-rs

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

add `no-std` #25

Closed claravanstaden closed 1 year ago

claravanstaden commented 1 year ago
ralexstokes commented 1 year ago

it looks like some of the changes in here were done in #29; please rebase main when you get a chance -- very excited to dig into this PR :)

claravanstaden commented 1 year ago

@ralexstokes thanks for the review! I have made the requested individual changes as well as the std lib imports as modelled by the serde crate in https://github.com/ralexstokes/ssz-rs/pull/25/commits/f29b3fe89b81a0a1d860ff0178b1a370805a78c3. Let me know if this is what you've had in mind. I can also move the crate std mod into lib, if you prefer (but I guess then the import would just be crate::*, which is less clear)? Or rename it something else. 😄

ralexstokes commented 1 year ago

Or rename it something else.

I like how serde does it and just calls it lib

can you update the mod name and then the imports so that it is crate::lib::* etc? thank you!

ralexstokes commented 1 year ago

also the fact that the tests are passing even though we are 'ignoring' some errors suggests the code coverage is not ideal -- I'll leave a note here to investigate further

claravanstaden commented 1 year ago

I like how serde does it and just calls it lib

can you update the mod name and then the imports so that it is crate::lib::* etc? thank you!

Done in https://github.com/ralexstokes/ssz-rs/pull/25/commits/d77632666961f66c49ff0ea8eaf5c21ecaf46dac. I removed the std mod and created a lib mod in lib.rs, since lib.rs exists already.

claravanstaden commented 1 year ago

also the fact that the tests are passing even though we are 'ignoring' some errors suggests the code coverage is not ideal -- I'll leave a note here to investigate further

This is a little worrying. The dropped error check I introduced was in pack_bytes, used by hash_tree_root, which the individual mod tests don't cover. But it is still concerning that the container tests didn't pick them up.

I had a quick look but must admit I couldn't quickly see how to add test cases that would trigger those error conditions.

dharjeezy commented 1 year ago

Is this still being worked on? Is there any way i can contribute to this?

claravanstaden commented 1 year ago

Is this still being worked on? Is there any way i can contribute to this?

@dharjeezy it's ready for review! @ralexstokes will probably get to it as soon as he can. 😄 You are welcome to also review and test if you are keen.

claravanstaden commented 1 year ago

@ralexstokes thanks for the review! 😄 Comments actioned.

claravanstaden commented 1 year ago

@ralexstokes thanks for the review! I've made the requested changes.

The one thing I am wondering about is how the new error mod fits into exposing child errors, if the crate user wants to check specific data structures like list::Error::IncorrectLength, like this: https://github.com/claravanstaden/use-ssz/blob/main/src/main.rs#L14. Or even just wants to check if Error is a List error:https://github.com/claravanstaden/use-ssz/blob/main/src/main.rs#L18 This doesn't seem to work:

error[E0308]: mismatched types
  --> src/main.rs:13:17
   |
12 |             match err {
   |                   --- this expression has type `ssz_rs::list::Error`
13 |                 Error::List(e) => println!("list error: {e}")
   |                 ^^^^^^^^^^^^^^ expected enum `ssz_rs::list::Error`, found enum `ssz_rs::Error`

Perhaps some explicit mapping between the new error module and child crates are needed for this to work? Or I am missing something that perhaps this error provides and I need to mirror that in this new PR.

Along the same line of thought, should we expose Error as ListError here? So that the user can match the kinds of errors (I mean in this case there is only one type of error IncorrectLength, so maybe it doesn't matter as much in this case, but just wondering about your thoughts on this).

ralexstokes commented 1 year ago

@claravanstaden I refactored the errors so this is no longer a concern; I haven't looked at the previous setup but the intention is to provide the user the ability to match on any 'internal' error to this library

that being said, there is the "top-level" Error that aggregates everything and serves as the entry point for most users of the library (renamed to SimpleSerializeError in the prelude so that use ssz_rs::prelude::*; doesn't try to clobber any Error symbol the user may also have in scope)

claravanstaden commented 1 year ago

Awesome @ralexstokes! I'm happy if you make the changes, I've given you write access on the Snowfork/ssz_rs repo. If you don't make the changes today, I'll make them tomorrow.