Feature/lit 2961 auth unification breaking remove existing authsigs #444

Ansonhkg · 2024-04-25T21:00:30Z

Description

Removed authSig as an option to pass in to endpoint functions eg. pkpSign executeJs and signSessionKey
Extracted out all parser, helper, transcompiler functions in the pkpSign and executeJs to a dedicated helpers folder with isolated unit tests
Removing getXXPromiseShares methods in the LitNodeClient & its interface, and replacing it with a generic generatePromise instead. See lit-node-client-nodejs.ts
Better types for IOs

Note

The commit history is a bit overlapping with other PRs because I need the tests and some other features, please review from the commit below onward (or I just open a new PR and cherry pick the commits if that's better, lmk.)

35ede36

Type of change

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)
This change requires a documentation update

How Has This Been Tested?

Unit test (Jest)
E2E test (Tinny)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

…d due to storage provider not found

…to feature/lit-3017-auth-unification-migrate-actual-tests

…reation' into feature/lit-3017-auth-unification-migrate-actual-tests

…ode-fetch as dev dependency for now

joshLong145

Left some comments. looking good :)

packages/auth-helpers/src/lib/resources.ts

packages/types/src/lib/interfaces.ts

joshLong145 · 2024-04-26T18:49:09Z

packages/types/src/lib/interfaces.ts

+  pkpPublicKey?: string;
+  authSig?: AuthSig;
+  siweMessage: string;
+  curveType: 'BLS' | 'ECDSA';


I thought we had a LIT_CURVE introduced as a new enum could we it here?

yea that's in the constants package, it will unfortunately cause circular dependencies. I think constants and types should just merge, they are static values anyway and don't depend on any other packages, but they love each other and we keep separating them 💔

this makes sense to me, Will make a task as a follow up.

1. Removed authSig and use sessionSigs 2. Moved all parsers/helpers to its own folder with unit tests 3. Updated `ExecuteJsProps` type to `JsonExecutionSdkParams` type and `JsonExecutionRequest` type to nodes 4.

…t test

joshLong145 · 2024-04-29T20:28:01Z

@Ansonhkg this looks awesome!
Thinking to get ready for this merger we should make a release/6.0.0 branch where we can start merging this work into and doing test runs. WDYT?
we then could start merging #444 #436

DashKash54 · 2024-04-29T21:50:13Z

@Ansonhkg this looks awesome! Thinking to get ready for this merger we should make a release/6.0.0 branch where we can start merging this work into and doing test runs. WDYT? we then could start merging #444 #436

I think we should merge #435 instead of these sub-branches

DashKash54

Starting to review from the commit: 35ede36. Lemme know if this needs to be reviewed before this commit
Full disclosure: I'm glossing over the helper files in the the lib/helpers directory as I presume it's only a copy paste from the existing code?
The AuthSig removal happens only in packages/types/src/lib/interfaces.ts and replacing getSessionOrAuthSig with getSessionSigByUrl? Anything else?

packages/constants/src/lib/constants/endpoints.ts

packages/encryption/src/lib/params-validators.ts

packages/lit-node-client-nodejs/src/lib/helpers/get-claims-list.ts

packages/lit-node-client-nodejs/src/lib/helpers/get-claims.ts

packages/lit-node-client-nodejs/src/lib/lit-node-client-nodejs.ts

packages/types/src/lib/interfaces.ts

packages/lit-node-client-nodejs/src/lib/helpers/get-claims-list.ts

joshLong145 · 2024-04-30T13:24:42Z

packages/lit-node-client-nodejs/src/lib/helpers/get-signatures.ts

+      const bigR = typedItem.bigR ?? typedItem.bigr;
+
+      typedItem.signatureShare = (typedItem.signatureShare ?? '').replaceAll(
+        '"',
+        ''
+      );
+      typedItem.bigR = (bigR ?? '').replaceAll('"', '');
+      typedItem.publicKey = (typedItem.publicKey ?? '').replaceAll('"', '');
+      typedItem.dataSigned = (typedItem.dataSigned ?? '').replaceAll('"', '');


Do we do this in more than just here? thinking this might be better as a separate helper for sanitizing signatures.

Yea, I think so. created a ticket for this https://linear.app/litprotocol/issue/LIT-3082/[js-sdk]-separate-flatten-shares-helper-function

packages/lit-node-client-nodejs/src/lib/helpers/normalize-array.ts

joshLong145 · 2024-04-30T19:42:15Z

packages/lit-node-client-nodejs/src/lib/helpers/get-signatures.ts

