fix(eddsa-poseidon): fixes carry handling of g function by introducing wrapper over original implementation #361
Conversation
…g original blake-hash imp This commit replaces the existing implementation of blake-hash with a TS wrapper over original blake-hash package and exposing required methods along with specified types for each re privacy-scaling-explorations#344
Arch0125
requested review from
ctrlc03,
cedoor,
artwyman and
sripwoud
as code owners
There was a problem hiding this comment.
I'll defer to @cedoor to drive the real review here with more context.
For my thoughts/concerns, see my commentary in the related issue here: #344 (comment)
| @@ -60,6 +60,7 @@ | |||
| "dependencies": { | |||
| "@zk-kit/baby-jubjub": "1.0.3", | |||
| "@zk-kit/utils": "1.2.1", | |||
| "blake-hash": "^2.0.0", | |||
| "blakejs": "^1.2.1", | |||
There was a problem hiding this comment.
- Does the blakejs library provide a blake1 hash which could be used here instead?
- Alternately, have you checked whether the blakejs library might have the same error that this PR is fixing, and need its own update for the blake2b use case?
There was a problem hiding this comment.
Does the blakejs library provide a blake1 hash which could be used here instead?
blakejs should only provide the following implementations: BLAKE2b and BLAKE2s.
Alternately, have you checked whether the blakejs library might have the same error that this PR is fixing, and need its own update for the blake2b use case?
Let me tag @thogiti for this question.
There was a problem hiding this comment.
Following up on the @artwyman's comment: #344 (comment).
It would be nice to have a test for this fix.
There was a problem hiding this comment.
Why this type in the poseidon-chipher lib?
| @@ -60,6 +60,7 @@ | |||
| "dependencies": { | |||
| "@zk-kit/baby-jubjub": "1.0.3", | |||
| "@zk-kit/utils": "1.2.1", | |||
| "blake-hash": "^2.0.0", | |||
| "blakejs": "^1.2.1", | |||
There was a problem hiding this comment.
Does the blakejs library provide a blake1 hash which could be used here instead?
blakejs should only provide the following implementations: BLAKE2b and BLAKE2s.
Alternately, have you checked whether the blakejs library might have the same error that this PR is fixing, and need its own update for the blake2b use case?
Let me tag @thogiti for this question.
Description
Related Issue(s)
Closes #344
Checklist
yarn stylewithout getting any errors