ed25519 keys support and import keys new flow #126
Conversation
himanshuchawla009
commented
Dec 12, 2023
•
himanshuchawla009
commented
- Added support for import keys using 2pc approach, please refer to this discord thread to know how it works in backend.
- https://discord.com/channels/514301842281463818/1148500225246375987
- Added support for ed25519 keys.
…stead of calling to all endpoints
…o corresponding nodes using node indexes
There was a problem hiding this comment.
From ed25519 standpoint, looks mostly good to me. Can't comment much on whether flows are correct beyond that, but generally looks good.
Left some minor comments.
Some general comments:
- Tests could be made more modular. Currently, a lot of code is duplicated between
secpandedtests, if I see that correctly. - Tests are currently failing. Not sure if that's an issue with this PR, or was already an issue before.
| const key = ecCurve.keyFromPublic({ x: publicKeyX.toString("hex", 64), y: publicKeyY.toString("hex", 64) }); | ||
| const publicKey = key.getPublic().encode("hex", false).slice(2); | ||
| log.info(key.getPublic().encode("hex", false), "public key"); | ||
| const evmAddressLower = `0x${keccak256(Buffer.from(publicKey, "hex")).slice(64 - 38)}`; | ||
| return toChecksumAddress(evmAddressLower); | ||
| } | ||
|
|
||
| export function getPostboxKeyFrom1OutOf1(ecCurve: ec, privKey: string, nonce: string): string { | ||
| export function getPostboxKeyFrom1OutOf1(ecCurve: EC, privKey: string, nonce: string): string { | ||
| const privKeyBN = new BN(privKey, 16); | ||
| const nonceBN = new BN(nonce, 16); | ||
| return privKeyBN.sub(nonceBN).umod(ecCurve.curve.n).toString("hex"); |
There was a problem hiding this comment.
some users / libraries may expect the hex to be 32 bytes long (64 characters). ideally should use a global function to encode to bytes / hex, so that we don't have to do and check this manually every time. (even better might be to create a package that can be used across libraries.)
There was a problem hiding this comment.
will address these suggestions to make test cases modular in other PRs, thanks for suggesting these improvement
src/helpers/common.ts
Outdated
| address: key.address, | ||
| }; | ||
| }); | ||
| const finalKey = result.keys[0]; |
There was a problem hiding this comment.
why are we using only the first key?
There was a problem hiding this comment.
So there are cases where it is possible that a user can have multiple keys but all the nodes might not have all those shares/pub keys for ex i have seen these cases when a node is temp down but key was assigned successfully by other threshold number of nodes.
and as a result we should only check threshold for first key rather than checking older keys which are not even being used on frontend and can have mismatch in threshold when some nodes have more keys assigned for a user than others.
himanshuchawla009
force-pushed
the
fix/ed25519-keys
branch
from
0deee17 to
12fb612
Compare
