Feat/get public address keytype #170
Conversation
src/helpers/keyUtils.ts
Outdated
| * @returns \{EC\} The elliptic curve. | ||
| * | ||
| */ | ||
| export const getEcCurve = (keyType: KeyType): EC => { |
There was a problem hiding this comment.
there's a similar function getKeyCurve that already exists in src/helpers/common.ts
There was a problem hiding this comment.
getKeyCurve can also be optimised to be similar to getEcCurve, i.e. to use constant instances:
const secp256k1EC = new EC("secp256k1");
const ed25519EC = new EC("ed25519");based on the elliptic's docs, it's safe to initialise the EC instance once and reuse it:
// Create and initialize EC context
// (better do it once and reuse it)
var ec = new EC('secp256k1');There was a problem hiding this comment.
the Torus class constructor and tests should use getKeyCurve to retrieve the EC instance for consistency
// current implementation
this.ec = new EC(this.keyType);
// proposed implementation
this.ec = getEcCurve(this.keyType);
src/torus.ts
Outdated
| enableOneKey: boolean | ||
| ): Promise<TorusPublicKey> { | ||
| const localKeyType = keyType ?? this.keyType; | ||
| const localEc = getEcCurve(localKeyType); |
There was a problem hiding this comment.
before retrieving an EC instance, we should probably do this check that is similar in the constructor:
(keyType === KEY_TYPE.ED25519 && LEGACY_NETWORKS_ROUTE_MAP[network as TORUS_LEGACY_NETWORK_TYPE])otherwise, we might pass the incorrect key type into formatLegacyPublicKeyData
would be nice to bake this check into a function that can be reused here and in the constructor
There was a problem hiding this comment.
it could make more sense to do this check in formatLegacyPublicKeyData, or simply apply the same check wherever getEcCurve is used
There was a problem hiding this comment.
Not going to check for the instance's keyType
this feature is to Allow getPublicAddress to retrieve different keyType based on input params regardless of the instances's keyType
There was a problem hiding this comment.
Assume that this.keyType is secp256k1 and this.network is a legacy network when the Torus class is initialised.
In getPublicAddress, if keyType is passed in as ed25519, then it won't be compatible with the legacy network. We need to guard against this potential bug.
There was a problem hiding this comment.
Added check on prior GetPubKeyOrKeyAssign
redundance function
src/helpers/common.ts
Outdated
| }; | ||
|
|
||
| const secp256k1EC = new EC("secp256k1"); | ||
| const ed25519EC = new EC("ed25519"); |
There was a problem hiding this comment.
this was intentionally not left as a global variable to reduce the build size
There was a problem hiding this comment.
would create EC object every time increase memory usage and latency?
src/torus.ts
Outdated
| ): Promise<TorusPublicKey> { | ||
| log.info(torusNodePubs, { verifier, verifierId, extendedVerifierId }); | ||
| return this.getNewPublicAddress(endpoints, { verifier, verifierId, extendedVerifierId }, this.enableOneKey); | ||
| return this.getNewPublicAddress(endpoints, { verifier, verifierId, extendedVerifierId, keyType }, this.enableOneKey); |
There was a problem hiding this comment.
can we pass keyType here as const localKeyType = keyType ?? this.keyType for the sake of consistency with other public functions
| }); | ||
| }); | ||
|
|
||
| it("should should fetch public address with keyType", async function () { |
There was a problem hiding this comment.
can you use existing test case and user for testcase to validate nothing changes on passing keyType for existing test user
There was a problem hiding this comment.
Willa_Funk11@gmail.com is existing testcase's email in the sapphire_devnet_ed25519.test.ts
Motivation and Context
To support multicurve tss (mpc core-kit) -
Allow getPublicAddress to retrieve different keyType based on input params
Jira Link:
https://toruslabs.atlassian.net/browse/IRT-1680
Description
How has this been tested?
Screenshots (if appropriate):
Types of changes
Checklist: