Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat/refactor(rlp): improve implementations #182

Merged
merged 3 commits into from
Jul 9, 2023
Merged

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented Jul 9, 2023

Motivation

Refactor decode and add new impls and features.

Preferably Decodable would have a 'de lifetime, and Decodable::decode's param would be a &mut Decoder<'de> struct which would allow for easier helper method access (like Header::decode -> d.decode_header()?; etc), and deserializing borrowed byte and string slices, but this would be a significant breaking change so I didn't implement this here.

Solution

  • dedup decoding code by adding more static methods to Header, and by refactoring Header::decode to use a match expression.
  • add smol_str dep + feature from reth-rlp
  • add arrayvec impls + gate encode_fixed_size function to feature
  • breaking changes to the free-standing encode_* functions' generics
  • un-#[doc(hide)] and document MaxEncodedLen, MaxEncodedLenAssoc
  • add a Result type alias for the decode error type
  • update derive macros
    • added support for generics

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@DaniPopes DaniPopes requested a review from prestwich as a code owner July 9, 2023 13:02
@DaniPopes DaniPopes marked this pull request as draft July 9, 2023 13:09
@DaniPopes DaniPopes force-pushed the dani/rlp-refactor branch 3 times, most recently from e25552c to 34df39d Compare July 9, 2023 16:30
@DaniPopes DaniPopes marked this pull request as ready for review July 9, 2023 16:49
@DaniPopes DaniPopes requested a review from gakonst as a code owner July 9, 2023 16:49
@DaniPopes DaniPopes force-pushed the dani/rlp-refactor branch 3 times, most recently from 8188f65 to e7af9f2 Compare July 9, 2023 19:13
}
}

impl<E: Decodable> Decodable for alloc::vec::Vec<E> {
#[cfg(feature = "smol_str")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does smol_str have a motivating usecase downstream? It's not really a common library

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reth-rlp, I'm assuming it's used somewhere there but I haven't verified that

@DaniPopes DaniPopes merged commit 5664d52 into main Jul 9, 2023
@DaniPopes DaniPopes deleted the dani/rlp-refactor branch July 9, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants