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: add function to decode an entire slice #23

Merged
merged 11 commits into from
Aug 5, 2024

Conversation

Wollac
Copy link
Contributor

@Wollac Wollac commented Jul 17, 2024

This PR fixes #21 by adding the public function decode_exact described above. It also introduces a new error type, since none of the existing ones fit.
This PR can be closed without merging if the decode_exact function is deemed unnecessary in the context of this crate.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes
  • Updated CHANGELOG.md

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

sorry for the delay, only now enabled notifications on this...

Comment on lines 15 to 16
/// Additional trailing bytes found after decoding.
TrailingBytes,
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be expressed with the Unexptectedlength variant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering that this is currently returned when the payload length doesn't match the array size or other payload length issues, I didn't think it would be a good fit for this since decode_exact() doesn't look at the payload, but rather the entire RLP encoded data. However, if this is just me, I can definitely adjust `UnexpectedLength' to better account for both cases...

Copy link
Member

Choose a reason for hiding this comment

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

I would be ok with the new variant however this is unfortunately a breaking change because the enum is not marked as #[non_exhaustive]. So for now I would prefer the other option and adding a TODO for the new variant

crates/rlp/src/decode.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this is reasonable and useful imo

any objections @Rjected @DaniPopes ?

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Thanks!

@DaniPopes DaniPopes merged commit 2ba859b into alloy-rs:main Aug 5, 2024
18 checks passed
@Wollac Wollac deleted the feat/decode-full branch October 2, 2024 08:11
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.

Decode an entire slice
3 participants