LIT-3920 - wrapped-keys - Batch generate private keys & optionally sign messages in a single operation

[MaximusHaximus]

Description

This PR adds a new API function to wrapped-keys and corresponding LITAction to wrapped-keys-lit-actions facilitates the execution of multiple operations -- previously requiring executing 4 independent actions -- in a single API call to a single LIT action execution.

Noteworthy changes in this PR:

• Refactored existing LIT actions to allow composition of existing generation and message signing functionality into the new LIT action, while using the same code in the existing LIT actions that individually generate keys or sign messages with keys for a specific network (evm or solana)
• Added a new LIT_ACTION_CID_REPOSITORY_COMMON constant, and a new litActionCodeRepositoryCommon constant to wrapped-keys; 'common' LIT actions are not network-specific, so these repositories do not have multiple entries keyed by Network (evm or solana) under each LIT action entry; instead there is a single entry per LIT action which provides functionality for multiple networks.

wrapped-keys

New functions

api.batchGeneratePrivateKeys()

• Takes an array of actions, where each action can define either network (evm or solana), and an optional message to sign and return the signature of

config.setLitActionsCodeCommon()

• Allows injection of a LIT actions code repository containing 'common' LIT actions that are not defined per-network

New types

BatchGeneratePrivateKeysResult / BatchGeneratePrivateKeysActionResult

BatchGeneratePrivateKeysParams / GeneratePrivateKeyAction

LitActionCodeRepositoryCommon

wrapped-keys-lit-actions

New LIT action

batchGenerateEncryptedKeys()

• Facilitates generating n new keys and optionally signing a message for each key, and returning the results of all generated keys and signed messages in a single execution of one LIT action

Type of change

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

How Has This Been Tested?

• Ran all wrapped-keys localtests and added a new one to test the new batch generate/sign LIT action
• Wrote and executed custom tests to verify that existing LIT actions continue to function identically to how they did before the internal refactors done in this PR

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

[MaximusHaximus]

This makes our tinny tests use the locally built code for all LIT actions in wrapped-keys instead of running them against the CIDs in wrapped-keys LIT_ACTION_CID_REPOSITORY

[DashKash54]

Since the idea is that users use the IPFS Cid shouldn't the tests reflect that?

[MaximusHaximus]

I guess it depends on the users; some people will be using the code directly, others the CIDs... what I want to do is run the entire suite against the cids and then again against the current code ideally, but see other comment re: doing so without jest.

The reason this is here right now is because we have a chicken and egg problem - in-progress code in PRs isn't in IPFS, and pinning repeatedly to IPFS as we iterate on a lit action would result in a ton of churn and manual updates right now.

We could make this code always use IPFS CIDs, but it would require that all actions we want to test would exist in IPFS before we can test against them -- and we want to run tests while developing the actions where they will be changing often.

In this PR, I wanted to be sure tests pass with the current code rather than previous CIDs -- once we have confirmed all of the LIT actions code here is GTG and published all of the lit actions to IPFS, I can update the CIDs in wrapped-keys and remove this code so the tests run against CIDs again

[DashKash54]

Cool as you mentioned we can run the tests against IPFS afterwards

[FedericoAmura]

I think we should stick to the LA code, not the IPFS. That way we can validate the code with our tests continuously, not depend on IPFS and we can always calculate the CID from the code but not the other way around
Also, after having the CID generation/LA publish step on SDK publish we will have them updated automatically every time this goes public

[MaximusHaximus]

👍 I agree @FedericoAmura <3 Ideally, all tests would run against the data in the same commit by default, and if we want to test against prior versions we'd explicitly do that. The main thing is just getting the CID repo in the same commit to always match the current source code in that commit... I think we can do that with a husky hook script that runs on-commit

[DashKash54]

husky hook script that runs on-commit

Good idea

[DashKash54]

@MaximusHaximus there appears to be a problem in the batch method as each node generates its own random private key and signs with it. But only one node returns the encrypted key to the user hence the encrypted key won't match the signedMessage

[DashKash54]

Why "empty"?

[MaximusHaximus]

