Feature/lit 3017 auth unification migrate actual tests #440

Ansonhkg · 2024-04-24T15:24:38Z

Description

This PR merges several other PRs #439 #438 #436, so we can run the tests as they rely on the new features introduced in those branches.

Please check this directory for all the tests

Changes

Introduced a helper function for composing an access control resource string. Previously, we only required dataToHash, but it turns out that the string of the hashed access control string must be prepended before it. See 72bcd8d.
Better types on Node response fe71ddf
Unfortunately node-fetch is needed when running against local tests for some reasons. The package is added a dev dependency so won't affect production. It's only being used in this test. 757d588

Also want to note that we should write scenario in the test/JsDocs for more complex operations, see example

js-sdk/local-tests/tests/testDelegatingCapacityCreditsNFTToAnotherWalletToExecuteJs.ts

Lines 6 to 19 in 757d588

    
           /** 
        
            * ## Scenario: 
        
            * Delegating capacity credits NFT to Bob (delegatee) for him to execute JS code to sign with his PKP 
        
            * - Given: The capacity credits NFT is minted by the dApp owner 
        
            * - When: The dApp owner creates a capacity delegation authSig 
        
            * - And: The dApp owner delegates the capacity credits NFT to Bob 
        
            * - Then: The delegated (Bob's) wallet can execute JS code to sign with his PKP using the capacity from the capacity credits NFT 
        
            * 
        
            * 
        
            * ## Test Commands: 
        
            * - ❌ Not supported in Cayenne, but session sigs would still work 
        
            * - ✅ NETWORK=manzano yarn test:local --filter=testDelegatingCapacityCreditsNFTToAnotherWalletToExecuteJs 
        
            * - ✅ NETWORK=localchain yarn test:local --filter=testDelegatingCapacityCreditsNFTToAnotherWalletToExecuteJs 
        
            */

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?

 NETWORK=localchain DEBUG=true yarn test:local

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

DashKash54 · 2024-04-25T00:02:41Z

This PR merges several other PRs #439 #438 #436, so we can run the tests as they rely on the new features introduced in those branches.

Is it juts a merged PR? So what's to review here? @Ansonhkg

Ansonhkg · 2024-04-25T00:03:56Z

This PR merges several other PRs #439 #438 #436, so we can run the tests as they rely on the new features introduced in those branches.

Is it juts a merged PR? So what's to review here? @Ansonhkg

The tests & the helper function

glitch003

crazy - so many tests! thx for your work here @Ansonhkg

DashKash54

All these tests have been tested against the Auth Uni node PR by spinning up the local nodes? Have they also been tested on Manzano?
Can we add some tests for that fail due to the PKP SessionSigs not having enough resource permissions?
So to summarize this PR tests all the functionality with eoaSessionSigs, pkpSessionSigs & litActionSessionSigs?

local-tests/test.ts

DashKash54 · 2024-04-29T20:34:45Z

local-tests/tests/testDelegatingCapacityCreditsNFTToAnotherPkpToExecuteJs.ts

+        ability: LitAbility.LitActionExecution,
+      },
+    ],
+    capabilityAuthSigs: [capacityDelegationAuthSig],


Is there any way of knowing whether the Capacity credit was used? It's enforced on Manzano & Habanero but I think you're these tests against local nodes where capacity credits aren't required?

You're testing it against only the local nodes since Manzano won't have these changes? But local nodes don't enforce capacity credits so we aren't really testing this? @Ansonhkg

DashKash54 · 2024-04-29T20:39:00Z

local-tests/tests/testDelegatingCapacityCreditsNFTToAnotherWalletToExecuteJs.ts

+
+    const regex = /urn:recap:[\w+\/=]+/g;
+
+    const recaps = bobsSingleSessionSig.signedMessage.match(regex) || [];


Will EOA sessionSigs always contain Lit Action/PKP Sign resources?

yea, if you don't provide them. It's just a helper function within the test environment so won't be used in the sdk

I'm sorry I didn't get it. In the actual SDK does the EOA sessionSig by default have these resources? I think it's enforced by the nodes? @Ansonhkg

