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: Implement RLP Encoding and Decoding #213

Merged
merged 15 commits into from
Dec 1, 2023

Conversation

Quentash
Copy link
Contributor

Implementation of RLP encoding and decoding based on https://ethereum.org/en/developers/docs/data-structures-and-encoding/rlp/ doc.

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: #212

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information

@Quentash Quentash marked this pull request as ready for review November 14, 2023 11:47
Copy link
Collaborator

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

Same for other functiosn

src/encoding/src/rlp.cairo Outdated Show resolved Hide resolved
@Quentash
Copy link
Contributor Author

Created a errors.cairo file inside encoding crate.
Changed panics to Results.

src/encoding/src/errors.cairo Outdated Show resolved Hide resolved
src/encoding/src/errors.cairo Outdated Show resolved Hide resolved
src/encoding/src/errors.cairo Outdated Show resolved Hide resolved
src/encoding/src/rlp.cairo Show resolved Hide resolved
src/encoding/src/rlp.cairo Show resolved Hide resolved
src/encoding/src/rlp.cairo Show resolved Hide resolved
src/encoding/src/rlp.cairo Show resolved Hide resolved
src/encoding/src/rlp.cairo Show resolved Hide resolved
src/numeric/src/integers.cairo Outdated Show resolved Hide resolved
@Quentash
Copy link
Contributor Author

  • Simplified errors handling and therefore deleted errors.cairo.
  • Moved RLPError into rlp.cairo
  • Made a generic trait but not sure about the name, do you have any suggestion ?
  • Added tests for the U32Impl in integers.cairo
  • Corrected list encoding tests variables naming
  • Added comments to explain the safe unwraps.

Copy link
Collaborator

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

remove the unnecessary (()) and either remove the unwraps or say why they're safe plz

src/encoding/src/rlp.cairo Outdated Show resolved Hide resolved
src/encoding/src/rlp.cairo Outdated Show resolved Hide resolved
src/encoding/src/rlp.cairo Outdated Show resolved Hide resolved
@Quentash
Copy link
Contributor Author

  • Modified error handling for PayloadTooLong using ok_or() instead of the unsafe unwrap()

src/encoding/src/tests/rlp_test.cairo Outdated Show resolved Hide resolved
src/encoding/src/rlp.cairo Outdated Show resolved Hide resolved
src/encoding/src/tests/rlp_test.cairo Outdated Show resolved Hide resolved
src/numeric/src/integers.cairo Outdated Show resolved Hide resolved
@Quentash
Copy link
Contributor Author

  • Moved comments right above their appropriate unwrap()
  • Removed once and for all the (()) ( my very bad )

@0xLucqs 0xLucqs merged commit 1de9e5f into keep-starknet-strange:main Dec 1, 2023
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants