-
Notifications
You must be signed in to change notification settings - Fork 8
refactor: validates key type and sig type for TSS Endpoints #112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/override-keytype-sigtype
Are you sure you want to change the base?
refactor: validates key type and sig type for TSS Endpoints #112
Conversation
packages/fnd-base/src/endpoints.ts
Outdated
| legacyNetwork?: TORUS_LEGACY_NETWORK_TYPE, | ||
| keyType = KEY_TYPE.SECP256K1 as WEB3AUTH_KEY_TYPE, | ||
| sigType?: WEB3AUTH_SIG_TYPE | ||
| sigType = SIG_TYPE.ECDSA_SECP256K1 as WEB3AUTH_SIG_TYPE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous implementation, if sigType is not provided and keyType is provided as KEY_TYPE.ED25519, then tssPath would be tss-frost. However, in this updated code given the same scenario, tssPath would be tss instead since there is a default for sigType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in here, a29b786
matthiasgeihs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well done
| export * from "./sapphireNetworkConfig"; | ||
| export * from "./utils"; | ||
|
|
||
| export function fetchLocalConfig(network: TORUS_NETWORK_TYPE, keyType: WEB3AUTH_KEY_TYPE, sigType?: WEB3AUTH_SIG_TYPE): INodeDetails | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to move this function here from utils file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I moved the function, fetchLocalConfig, to prevent circular import, because ~
fetchLocalConfig needs import from /endpoints and getTSSEndpoints function in /endpoints also needs to import /utils to use validation function.
| const finalSigType = sigType ?? this._sigType; | ||
|
|
||
| // validate key type and sig type to fetch TSS endpoints | ||
| validateKeyTypeAndSigTypeForTSS(finalKeyType, finalSigType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should do validation on server side only specifically to maintain forward compatibility specially for fnd sdk. Its a good practice to fail fast in general but in this case we want server to be source of truth to ensure older client sdks will remain compatible to future changes in endpoints. I dont know any specific reason to do that for now but its just from past experience we want this sdk to remain future compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok.
But in the same method, https://github.com/torusresearch/fetch-node-details/blob/feat/key-sig-type-validation/packages/fetch-node-details/src/nodeDetailManager.ts#L117, we are also directly calling the fetchLocalConfig from fnd-base when it's failed to fetch from the server.
I think it'll still do validation on the client side.
Motivation and Context
Jira Link:
Description
Note: This PR is to address the comments in the another PR, #110
Do client-side validation to quickly catch errors before sending a network request.
sigTypeandkeyTypevalidation to fetch TSS EndpointssigTypeandkeyTypein NodeDetailsManagergetNodeDetailsmethod before fetching endpoints from fnd-serverHow has this been tested?
Screenshots (if appropriate):
Types of changes
Checklist: