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

Implement keccak256 as builtin method #255

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Feb 16, 2021

What was wrong?

As discussed in #188. Fe needs to expose the keccak256 hash function to be usable from user code.

How was it fixed?

  1. Implemented keccak256 as a global function
  2. Analyzer ensures it can only be called with bytes[n] parameter
  3. Mapper is pretty straight forward
  4. keccak utils were slightly refactored to expose a bare get_keccak256 method
  5. Added contract tests to calculate various keccaks
  6. Added test to prove keccak256 can not be called with arrays of other types
  7. Added bare runtime tests for the YUL keccak256 function

To-Do

  • OPTIONAL: Update Spec if applicable
  • Add entry to the release notes (may forgo for trivial changes)

@codecov-io
Copy link

codecov-io commented Feb 16, 2021

Codecov Report

Merging #255 (bf2117b) into master (9c49805) will increase coverage by 0.08%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   93.91%   93.99%   +0.08%     
==========================================
  Files          54       54              
  Lines        3744     3761      +17     
==========================================
+ Hits         3516     3535      +19     
+ Misses        228      226       -2     
Impacted Files Coverage Δ
analyzer/src/lib.rs 91.93% <ø> (ø)
compiler/src/yul/mappers/expressions.rs 97.29% <75.00%> (+0.13%) ⬆️
analyzer/src/traversal/expressions.rs 92.63% <100.00%> (-0.20%) ⬇️
compiler/src/yul/runtime/abi_dispatcher.rs 100.00% <0.00%> (ø)
compiler/src/yul/runtime/functions/structs.rs 100.00% <0.00%> (ø)
analyzer/src/namespace/types.rs 82.50% <0.00%> (+1.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c49805...bf2117b. Read the comment docs.

@cburgdorf cburgdorf force-pushed the christoph/feat/keccak256 branch 2 times, most recently from efcbb55 to 65e5530 Compare February 16, 2021 15:16
@cburgdorf cburgdorf force-pushed the christoph/feat/keccak256 branch 3 times, most recently from e1adfa5 to b1a5c80 Compare February 17, 2021 19:43
@cburgdorf cburgdorf marked this pull request as ready for review February 17, 2021 19:46
Copy link
Member

@g-r-a-n-t g-r-a-n-t left a comment

Choose a reason for hiding this comment

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

looks good

@@ -67,8 +71,7 @@ fn move_expression(
from: Location,
to: Location,
) -> Result<yul::Expression, CompileError> {
let typ =
FixedSize::try_from(typ).map_err(|_| CompileError::static_str("invalid attributes"))?;
let typ = to_fixed_size(&typ)?;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep the original line with an expect instead of map_err, so we crash in this situation. All the results should be removed in this module.

@cburgdorf cburgdorf merged commit ce2834f into ethereum:master Feb 18, 2021
@cburgdorf cburgdorf mentioned this pull request Feb 19, 2021
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.

3 participants