add Keccak256 hash to cadence #1327
Conversation
zhiqiangxu
requested review from
joshuahannan,
SupunS and
turbolent
as code owners
|
Thank you for contributing @zhiqiangxu! This looks good 👍 From what I can see, this is all that is needed to be changed. However, after adding the new "enum case" to |
|
@tarakby Could you please have a look? |
docs/language/crypto.mdx
Outdated
| @@ -27,6 +27,9 @@ pub enum HashAlgorithm: UInt8 { | |||
| /// used in BLS signatures. | |||
| pub case KMAC128_BLS_BLS12_381 = 5 | |||
|
|
|||
| /// KECCAK_256 is ethereum-compatible Keccak-256 hash | |||
There was a problem hiding this comment.
It is preferable not to define the cryptographic algorithms based on its blockchain application, but using its original definition. I'd suggest something like:
| /// KECCAK_256 is ethereum-compatible Keccak-256 hash | |
| /// KECCAK_256 is the legacy Keccak algorithm with a 256-bits digest, as per the original submission to the NIST SHA3 competition. | |
| /// KECCAK_256 is different than SHA3 and is used by Ethereum. |
There was a problem hiding this comment.
Let's not remove Ethereum entirely from that description, perhaps? My suggestion:
/// KECCAK_256 is the 256-bit legacy Keccak algorithm (NIST SHA3 candidate submission), as used by Ethereum and others
There was a problem hiding this comment.
Ethereum is not removed entirely, it's on the 2nd line:)
| const HashAlgorithmDocStringKECCAK_256 = ` | ||
| This is a specific function for calculating the Keccak256 hash of arbitrary data. | ||
| ` |
There was a problem hiding this comment.
The text of the above algorithms seems to be a copy of the doc text, in that case I suggest to do the same for Keccak.
| const HashAlgorithmDocStringKECCAK_256 = ` | |
| This is a specific function for calculating the Keccak256 hash of arbitrary data. | |
| ` | |
| const HashAlgorithmDocStringKECCAK_256 = ` | |
| KECCAK_256 is the legacy Keccak algorithm with a 256-bits digest, as per the original submission to the NIST SHA3 competition. | |
| KECCAK_256 is different than SHA3 and is used by Ethereum. | |
| ` |
|
@zhiqiangxu I investigated the CI test failures. The addition of the new enum case causes a different iteration order, so the assertions in the failing tests need to be updated. Is it OK to commit the fixes to your branch? |
Sure, please do it, thanks! I haven't got time to look into the CI issue yet.. |
atree iteration order is now different, as the new built-in HashAlgorithm enum case is constructed before the test values
Codecov Report
@@ Coverage Diff @@
## master #1327 +/- ##
=======================================
Coverage 75.78% 75.78%
=======================================
Files 280 280
Lines 38779 38785 +6
=======================================
+ Hits 29390 29395 +5
- Misses 8039 8040 +1
Partials 1350 1350
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
FYI, @ramtinms and I will be adding Keccak-256 to Flow-go next week, for both the crypto and FVM side. |
ok, but seems that CI is still not happy? |
This is a PR for #537 .
I'll make another PR to the prefixedHashing in flow-go repo once this is merged (in order to reference to the newly added
runtime.HashAlgorithmKECCAK_256).Basically the PR for flow-go will be this commit:
zhiqiangxu/flow-go@317a094