no, in the actual SDK we don't have set resources by default.

DashKash54 · 2024-04-29T20:43:06Z

local-tests/tests/testUseCapacityDelegationAuthSigWithUnspecifiedCapacityTokenIdToExecuteJs.ts

How is it different from testDelegatingCapacityCreditsNFTToAnotherWalletToExecuteJs?
What do you mean by unspecified capacity token? In case of it being specified where exactly is it specified?

unspecified capacity token - meaning you are having the capacityTokenId field omitted in the SDK, which also mean that you are having the nft_id omitted from the node side, which means that there's no nft_id to scope into, aka. the function does not limit the scope of the query to specific NFT IDs, meaning it assumes that all NFTs owned by the user (as determined by their address) are eligible.

To ensure that we're on the same page, can you please explain what capacityTokenId & nft_id are used for. An example would help @Ansonhkg

capacityTokenId is the sdk param, and nft_id is the node param

DashKash54 · 2024-04-29T20:50:35Z

local-tests/tests/testUseCapacityDelegationAuthSigWithUnspecifiedCapacityTokenIdToPkpSign.ts

+
+    const appOwnersCapacityDelegationAuthSig = (
+      await devEnv.litNodeClient.createCapacityDelegationAuthSig({
+        dAppOwnerWallet: alice.wallet,


Why do we have multiple ways to create a delegationAuthSig? Wouldn't this confuse the users? Why is providing dAppOwnerWallet when you can simply create the authSig with the wallet itself?

we do not have multiple ways to create it, those you saw are helper functions within the test.

re: dAppOwnerWallet you can simply create the authSig yourself

yes, you technically can, but this function has an explicit behaviour that matches user's intention when they call it, whereas "simply create the authSig yourself" is implicit, you can use it if you know what you're doing.

So this is only for testing? The SDK only provides a one way to do so? @Ansonhkg

no, the SDK now provides 3 ways using the as outlined in this PR #436
a generic one with implicit behaviour, and 2 with explicit behaviours

DashKash54

Left a few more imp comments

local-tests/tests/testUseValidLitActionCodeGeneratedSessionSigsToEncryptDecryptFile.ts

DashKash54 · 2024-04-29T21:09:47Z

local-tests/tests/testUseValidLitActionCodeGeneratedSessionSigsToExecuteJsClaimMultipleKeys.ts

+      Lit.Actions.claimKey({keyId: "foo"});
+      Lit.Actions.claimKey({keyId: "bar"});


Isn't there a step required before claiming a PKP? Someone has to make it claimable right or can anyone claim any PKP? @joshLong145

@DashKash54 the keyId is postfixed with the ipfs id so it makes the claim unique to the Lit Action. So you are only claim in context to the lit action as the app id

Hmm I thought the app had to do something before for making the PKP claimable. Turns out your just need to provide the keyId? @joshLong145

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

DashKash54 · 2024-04-29T21:20:07Z

packages/types/src/lib/interfaces.ts

@Ansonhkg Can you please double check all you interfaces against the node counter parts eg: https://github.com/LIT-Protocol/lit-assets/blob/aff82c4381139476aeaa6faa814069b184d38d39/rust/lit-node/src/models/mod.rs#L177
Note, some params are still accepted in the nodes for backwards compatible reasons but they don't need to be present in the SDK as we're not making the SDK backwards compatible with the older node versions
For instance, we don't allow passing an AuthSig for signSessionKey: https://github.com/LIT-Protocol/lit-assets/blob/aff82c4381139476aeaa6faa814069b184d38d39/rust/lit-node/src/endpoints/versions/v1.rs#L59
There are bunch of other restrictions for the v1 endpoint in the above node file. We should make the SDK interfaces compatible with those restriction as well

@Ansonhkg this comment is remaining

Yea has been addressed in one of the comments below, will double check again here #435

packages/types/src/lib/interfaces.ts

DashKash54

Left some more comments

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

DashKash54 · 2024-04-29T21:30:37Z

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

-      statement,
-    } = params;
+    // Useful log for debugging
+    if (!params.delegateeAddresses || params.delegateeAddresses.length === 0) {


How about making passing delegateeAddresses mandatory & when provided empty array that means anyone can use it?

What do you think @Ansonhkg?

Providing an empty array means no one can use it though, I think we should not allow an empty array instead. btw the log message has been updated in another PR that's merged to the staging. Noting it here #435

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

decriptedFile: Uint8Array(11) [ 72, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100 ] ✔ 1. testUseValidLitActionCodeGeneratedSessionSigsToEncryptDecryptFile - Passed (5459.23 ms)

DashKash54 · 2024-05-01T06:43:35Z

packages/types/src/lib/interfaces.ts

+ * FIXME: We should create a separate interface for JsExecutionRequestBody
+ * a body that the SDK accepts, and another one the node actually accepts.


Why can't we have a single interface?

DashKash54 · 2024-05-01T06:45:10Z

packages/lit-auth-client/src/lib/providers/BaseProvider.ts

-            statement: authCallbackParams.statement,
-            sessionKey: params.sessionSigsParams.sessionKey,
+          authSpecificData = {
+            authSig: JSON.parse(params.authMethod.accessToken),


SignSessionKey no longer accepts AuthSig

DashKash54 · 2024-05-01T06:49:12Z

Responded to your comments & left a couple new comments. After these are addresses this is good to merge 😄 @Ansonhkg

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-3017-auth-unification-migrate-actual-tests

…` return types

Ansonhkg self-assigned this Apr 24, 2024

Ansonhkg requested review from glitch003, joshLong145, MaximusHaximus, DashKash54, FedericoAmura and jtary April 24, 2024 15:33

Ansonhkg marked this pull request as ready for review April 24, 2024 15:34

Ansonhkg changed the base branch from master to feature/lit-2958-auth-unification-ts-tests April 24, 2024 15:38

Ansonhkg requested a review from spacesailor24 April 24, 2024 15:40

glitch003 approved these changes Apr 25, 2024

View reviewed changes

Ansonhkg mentioned this pull request Apr 25, 2024

Auth Unification, Custom Auths, better tests #435

Merged

18 tasks

DashKash54 requested changes Apr 29, 2024

View reviewed changes

DashKash54 mentioned this pull request Apr 29, 2024

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

Merged

14 tasks

Ansonhkg added 12 commits April 30, 2024 15:48

fix: #440 (comment)

9c4077b

decriptedFile: Uint8Array(11) [ 72, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100 ] ✔ 1. testUseValidLitActionCodeGeneratedSessionSigsToEncryptDecryptFile - Passed (5459.23 ms)

fix: only BLS #440 (comment)

e01e076

fix: #440 (comment)

bcad173

fix: #440 (comment)

451e0e7

chore: added note #440 (comment)

d428bad

fix: improve readability

eaf014b

fix: pass-in session key in base provider

660759f

feat: cherry-pick ee7011a

f30a10a

fix: pass-in session key in base provider

8b92666

fix: #446 (comment)

b4a53db

feat: merge #446

1c17275

fix: remove TEMP_CACHE

8a8694a

Ansonhkg added 2 commits April 30, 2024 16:33

fix: #440 (comment)

3b7de45

fix: #440 (comment)

e424daf

Ansonhkg requested a review from DashKash54 April 30, 2024 15:38

fix: #440 (comment)

6e0e4ef

DashKash54 reviewed May 1, 2024

View reviewed changes

DashKash54 approved these changes May 1, 2024

View reviewed changes

Ansonhkg added 2 commits May 1, 2024 15:12

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

2695204

…github.com/LIT-Protocol/js-sdk into feature/lit-3017-auth-unification-migrate-actual-tests

fix: remove authSig (need to fix decryptToString and `decryptToFile…

7bc363d

…` return types

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

Ansonhkg deleted the feature/lit-3017-auth-unification-migrate-actual-tests branch May 1, 2024 16:11

joshLong145 mentioned this pull request May 13, 2024

Staging/v6 #464

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/lit 3017 auth unification migrate actual tests #440

Feature/lit 3017 auth unification migrate actual tests #440

Ansonhkg commented Apr 24, 2024 •

edited

Loading

DashKash54 commented Apr 25, 2024

Ansonhkg commented Apr 25, 2024 •

edited

Loading

glitch003 left a comment

DashKash54 left a comment

DashKash54 Apr 29, 2024

DashKash54 May 1, 2024 •

edited

Loading

DashKash54 Apr 29, 2024

Ansonhkg Apr 30, 2024

DashKash54 May 1, 2024 •

edited

Loading

Ansonhkg May 1, 2024

DashKash54 Apr 29, 2024

Ansonhkg Apr 30, 2024 •

edited

Loading

DashKash54 May 1, 2024 •

edited

Loading

Ansonhkg May 1, 2024

DashKash54 Apr 29, 2024

Ansonhkg Apr 30, 2024

DashKash54 May 1, 2024 •

edited

Loading

Ansonhkg May 1, 2024

DashKash54 left a comment

DashKash54 Apr 29, 2024

joshLong145 Apr 30, 2024

DashKash54 May 1, 2024

DashKash54 Apr 29, 2024

DashKash54 May 1, 2024

Ansonhkg May 1, 2024

DashKash54 left a comment

DashKash54 Apr 29, 2024

DashKash54 May 1, 2024

Ansonhkg May 1, 2024 •

edited

Loading

DashKash54 May 1, 2024

DashKash54 May 1, 2024

DashKash54 commented May 1, 2024 •

edited

Loading

DashKash54 left a comment

	/**
	* ## Scenario:
	* Delegating capacity credits NFT to Bob (delegatee) for him to execute JS code to sign with his PKP
	* - Given: The capacity credits NFT is minted by the dApp owner
	* - When: The dApp owner creates a capacity delegation authSig
	* - And: The dApp owner delegates the capacity credits NFT to Bob
	* - Then: The delegated (Bob's) wallet can execute JS code to sign with his PKP using the capacity from the capacity credits NFT
	*
	*
	* ## Test Commands:
	* - ❌ Not supported in Cayenne, but session sigs would still work
	* - ✅ NETWORK=manzano yarn test:local --filter=testDelegatingCapacityCreditsNFTToAnotherWalletToExecuteJs
	* - ✅ NETWORK=localchain yarn test:local --filter=testDelegatingCapacityCreditsNFTToAnotherWalletToExecuteJs
	*/


		const regex = /urn:recap:[\w+\/=]+/g;

		const recaps = bobsSingleSessionSig.signedMessage.match(regex) \|\| [];

		Lit.Actions.claimKey({keyId: "foo"});
		Lit.Actions.claimKey({keyId: "bar"});

		* FIXME: We should create a separate interface for JsExecutionRequestBody
		* a body that the SDK accepts, and another one the node actually accepts.

Feature/lit 3017 auth unification migrate actual tests #440

Feature/lit 3017 auth unification migrate actual tests #440

Conversation

Ansonhkg commented Apr 24, 2024 • edited Loading

Description

Changes

Type of change

How Has This Been Tested?

Checklist:

DashKash54 commented Apr 25, 2024

Ansonhkg commented Apr 25, 2024 • edited Loading

glitch003 left a comment

Choose a reason for hiding this comment

DashKash54 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DashKash54 May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DashKash54 May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ansonhkg Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

DashKash54 May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DashKash54 May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DashKash54 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DashKash54 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ansonhkg May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DashKash54 commented May 1, 2024 • edited Loading

DashKash54 left a comment

Choose a reason for hiding this comment

Ansonhkg commented Apr 24, 2024 •

edited

Loading

Ansonhkg commented Apr 25, 2024 •

edited

Loading

DashKash54 May 1, 2024 •

edited

Loading

DashKash54 May 1, 2024 •

edited

Loading

Ansonhkg Apr 30, 2024 •

edited

Loading

DashKash54 May 1, 2024 •

edited

Loading

DashKash54 May 1, 2024 •

edited

Loading

Ansonhkg May 1, 2024 •

edited

Loading

DashKash54 commented May 1, 2024 •

edited

Loading