feat(account-lib): add support for secp256k1 curve TSS#2256
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2256 +/- ##
===========================================
+ Coverage 0 84.16% +84.16%
===========================================
Files 0 174 +174
Lines 0 7788 +7788
Branches 0 1219 +1219
===========================================
+ Hits 0 6555 +6555
- Misses 0 826 +826
- Partials 0 407 +407
Flags with carried forward coverage won't be shown. Click here to find out more.
|
4cbf25f to
c07e63d
Compare
c07e63d to
1e911c4
Compare
71e5536 to
fb4f5e0
Compare
tylerlevine
left a comment
There was a problem hiding this comment.
How come we're implementing these primitive curve operations in account-lib instead of using a dependency for these that's already in the utxo-lib dependency tree (eg, noble-secp256k1 or a pre-wasm version of tiny-secp256k1)?
fb4f5e0 to
7e13be3
Compare
7e13be3 to
70fd762
Compare
|
Hi @tylerlevine , the implementations are for the basic scalar operations over that finite field , for point operations we will be making use of noble-secp256k1 or tiny-secp256k1 or any suitable library. Still finalising on it |
70fd762 to
5f70eb7
Compare
5f70eb7 to
3c1ba89
Compare
a9d067d to
4f1c91c
Compare
d2f2cff to
be548a0
Compare
brandonblack
left a comment
There was a problem hiding this comment.
Couple of minor things left. Looking very clean.
|
The `Hex` type in noble-secp is `Uint8Array | string`, Buffer satisfies the
interface, IIUC.
…On Thu, May 19, 2022 at 1:00 PM Sachu Shaji Abraham < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In modules/account-lib/src/mpc/curves/secp256k1.ts
<#2256 (comment)>:
> @@ -0,0 +1,55 @@
+import { bigIntFromBufferBE, bigIntToBufferBE } from '../util';
+import BaseCurve from './baseCurve';
+import * as secp from ***@***.***/secp256k1';
+
+const order = secp.CURVE.n;
+
+export class Secp256k1Curve implements BaseCurve {
+ scalarRandom(): bigint {
+ return bigIntFromBufferBE(Buffer.from(secp.utils.randomPrivateKey()));
+ }
+
+ scalarAdd(x: bigint, y: bigint): bigint {
+ return bigIntFromBufferBE(Buffer.from(secp.utils.privateAdd(x, y.toString(16))));
[image: image]
<https://user-images.githubusercontent.com/98419902/169394545-8a87842d-aedb-427f-9234-166e36d5fd54.png>
Seems like tweek is expecting a hex value ? @brandonblack
<https://github.com/brandonblack>
—
Reply to this email directly, view it on GitHub
<#2256 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVTUYXBEWTGWTKLUAWUNC73VK2MV7ANCNFSM5VFNIHVQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
be548a0 to
52aec81
Compare
|
I believe that Paul on noble-secp is working on adding the build products to his tree similar to how I have them in my fork, so we should be able to move to the mainline soon. Not sure on his plans for a release. Until then, I think it is OK to depend on one commit off mainline with the build products checked in. We should probably update to use the commit hash that we're happy with rather than the branch though. |
52aec81 to
92d7824
Compare
tylerlevine
left a comment
There was a problem hiding this comment.
almost there. Thanks for providing in-depth review on this @brandonblack
There was a problem hiding this comment.
Why return a buffer? What do the return values mean? Please add a doc comment if it's not going to return a boolean.
There was a problem hiding this comment.
From the docs I looked up and reading through testcases, sodium.crypto_sign_open returns the original message if the signature is valid. I have added a comment for clarity @tylerlevine
There was a problem hiding this comment.
@joey-bai just want to bring your attention to this change to the verify function to have it follow same base parameters as ECDSA to have the message also passed to verify after discussing with @johnoliverdriscoll , I have a feeling that this could break some places, double checking with you. I think it would make sense to have it return as a boolean value if it verifies the signature correctly if there is no specific reason to return the actual message if signature is valid.
There was a problem hiding this comment.
Makes sense - i agree that boolean makes it more more clear how to use the output of this func
There was a problem hiding this comment.
updated to have it as boolean
92d7824 to
9e495d7
Compare
joey-bai
left a comment
There was a problem hiding this comment.
overall LGTM.
you may also want to get input from @johnoliverdriscoll, who implemented ed25519 + tss
12ead8d to
c17e289
Compare
this change adds support for secp256k1 curve and also changes verify function in ed25519 to return boolean TICKET:BG-47402
c17e289 to
7467f8c
Compare
tylerlevine
left a comment
There was a problem hiding this comment.
one nit but feel free to handle in follow up. Nice work 👍
this change adds support for secp256k1 curve for MPC / TSS and also changes verify function in ed25519 to return boolean
TICKET:BG-47402