feat: refactor multicurve #217

ieow · 2025-02-12T06:49:40Z

Motivation and Context

Jira Link:
In this pr, the mpc corekit will be keyTypes and sigType agnostic.
it will throw error only during signing if the required keyTypes and tssLib is not available.

all keyTypes for

getTssData will have same
- tssFactorPubs
getTssShare for same factor key
- tssShareIndex

check the added new tests on how it can be call for different signing scheme

Description

Refactor multicurve

How has this been tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project. (run lint)
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
My code requires a db migration.

integrate multicurve tkey tests passed

fix tests add multicurve tests

matthiasgeihs · 2025-02-25T16:59:54Z

src/interfaces.ts

+  // sign(
+  //   data: Buffer,
+  //   opts?: {
+  //     hashed?: boolean;
+  //     secp256k1Precompute?: Secp256k1PrecomputedClient;
+  //     keyTweak?: BN;
+  //   }
+  // ): Promise<Buffer>;


remove if not needed.

why does the core kit interface no longer have a sign method? isn't this the most important method?

update with relevant function signatures and comments

src/mpcCoreKit.ts

matthiasgeihs · 2025-02-25T17:08:01Z

src/mpcCoreKit.ts


-  public getTssData(args: { skipThrow: boolean; keyType?: KeyType } = { skipThrow: false }) {
-    const result = this.tkey.metadata.getTssData(args.keyType ?? this.keyType, TSS_TAG_DEFAULT);
+  public getTssData(args: { skipThrow?: boolean; keyType: KeyType }) {


this skipThrow flag looks super odd. if you want to skip an error, just catch and discard!?

break the function to 2 functions for more clearity

src/mpcCoreKit.ts

matthiasgeihs · 2025-02-25T17:11:54Z

src/mpcCoreKit.ts

-    if (this._sigType === "ed25519" && this.options.useDKG) {
-      throw CoreKitError.invalidConfig("DKG is not supported for ed25519 signature type");
-    }
+    const nodeDetails = fetchLocalConfig(this.options.web3AuthNetwork, KeyType.ed25519);


Why do you fix the key type to ed25519?

the nodedetails variables is used to get the metadata url.
It does not matter which keyType is used to get the node details.
We can replace with the spKeyType for consistency

matthiasgeihs · 2025-02-25T17:32:55Z

src/mpcCoreKit.ts

+        if (this.supportedCurveKeyTypes.has(KeyType.ed25519)) {
+          const importTssBufEd25519 = importTssKey.ed25519 ? Buffer.from(importTssKey.ed25519, "hex") : undefined;
+          // check if key is in the tsslib and keytype exists
+          await this.tKey.initializeTss({
+            importKey: importTssBufEd25519,
+            tssKeyType: KeyType.ed25519,
+            serverOpts: {
+              // selectedServers: [],
+              authSignatures: this.state.signatures,
+            },
+          });
+        }
+      } else {


hu? why do you duplicate this part from below and nest it here? just move the below part out of the else and condition it like here!?

it is different for initialising first curve and 2nd curve.
during initializing 2nd curve, factorPub, and tssIndex are not allowed to be provided,

updating the logic and comment for easier reading

matthiasgeihs · 2025-02-25T17:34:37Z

src/mpcCoreKit.ts

  private async copyOrCreateShare(newFactorTSSIndex: number, newFactorPub: Point) {
    this.checkReady();
-    const tssData = this.getTssData();
+    const firstKeyType = this.getSupportedCurveKeyTypes()[0];


Since you use this a lot, I think you should create a dedicated function for it.
What's the logic behind always using the first key type here?

I are trying to get tssIndex from getTssShare or factorPub from getTssData.
both curves should have the same tssIndex (for same factorKey) and factorPub
Using either curve's keyType will get the same value.

using the first as instance might configured with single curve keytype only

matthiasgeihs · 2025-02-25T17:36:53Z

src/mpcCoreKit.ts


-    const { tssIndex } = await this.getTssShare(factorKey);
+    const firstKeyType = this.getSupportedCurveKeyTypes()[0];
+    const { tssIndex } = await this.getTssShare({ keyType: firstKeyType, factorkey: factorKey });


Isn't this wrong? What if you want to get the ed25519 share but first key type is secp256k1? Same applies to several other instances of first key type usage.

I are trying to get tssIndex from getTssShare or factorPub from getTssData.
both curves should have the same tssIndex (for same factorKey) and factorPub
Using either curve's keyType will get the same value.

using the first as instance might configured with single curve keytype only

src/mpcCoreKit.ts

src/utils.ts

matthiasgeihs · 2025-02-25T20:37:28Z

i think it would be good to merge this into the multi curve branch and then review together. why are the two PRs separated? @ieow

…TssTag feat: refactor setTssTag

remove unsed code and comment update new user comments and flow update comment

rename function name

ieow · 2025-02-27T07:05:19Z

i think it would be good to merge this into the multi curve branch and then review together. why are the two PRs separated? @ieow

The first pr is initial solution using setTkeyType to set active keyType
this current pr is keyType agnostic ( no active keyType)
previously there was was on discussion to choose which direction.
It was decided to use the agnostic direction.
Previous PR should be closed. I closed it yesterday.

Recent added new pr is support tkey's pr which add back the setTssTag instead of passing tag to most of the tkey tss function

since the prs is merged in tkey side, mpc-corekit prs is also merged

now there is only single pr in mpc core kit and tkey tss

fix ed25519 with accountIndex 0 fix useClientGeneratedTSSKey fix typo

fix multicurve test to use new storage instance fix useClientGeneratedTSSKey bug

ieow added 9 commits February 7, 2025 19:51

feat: initial commit

e61342e

integrate multicurve tkey tests passed

fix: add randomId verifier testcases

9a25f60

fix: enable multicurve

b80ad8a

fix tests add multicurve tests

feat: refactor multicurve

88bbf5d

fix: allow tsslib to be added later

6953287

fix: multicurve tests

8e7fde9

fix: stable with most test passed

7e699d2

fix: import recovery test

bc907dd

feat: refactor setTssTag

4422e74

ieow marked this pull request as ready for review February 21, 2025 21:35

ieow requested review from himanshuchawla009 and matthiasgeihs February 21, 2025 21:35

matthiasgeihs requested changes Feb 25, 2025

View reviewed changes

Merge pull request #218 from Web3Auth/feat/refactor-v2-multicurve-set…

1892f5e

…TssTag feat: refactor setTssTag

This was referenced Feb 26, 2025

feat: multi curve support #216

Closed

feat: refactor setTssTag #218

Merged

ieow added 4 commits February 27, 2025 08:02

fix: comments

214e1e9

remove unsed code and comment update new user comments and flow update comment

fix: get keyType and SigType from frostLib

7723c8a

fix: update interface and comment

2a4322d

rename function name

feat: break getTssData to 2 function for clearity

0415c0e

ieow changed the base branch from feat/v4-multicurve to v4 February 27, 2025 07:06

fix: address comment

d4ebfbc

fix ed25519 with accountIndex 0 fix useClientGeneratedTSSKey fix typo

matthiasgeihs self-requested a review February 27, 2025 17:09

ieow added 2 commits February 28, 2025 02:52

fix: rename signECDSA function name

5bac894

fix multicurve test to use new storage instance fix useClientGeneratedTSSKey bug

fix: update demo app

92340a7

feat: refactor multicurve #217

Are you sure you want to change the base?

feat: refactor multicurve #217

Uh oh!

Conversation

ieow commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Description

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ieow Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ieow Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matthiasgeihs commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ieow commented Feb 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ieow commented Feb 12, 2025 •

edited

Loading

ieow Feb 27, 2025 •

edited

Loading

ieow Feb 27, 2025 •

edited

Loading

matthiasgeihs commented Feb 25, 2025 •

edited

Loading