feat: add zilliqa schnorr signing #185
Conversation
feri42
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
feri42
force-pushed
the
f/add-zilliqa-schnorr
branch
from
2269512
to
9d50fc5
Compare
feri42
force-pushed
the
f/add-zilliqa-schnorr
branch
from
a03e1ae
to
79a545c
Compare
| import { hashSync } from '@exodus/crypto/hash' | ||
| import { hmacSync } from '@exodus/crypto/hmac' | ||
| import { randomBytes } from '@exodus/crypto/randomBytes' | ||
| import * as secp256k1 from '@noble/secp256k1' |
There was a problem hiding this comment.
for reference, the code is based on https://github.com/ExodusMovement/assets/blob/a8fec3450079e64d5fdff5600fa5f4f0aca0d111/zilliqa/zilliqa-lib/src/schnorr.js
question, would the code in asset's zilliqa-lib be removed?
There was a problem hiding this comment.
Yes. We can directly import @exodus/keychain/module/crypto/secp256k1 for use in the library.
There was a problem hiding this comment.
Unless you have a different suggestion?
There was a problem hiding this comment.
Maybe in the future, an asset won't be able/won't need to define signing, signing is always happing via the keychain. (move funds/import key would break this logic though)
There was a problem hiding this comment.
We would need to add support for adding a private key to the keychain. Not sure what that would look like. Currently the keychain operates by deriving private keys from a seed. This private key would not be associated with a seed.
|
The code is really just copied from here: https://github.com/ExodusMovement/assets/blob/a8fec3450079e64d5fdff5600fa5f4f0aca0d111/zilliqa/zilliqa-lib/src/schnorr.js#L37 . Don't think we need to review the cryptography itself. But I do need at least concept approval. Or suggestions for a different approach. |
|
how are zilliqa schnorr sigs diff, can we just use regular schnorr? |
I won't pretend to understand the differences, but the way I see it is the normal schnorr gets the random entropy once through the The zilliqa schnorr computes a new random value on every iteration. I'm not even sure there is a quantitative difference there. |
There was a problem hiding this comment.
Concept ack (discussed a while ago)
Did not closely review the code (yet)
Not sure if we should place this in /crypto, but that might take a while, so let's not block this PR -- can move it later
Nit: given the async nature of keychain, this should use async hmac/hash instead, that will make those use native impls in webcrypto contexts
@mvayngrib, no, zil has a different schnorr impl Introducing it somewhere was the plan we discussed about a month ago |
feri42
force-pushed
the
f/add-zilliqa-schnorr
branch
from
a72f683
to
f0b55b1
Compare
Add schnorr for Zilliqa. To be used in seed signer: https://github.com/ExodusMovement/exodus-hydra/pull/10588
Toward: https://github.com/ExodusMovement/assets/issues/2092