Wrapped Keys

[MaximusHaximus]

Description

• Updates wrapped keys to support multiple networks; Production for Habanero (and Datil in the future), TestNetworks for Cayenne and Manzano
• Adds new keyType and publicKey keys to the persisted metadata, and requires they are provided when consumers call the methods that require them
• Organizes types, constants, and logic by domain -- lit-actions-client contains all code specific to interacting with the lit actions, service-client contains all of the same for interactions with the wrapped keys backend, and api contains definitions of all top-level API methods that will be exposed to the consumer of the package
• Explicitly exporting types and methods from the top-level index.ts rather than blanket exporting everything in the package with * exports. The goal here is to facilitate us being able to modify the LIT action APIs without making breaking changes to the top-level API methods that consumers are using as we need to.
• Added a bunch of JSDocs and explicit types for parameters for API-exposed methods and their return values
• Removed a lot of duplicated JSDoc comments by using @typedef and @extends
• Removed the customXXXX methods, in favour of us instead add a basic documentation blurb to the JSDocs for prospective integrators that shows them how easy it is to get the private key metadata from the wrapped keys service, and then submit it to any custom LIT action, using the LitNodeClient instance that they already have in scope. The custom behaviour in the SDK exposes internals of LIT actions to our consumer API, and would cause us to get tickets against the wrapped-keys SDK that are actually issues with custom LIT Action implementations on top of making the API worse for consumers of the package who want to use 'supported' networks.

e.g.:

import { getPrivateKeyMetadata } from '@lit-protocol/wrapped-keys'

import { litNodeClient } from '@lit-protocol/lit-node-client'

await litNodeClient.connect({litNetwork: 'cayenne'});
const sessionSigs = await litNodeClient.getSessionSigs(...);
const keyMetadata = await getPrivateKeyMetadata({ litNodeClient, sessionSigs})
const customLitActionWithWrappedKeyResponse = await litNodeClient.executeJs({
  sessionSigs,
  ipfsId: litActionIpfsCid, // OR
  code: litActionCode, 
  jsParams: {
    accessControlConditions,
    keyMetadata,
  },
});

// Do whatever you want to with `customLitActionWithWrappedKeyResponse

Type of change

• 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?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Ran all local tests against Cayenne. The only ones that fail say they intentionally fail (?!?!?!)

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

[spacesailor24]

The local-tests/tests/testImportWrappedKey.ts test is importing a Solana key but says the keyType is K256

This test also isn't really testing if the key was imported correctly, but I think we'd have to use either getEncryptedKeyMetadata or the sign methods to verify it was

[spacesailor24]

Curious why we're wrapping this in an object, does this make it not tree shakeable if a user is only using one or a few of these methods?

[DashKash54]

@MaximusHaximus ^

[MaximusHaximus]

It's still tree-shakeable; bundlers will only include the values from the exported object that the consumer actually uses, and these are all specific functions, rather than classes with mixed dependencies.

I actually had considered adding a createClient({litNodeClient, pkpSessionSigs}) method to the module exports, which would eliminate those 2 arguments from all of our API functions, simplify all of our typescript types and make our function signature more concise and easy to understand -- but decided not to because the return value of createClient() would not have been tree-shakeable and would have always included all of the functions.

[DashKash54]

If both are tree-shakable then what's the reason behind export const api = {..} instead of directly exporting the individual functions? @MaximusHaximus

[spacesailor24]

It would be nice if we could restrict this to the supported EVM networks for EthereumLitTransactions and mainnet-beta | testnet | devnet for Solana txs

[DashKash54]

Sure for later, can you cut a ticket for tracking this @spacesailor24 and comment here

[spacesailor24]

Yes it should, however, in my testing waiting for the transaction hash always resulted in the Lit Action timing out. I'm going to add the following code to the docs to show users how to obtain the tx hash/receipt outside of the Lit Action:

// Assuming signature is in base64, convert it back to byte array for querying
const byteSignature = Buffer.from(signature, 'base64');

// Wait for confirmation and fetch the transaction details
const confirmation = await solanaConnection.confirmTransaction(byteSignature);
console.log('Transaction confirmation status:', confirmation.value);

// Get the transaction receipt
const receipt = await solanaConnection.getTransaction(byteSignature.toString('base64'), { commitment: 'confirmed' });
console.log('Transaction receipt:', receipt);

[MaximusHaximus]

If we can't keep the return values and behaviours of the functions consistent, it might make more sense to split them into separately named methods at the API layer. It's already a bit funky that we have a union type for the transaction -- if the return values and behaviour of the actual LIT actions differ as well, keeping them under a single method name in our API is probably hurting more than helping consumers of the SDK.

Can you look into this approach and see if it will work for us? @spacesailor24
https://solana.stackexchange.com/a/24

