Skip to content

Conversation

contractormario
Copy link
Contributor

PR adds core/format/KeyGenerator. Usage:

const u64 = KeyGenerator.fromString('key_name') // returns UInt64

@coveralls
Copy link

coveralls commented Oct 7, 2019

Pull Request Test Coverage Report for Build 732

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 720: 0.0%
Covered Lines: 10
Relevant Lines: 10

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 727

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 720: 0.0%
Covered Lines: 10
Relevant Lines: 10

💛 - Coveralls

/**
* @returns {UInt64} Deterministic uint64 value for the given string
*/
public static fromString(input: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why fromString? should it be a better name e.g gernerateXXX?

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing return type in the signature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What name do you suggest, generateUint64? generateKey?

if (input.length === 0) {
throw Error(`Input must not be empty`);
}
if (input.length > 1024) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not all keys have the same restriction. the check should be case specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly only the metadata.value require this check. Don't think we need this check at all. do we?

if (input.length === 0) {
throw Error(`Input must not be empty`);
}
if (input.length > 1024) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly only the metadata.value require this check. Don't think we need this check at all. do we?

throw Error(`Input exceeds 1024 characters (has ${input.length})`);
}
const hex = sha3_256(input)
return UInt64.fromHex(hex.substr(0, 16))
Copy link
Contributor

Choose a reason for hiding this comment

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

if you check the definition of UInt64, it uses 2 Uint32 (low / high). I would convert the hash buffer array to uint32 array and return the new Uint64( ['low', 'high']). Not quite sure about the substring here.

I would do something like

const result = new Uint32Array(hex.arrayBuffer());
return new UInt64([result[0], (result[1] | 0x80000000) >>> 0]);

* @param {string} input Input string
* @returns {UInt64} Deterministic uint64 value for the given string
*/
public static fromString(input: string): UInt64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we call it generateUInt64Key instead?
@evias @dgarcia360

Copy link
Contributor

@rg911 rg911 left a comment

Choose a reason for hiding this comment

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

👍

@rg911 rg911 merged commit 7615a42 into symbol:master Oct 7, 2019
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.

4 participants