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

dev: improve rlp.cairo to support recursive list #457

Merged
merged 21 commits into from
Nov 1, 2023

Conversation

Quentash
Copy link
Contributor

Implemented recursive struct in rlp.cairo to be able to handle recursive list. I also tried to be as close as the code given in the ethereum doc ; https://ethereum.org/en/developers/docs/data-structures-and-encoding/rlp/ .

Here is a list of the biggest changes :

  • Types have been reduced to only be String or List.
  • decode_type() ( former from_bytes() ) now takes an array to be able to retrieve the long strings/lists size. It does now return the type which is either a string or a list, the offset at which it starts and its size.
  • decode() is now recursively called in case of recursive lists.

Here is a screenshot of gas consommation before the modifications ;

image

And here is one after the modifications ;

image

It's worth noting that some tests show improvement.
For instance, the test_decode_string appears to consume more gas than before, but this test is essentially a loop that converts a single character into an array, explaining the increased gas consumption. But on the other hand, the test_decode_long_list test, which deals with a larger amount of bytes, consumes less gas than it did originally.

I am certain that there is still potential for further improvement, and I would appreciate feedback on the direction these changes are taking. Whether you agree or disagree with some of the modifications.

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?

rlp.cairo.decode() doesn't handle recusrive list.

Resolves: #428

What is the new behavior?

  • handle recursive list

Does this introduce a breaking change?

  • Yes
  • No

Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

First review, will have more questions in the coming days as I have more time to do a second review

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

Made a better recursive implementation for the rlp_decode().
No more "Option::Error", no more loop!

@Quentash
Copy link
Contributor Author

I had to downgrade starknet/cairo versions in scarb files back to 2.3.0-rc0 because the recursivity seems to fail in Cairo 2.3.0 giving a very strange error (see screenshot).
image

Which still appears on github CL/Tests.
To make the tests run on local, install scarb-rc1 or 0, they both seem to handle struct recursivity. I don't understand why the 2.3.0 break it yet, maybe an issue should be open on the compiler github ?

@Eikix
Copy link
Member

Eikix commented Oct 30, 2023

Interestingly, there was a bug in the compiler, it'll be fixed in the next release

@Eikix Eikix marked this pull request as ready for review October 31, 2023 17:43
Scarb.toml Outdated Show resolved Hide resolved
Scarb.toml Show resolved Hide resolved
Eikix
Eikix previously approved these changes Nov 1, 2023
Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

lgtm

Scarb.lock Outdated Show resolved Hide resolved
Copy link
Member

@Eikix Eikix left a comment

Choose a reason for hiding this comment

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

Congratulations! Well fought

@Eikix Eikix merged commit ceeba70 into kkrt-labs:main Nov 1, 2023
2 of 3 checks passed
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.

dev: improve rlp.cairo to support recursive list
4 participants