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

Added solidityKeccak256 Function #77

Merged
merged 26 commits into from
Apr 21, 2022
Merged

Added solidityKeccak256 Function #77

merged 26 commits into from
Apr 21, 2022

Conversation

arimgibson
Copy link
Contributor

@arimgibson arimgibson commented Apr 18, 2022

Closes #66

  • Added solidityKeccak256 function based off ethers.js
  • Added testing, checking match with ethers.js for multiple types
  • Added inline documentation using TypeDoc
  • Add toTwos method onto TinyBig -- @dawsbot
  • Add test for different types in the same function call

Improvements to be added after MVP functionality

  • Clean up code and flatten function
  • Create and export keccak256 function, which the solidityKeccak256 function relies on
  • Update test for static bytes arrays (e.g. bytes5[3]) after determining if this is a bug in both ethers.js and essential-eth
    Added solidityKeccak256 Function #77 (review)

Polyfill buffer - No longer needed as other essential-eth functions aren't polyfilled

@vercel
Copy link

vercel bot commented Apr 18, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
essential-eth ✅ Ready (Inspect) Visit Preview Apr 21, 2022 at 10:55PM (UTC)

@arimgibson
Copy link
Contributor Author

@dawsbot I added a task to add the toTwos method onto the TinyBig class, which is needed for the pack helper function

@dawsbot
Copy link
Owner

dawsbot commented Apr 20, 2022

@arimgibson sounds great, I'll take on the toTwos here in a moment

'0xB5503a7db1A9105cd459D99153e69a76a8EF1530',
'0xaa0fc255b079e775f9307e5cfec472a555cebc3a',
],
[[15]],
Copy link
Contributor Author

@arimgibson arimgibson Apr 21, 2022

Choose a reason for hiding this comment

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

@dawsbot this input [[15]] should be valid in theory but fails in both essential-eth and ethers because essential-eth relies on code I believe to be broken that comes from ethers. Parsing in an array triggers the _pack fn to call itself passing the isArray parameter as true https://github.com/Earnifi/essential-eth/blob/675424d214951ba0dcf3be071cf5c6e641bd2578/src/utils/solidity-keccak256.ts#L85 which triggers _pack to add zeros stored in the Zeros const https://github.com/Earnifi/essential-eth/blob/675424d214951ba0dcf3be071cf5c6e641bd2578/src/utils/solidity-keccak256.ts#L11-L12 to the original 15 value passed in https://github.com/Earnifi/essential-eth/blob/675424d214951ba0dcf3be071cf5c6e641bd2578/src/utils/solidity-keccak256.ts#L67 This then triggers arrayify to throw an error because the number passed in is too large https://github.com/Earnifi/essential-eth/blob/675424d214951ba0dcf3be071cf5c6e641bd2578/src/utils/bytes.ts#L101 Any thoughts on remedying this? We could aim to fix this functionality in essential-eth, but then it won't match ethers. From what I can tell, this seems like a bug in ethers opposed to expected functionality

Copy link
Owner

Choose a reason for hiding this comment

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

@arimgibson can you create a GH issue in ethers?

Let's merge it as-is since it matches ethers for now.

@dawsbot dawsbot merged commit f44b6ca into master Apr 21, 2022
@dawsbot dawsbot deleted the feature/solidityKeccak256 branch April 21, 2022 22:55
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.

utils fn solidityKeccak256
2 participants