The way the fallback logic inside of the wrapped-keys package works, if the code for a particular LIT action is an empty string (falsey), it uses CIDs -- otherwise it uses the string as the LIT action code. It's 'empty' because it contains no code at all, for any key -- basically it resets the entire code repository so that any calls to use the LIT actions will use the defined CIDs instead of trying to use code. For type safety it needs to be an object with the correct keys, where all appropriate values remain strings.

[DashKash54]

The way the fallback logic inside of the wrapped-keys package works, if the code for a particular LIT action is an empty string (falsey), it uses CIDs -- otherwise it uses the string as the LIT action code

The fallback logic sounds a lil convoluted, like we intentionally keep it empty and then fall back- is it like this to work with TS? Doesn't the user provides a param to pass the code or IPFS Cid

[MaximusHaximus]

Keeping the values on the object always as strings keeps the type consistent so handling values in the object requires less complexity in typescript -- because values on the object are all of type string, we always know what type a value is without needing to do assertions against it, and the code to check if it's a falsy string to know if we should try to use any particular value as code or not is the same as if the value was 'null' -- but typescript doesn't force us to check it's a string or add a manual type annotation telling TS it IS a string before using it as code like it would if we had defined it as a { string | null } type :)

[DashKash54]

Few more comments

[MaximusHaximus]

@DashKash54 Per our conversation, I've pushed commits that remove all Lit.Actions.runOnce() usage in the new generate function, and moved the generation of new private keys inside of the runOnce blocks for existing keys.

Thinking about this change, though -- we could also normalize this behaviour and remove the runOnce usage for the existing LIT actions, and configure them to use the same new 'only use response from a single node' logic. This would improve performance for anyone using the per-network generate methods also. It would be a similar 1-line change 🤔

[FedericoAmura]

I think we should stick to the LA code, not the IPFS. That way we can validate the code with our tests continuously, not depend on IPFS and we can always calculate the CID from the code but not the other way around
Also, after having the CID generation/LA publish step on SDK publish we will have them updated automatically every time this goes public

[MaximusHaximus]

@DashKash54 I merged in the new executeJs change from #677 and updated batchGenerateKeysWithLitAction() to set it.

This behaviour leverages the existing executeJs function behaviour to get the absolute fastest response from any node and return immediately, as soon as any one successful response is received. This also gives us the added benefit that if any one request to any node succeeds, the call will succeed. Basically the best of both worlds when it comes to both speed of response and resilience to errors.

[DashKash54]

Looks good, approving but left few more comments & questions @MaximusHaximus

[DashKash54]

Just a small suggestion for big PRs like this it'll be nice to keep the name same and raise another PR for updating the naming, makes easier to review @MaximusHaximus

[DashKash54]

we could also normalize this behaviour and remove the runOnce usage for the existing LIT actions

We can update the existing Lit Actions later as it's not only the Lit Action but also the client that needs to be updated with run on single node flag eg: generatePrivateKey()

[DashKash54]

This behaviour leverages the existing executeJs function behaviour to get the absolute fastest response from any node and return immediately, as soon as any one successful response is received.

It has a down side that all the node loads increase, we can reduce the network load by 8-10x if we run only on once
We haven't experience Lit Action throwing random errors like this so we should be confident to make this change @MaximusHaximus @Ansonhkg

[MaximusHaximus]

we could also normalize this behaviour and remove the runOnce usage for the existing LIT actions

We can update the existing Lit Actions later as it's not only the Lit Action but also the client that needs to be updated with run on single node flag eg: generatePrivateKey()

OK, I'll leave them as is for now -- just FYI, the client change only involves adding a single parameter into each API function's file where they call executeJs -- not a big deal.

[DashKash54]

Please address the remaining comments specifically the ones in which I've tagged you @MaximusHaximus

[DashKash54]

OK, I'll leave them as is for now -- just FYI, the client change only involves adding a single parameter into each API function's file where they call executeJs -- not a big deal.

If you want feel free to do it now @MaximusHaximus

[MaximusHaximus]

@DashKash54 All set

• Replaced numResponsesRequired with runOnSingleNode which sends only the request to a single, random node.
• I went ahead and updated the per-network generate LA's to use the new runOnSingleNode too
• I updated the CID repository with the latest CIDs

[DashKash54]

LGTM!