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 solidity like bytes which supports keccak and sha256 #184

Merged
merged 5 commits into from
Oct 10, 2023

Conversation

zkcarter
Copy link
Contributor

@zkcarter zkcarter commented Oct 5, 2023

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?

bytes is an implementation similar to Solidity bytes written in Cairo 1.
It is notable for its built-in implementation of Keccak and Sha256 hash functions, offering convenience for migrating EVM Contracts written in Solidity to the Starknet ecosystem.

Issue Number: N/A

What is the new behavior?

  • Add bytes implementation
  • Add bytes unit tests

Does this introduce a breaking change?

  • Yes
  • No

Other information

The implementation of bytes differs from byte_array in corelib. Each bytes array element stores 16 bytes and is designed to calculate byte stream hashes more quickly by trading more space for less computational cost.

@zkcarter zkcarter requested a review from 0xLucqs as a code owner October 5, 2023 06:31
@0xLucqs
Copy link
Collaborator

0xLucqs commented Oct 5, 2023

Thank you for this PR this looks extreeeemely useful

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.

First review

src/bytes/src/bytes.cairo Outdated Show resolved Hide resolved
src/bytes/src/bytes.cairo Outdated Show resolved Hide resolved
src/bytes/src/bytes.cairo Outdated Show resolved Hide resolved
src/bytes/src/bytes.cairo Outdated Show resolved Hide resolved
src/bytes/src/bytes.cairo Outdated Show resolved Hide resolved
src/bytes/src/bytes.cairo Outdated Show resolved Hide resolved
src/bytes/src/bytes.cairo Outdated Show resolved Hide resolved
src/bytes/src/bytes.cairo Outdated Show resolved Hide resolved
src/bytes/src/bytes.cairo Outdated Show resolved Hide resolved
src/bytes/src/bytes.cairo Outdated Show resolved Hide resolved
@zkcarter
Copy link
Contributor Author

zkcarter commented Oct 6, 2023

@LucasLvy Thanks for your review. I have fixed the problems above.

@zkcarter zkcarter requested a review from 0xLucqs October 6, 2023 03:51
src/bytes/src/bytes.cairo Show resolved Hide resolved
src/bytes/src/bytes.cairo Show resolved Hide resolved
src/bytes/src/utils.cairo Outdated Show resolved Hide resolved
src/bytes/src/utils.cairo Outdated Show resolved Hide resolved
src/bytes/src/utils.cairo Outdated Show resolved Hide resolved
@zkcarter
Copy link
Contributor Author

zkcarter commented Oct 7, 2023

@LucasLvy Sorry for the codes likes break ();. I have wrote those codes serveral months ago and Cairo 1 sytnax changes fast. I have fix the problems you mentioned above. Thanks for your review again.

@zkcarter zkcarter requested a review from 0xLucqs October 7, 2023 07:37
@0xLucqs
Copy link
Collaborator

0xLucqs commented Oct 10, 2023

Note that this will be redundant with bytes 31 at some point

@0xLucqs 0xLucqs merged commit 69e36b9 into keep-starknet-strange:main Oct 10, 2023
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 12, 2023
@0xLucqs
Copy link
Collaborator

0xLucqs commented Oct 17, 2023

hey @zkcarter can you register on https://app.onlydust.xyz/ so i can pay for your contribution ? (can you also tell me how many hours/days you spent on this pr plz)

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