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: make it clear when an invalid instance is created #8

Merged
merged 2 commits into from
Feb 26, 2024

Conversation

DaniPopes
Copy link
Member

  • remove From<Vec<u8>> impl in favor of from_vec_unchecked method
  • remove Extend impl in favor of extend_from* methods
  • allow unchecked access to underlying SmallVec
  • add "safe" counterparts to _unchecked that check validity of nibbles, and viceversa add missing checks and unchecked versions
  • update docs to use safe versions, add examples of bad usage

@DaniPopes DaniPopes merged commit 4acd82f into master Feb 26, 2024
16 checks passed
/// # Internal representation
///
/// The internal representation is currently a [`SmallVec`] that stores one nibble per byte. Nibbles
/// are stored inline (on the stack) up to a length of 64 nibbles, or 32 unpacked bytes. This means

Choose a reason for hiding this comment

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

Bit of a nit pick but I think this should be "32 packed bytes" rather than "32 unpacked bytes"

/// ```
#[inline]
#[track_caller]
pub fn from_nibbles<T: AsRef<[u8]>>(bytes: T) -> Self {

Choose a reason for hiding this comment

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

For the checked versions I would encourage using an Error rather than panicking on invalid nibbles.

e.g. for the following functions if any single bytes is > 0x0f

  • from_nibbles()
  • from_vec()

Although not strictly necessary this could be helpful for users decoding data from untrusted input, such that they don't need to validate the nibbles themselves. If they have validated the bytes they can use the unchecked() versions.

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