[spacesailor24]

Are there tests for getEncryptedKeyMetadata and storeEncryptedKeyMetadata?

[CLAassistant]

All committers have signed the CLA.

[DashKash54]

Left a few comments

[DashKash54]

nit: Why remove the constant?

[MaximusHaximus]

So once we started restricting network to be specific values using a string union type, I discovered that if someone used the const to call our method, the typescript checker would throw an error TS2322: Type string is not assignable to type Network -- because the type of the constant is string, decides that it is too generate -- it's not a specific string literal.

So then I tried defining these as:

export const NETWORK_EVM: Network = 'evm';
export const NETWORK_SOLANA: Network = 'solana';

That did fix the consumers code's typechecking, but then, inside the SDK I started getting TS checker errors in the lit-actions-client/constants.ts file where we define the LIT_ACTION_CID_REPOSITORY:

TS2739: Type Readonly<{ [x: string]: 'QmdYUhPCCK5hpDWMK1NiDLNLG6RZQy61QE4J7dBm1Y2nbA' | 'QmSi9GL2weCFEP1SMAUw5PDpZRr436Zt3tLUNrSECPA5dT'; }> is missing the following properties from type Readonly<Record<Network, string>>: evm, solana

Even though the entry clearly has the correct keys in it, defined by their constants :(

import { LitCidRepository } from './types';
import { NETWORK_EVM, NETWORK_SOLANA } from '../constants';

import { Network } from '../types';

export type LitActionType =
  | 'signTransaction'
  | 'signMessage'
  | 'generateEncryptedKey';

export type LitCidRepositoryEntry = Readonly<Record<Network, string>>;

export type LitCidRepository = Readonly<
  Record<LitActionType, LitCidRepositoryEntry>
>;

const LIT_ACTION_CID_REPOSITORY: LitCidRepository = {
  signTransaction: Object.freeze({
    [NETWORK_EVM]: 'QmdYUhPCCK5hpDWMK1NiDLNLG6RZQy61QE4J7dBm1Y2nbA',
    [NETWORK_SOLANA]: 'QmSi9GL2weCFEP1SMAUw5PDpZRr436Zt3tLUNrSECPA5dT',
  }),
  signMessage: Object.freeze({
    [NETWORK_EVM]: 'QmTMGcyp77NeppGaqF2DmE1F8GXTSxQYzXCrbE7hNudUWx',
    [NETWORK_SOLANA]: 'QmYQC6cd4EMvyB4XPkfEEAwNXJupRZWU5JsTCUrjey4ovp',
  }),
  generateEncryptedKey: Object.freeze({
    [NETWORK_EVM]: 'QmaoPMSqcze3NW3KSA75ecWSkcmWT1J7kVr8LyJPCKRvHd',
    [NETWORK_SOLANA]: 'QmdRBXYLYvcNHrChmsZ2jFDY8dA99CcSdqHo3p1ES3UThL',
  }),
};

export { LIT_ACTION_CID_REPOSITORY };

In the end, I discovered that I had to use string literals in the LIT_ACTION_CID_REPOSITORY to make TS happy; I couldn't use our consts as keys. Then I was faced with a conundrum; now external consumers would be using constants that we didn't use ourselves. Since TS can't use runtime data as types, I went the opposite direction and exported the string union type. I did some digging around the net to try and identify a solution that would let me use the same consts that consumers used to define the keys on our repository, but couldn't find a solution and it was taking quite a lot of time at that point.

Given that these are user-facing arguments, maybe it'd be fine for our internal files to use string constants when defining actual records, but external consumers still have constants to use if they want to? I've pushed a commit that makes that change -- take a look and LMK what you think. I also added exported constants for keyType using the same pattern.

[DashKash54]

Why have we changed these to interfaces do we plan to extend it?

[MaximusHaximus]

Nothing changed -- our ESLint rule configuration (and SDK-wide convention) enforces that we always use interfaces unless we are using features that require a type. I ran eslint --fix on the file and it fixed the entries that should have been interfaces :). See first commit in the pr here: 21cfaf3

[DashKash54]

Then EsLint is not being run in the CI? @MaximusHaximus @Ansonhkg ?

[MaximusHaximus]

That's right, there is no mutative CI that changes files, and not everyone has their editors set to run eslint --fix on save, so these things happen ™️. It's been on the roadmap to implement for a few months.

[MaximusHaximus]

Are there tests for getEncryptedKeyMetadata and storeEncryptedKeyMetadata?

Not directly; they are used by all of the other tests, so are extensively tested alreeady

[MaximusHaximus]

Addressed feedback and responded to some questions with questions @DashKash54 :)

[DashKash54]

This looks good

[DashKash54]

LGTM!