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 SHA3 and Keccak-256 #3

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ray-react0r
Copy link

Proposed Changes

  • Added SHA3 function and tests
  • Added checkSum to Hash type to simplify sum functions for DRY (IIRC)

Copy link
Member

@q-uint q-uint left a comment

Choose a reason for hiding this comment

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

Make sure to follow the Motoko Naming Guidelines.

Comment on lines +9 to +11
// Return the hash.
// TODO: Should probably rename this
checkSum() : [Nat8];
Copy link
Member

Choose a reason for hiding this comment

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

It there are reason this was added to the Hash interface?

Copy link
Author

Choose a reason for hiding this comment

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

I added it to avoid repeating myself in the SHA3_* by reusing New. The checkSum function in SHA2.mo seemed to be the main part of the hashing too. Thought it'd be good to add. I can define the parameters like H256 instead though.

@@ -21,6 +21,8 @@ module HMAC {

public func size() : Nat { outer.size() };

public func checkSum() : [Nat8] { [0] };
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed.

Comment on lines +108 to +114
private func dump() {
Debug.print("State");
for (i in Iter.range(0, state.size()-1)) {
let b = Array.reverse<Nat8>(unpack64(state[i]));
Debug.print(Hex.encode(b));
};
};
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed.

Copy link
Author

@ray-react0r ray-react0r Mar 22, 2023

Choose a reason for hiding this comment

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

Thought I'd leave it, in case more testing (and debugging) need to be done. Easier to have it and ask to remove IMHO. Can remove it no problem.

state[pt/8] ^= Nat64.fromNat(Nat8.toNat(delimitedSuffix)) << Nat64.fromNat((pt%8)*8);
state[(rsize-1:Nat)/8] ^= 0x80 : Nat64 << Nat64.fromNat(((rsize-1):Nat%8)*8);

//dump();
Copy link
Member

Choose a reason for hiding this comment

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

Idem.

pt := 0;
};
};
//dump();
Copy link
Member

Choose a reason for hiding this comment

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

Idem.


import Keccak_256 "../src/SHA/Keccak_256";

//Debug.print("Testing Keccak_256");
Copy link
Member

Choose a reason for hiding this comment

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

Should either be removed or uncommented (same for other print statements).

Copy link
Author

Choose a reason for hiding this comment

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

To me, these served more as comments about describing the tests and to break them up. Safe to assume you want them removed as the other tests don't emit any messages?

};

// Function to update internal state with content
// TODO: should data be a blob? Sticking with Array from Hash type
Copy link
Member

@q-uint q-uint Mar 22, 2023

Choose a reason for hiding this comment

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

Once you can get individual bytes out of a blob, the whole package will migrate to it.
Currently blobs do not allow you to do much.

Copy link
Author

Choose a reason for hiding this comment

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

Good to know. I'll drop the comment.

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.

2 participants