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

refactor: bytes_i_to_uint256 #1042

Merged
merged 16 commits into from
Apr 5, 2024

Conversation

obatirou
Copy link
Contributor

@obatirou obatirou commented Mar 24, 2024

Time spent on this PR:

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?

Resolves #897

What is the new behavior?

  • rename bytes_i_to_uint256 to bytes_big_endian_to_uint256
  • simplify boolean condition for is_bytes_len_16_bytes_or_less
  • follow conventions by having bytes_len before bytes
  • rename compute_half_uint256 to compute_half_uint256_from_bytes
  • improved readability for pow256_rev
  • use pow256_rev instead of pow from starkware lib: from 2419 to 1261 steps

With pow from starkware lib
before opti

With pow256_rev
after opti


This change is Reviewable

@enitrat
Copy link
Collaborator

enitrat commented Mar 24, 2024

hi @obatirou thanks for the PR, it's normal that CI is failing given that the runner was recently updated for the Cancun fork, but not the main branch of this repo! will take a look tomorrow

src/utils/utils.cairo Outdated Show resolved Hide resolved
src/utils/utils.cairo Outdated Show resolved Hide resolved
src/utils/utils.cairo Outdated Show resolved Hide resolved
src/utils/utils.cairo Outdated Show resolved Hide resolved
Copy link
Member

@ClementWalter ClementWalter left a comment

Choose a reason for hiding this comment

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

please remove the _big_endian in the function name as in solidity this is the default behavior (ie. that uint256(bytes32) is big endian actually)

@enitrat
Copy link
Collaborator

enitrat commented Mar 25, 2024

please remove the _big_endian in the function name as in solidity this is the default behavior (ie. that uint256(bytes32) is big endian actually)

@ClementWalter In SSJ there are occurences where we need to use conversions from little-endian bytes. I think it's for precompiles though, so it might not be a direct concern here. But I still think it's good to have it :)

@ClementWalter
Copy link
Member

please remove the _big_endian in the function name as in solidity this is the default behavior (ie. that uint256(bytes32) is big endian actually)

@ClementWalter In SSJ there are occurences where we need to use conversions from little-endian bytes. I think it's for precompiles though, so it might not be a direct concern here. But I still think it's good to have it :)

there are some places where we specifically put _little_ but by default without any precision I think that it's obvious that it's end. It's the solidity/yul behavior, it's part of our EVM ubiquitous languages

ClementWalter
ClementWalter previously approved these changes Apr 5, 2024
Copy link
Member

@ClementWalter ClementWalter left a comment

Choose a reason for hiding this comment

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

can you run once again the profiling to check the final steps count?

@ClementWalter
Copy link
Member

image
final results, ie steps down from 2425 to 601, ie x4 improvmenet

@ClementWalter ClementWalter merged commit 1904d37 into kkrt-labs:main Apr 5, 2024
6 checks passed
This was referenced Apr 11, 2024
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: refacto bytes_i_to_uint256
3 participants