+  minNodeCount: number;
+  signedData: any[];
+  requestId: string;
+}): T | { signature: SigResponse; sig: SigResponse } => {


I'm wondering if we could standardize on one of the two return values so we can remove T as it is a bit confusing the consumer of this method.

DashKash54 · 2024-05-01T06:56:58Z

@Ansonhkg Still a bunch of unresolved comments. Can you please respond and resolve those, thanks

DashKash54

Approving the PRs though there are still some unresolved comments which aren't addressed. Please address those and resolve. Great job @Ansonhkg 🚀

…github.com/LIT-Protocol/js-sdk into feature/lit-2961-auth-unification-breaking-remove-existing-authsigs

Ansonhkg added 10 commits April 24, 2024 15:28

fix(sessionKey): signed session key mismatch with the one being signe…

5b7e311

…d due to storage provider not found

chore: pretty pretty lint

4344d6a

Merge branch 'feature/lit-2960-auth-unification-lit-endpoints-map' in…

30840f1

…to feature/lit-3017-auth-unification-migrate-actual-tests

Merge branch 'feature/lit-2962-auth-unification-better-siwe-authsig-c…

b954e37

…reation' into feature/lit-3017-auth-unification-migrate-actual-tests

feat(accs): added compose lit action resource string helper function

72bcd8d

feat(types): better interfaces

fe71ddf

feat(test): add new e2e tests

430abb7

fix: unable to polyfill fetch when using cross-fetch, replaced with n…

757d588

…ode-fetch as dev dependency for now

chore: pretty pretty pretty lint

84f7095

feat(migration): update pkpSign function

35ede36

Ansonhkg changed the base branch from master to feature/lit-2958-auth-unification-ts-tests April 25, 2024 21:01

joshLong145 reviewed Apr 26, 2024

View reviewed changes

Ansonhkg added 6 commits April 26, 2024 22:25

feat(remove-authSig):

45f9eed

1. Removed authSig and use sessionSigs 2. Moved all parsers/helpers to its own folder with unit tests 3. Updated `ExecuteJsProps` type to `JsonExecutionSdkParams` type and `JsonExecutionRequest` type to nodes 4.

fix(tinny): set default MAX_ATTEMPT = 1

63e3b54

fix(executeJs): jsParam is optional

e4c3e79

fix: node promises. now pkpSign & executeJs work!

e01daaa

feat: add pkp sign response parser as its own helper function and uni…

e2adf7d

…t test

fix: #444 (comment)

1b9dd2c

Ansonhkg marked this pull request as ready for review April 27, 2024 02:03

Ansonhkg requested review from FedericoAmura, MaximusHaximus, DashKash54, jtary and glitch003 April 27, 2024 02:03

chore: remove unused function

500967c

joshLong145 self-requested a review April 29, 2024 20:26

joshLong145 mentioned this pull request Apr 29, 2024

Feature/lit 3041 js sdk cherry pick fixes for session key mismatch 443#issuecomment 2082614246 #446

Merged

14 tasks

DashKash54 requested changes Apr 29, 2024

View reviewed changes

Ansonhkg added 2 commits April 30, 2024 14:42

fix: #444 (comment)

e30165e

fix: #444 (comment)

3b66113

joshLong145 reviewed Apr 30, 2024

View reviewed changes

Ansonhkg added 2 commits April 30, 2024 14:55

fix: #444 (comment)

65e5f21

fix: #444 (comment)

ffa6555

Ansonhkg requested review from DashKash54 and joshLong145 April 30, 2024 14:08

fix: misleading function name

dce9581

joshLong145 reviewed Apr 30, 2024

View reviewed changes

DashKash54 approved these changes May 1, 2024

View reviewed changes

Merge branch 'feature/lit-2958-auth-unification-ts-tests' of https://…

bc3adbd

…github.com/LIT-Protocol/js-sdk into feature/lit-2961-auth-unification-breaking-remove-existing-authsigs

Ansonhkg merged commit 8709d11 into feature/lit-2958-auth-unification-ts-tests May 1, 2024
2 checks passed

Ansonhkg deleted the feature/lit-2961-auth-unification-breaking-remove-existing-authsigs branch May 1, 2024 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/lit 2961 auth unification breaking remove existing authsigs #444

Feature/lit 2961 auth unification breaking remove existing authsigs #444

Ansonhkg commented Apr 25, 2024 •

edited

Loading

joshLong145 left a comment

joshLong145 Apr 26, 2024

Ansonhkg Apr 27, 2024

joshLong145 Apr 29, 2024

joshLong145 commented Apr 29, 2024

DashKash54 commented Apr 29, 2024

DashKash54 left a comment

joshLong145 Apr 30, 2024

Ansonhkg Apr 30, 2024

joshLong145 Apr 30, 2024

DashKash54 commented May 1, 2024

DashKash54 left a comment

Feature/lit 2961 auth unification breaking remove existing authsigs #444

Feature/lit 2961 auth unification breaking remove existing authsigs #444

Conversation

Ansonhkg commented Apr 25, 2024 • edited Loading

Description

Note

Type of change

How Has This Been Tested?

Checklist:

joshLong145 left a comment

Choose a reason for hiding this comment

joshLong145 Apr 26, 2024

Choose a reason for hiding this comment

Ansonhkg Apr 27, 2024

Choose a reason for hiding this comment

joshLong145 Apr 29, 2024

Choose a reason for hiding this comment

joshLong145 commented Apr 29, 2024

DashKash54 commented Apr 29, 2024

DashKash54 left a comment

Choose a reason for hiding this comment

joshLong145 Apr 30, 2024

Choose a reason for hiding this comment

Ansonhkg Apr 30, 2024

Choose a reason for hiding this comment

joshLong145 Apr 30, 2024

Choose a reason for hiding this comment

DashKash54 commented May 1, 2024

DashKash54 left a comment

Choose a reason for hiding this comment

Ansonhkg commented Apr 25, 2024 •

edited

Loading