Skip to content

Conversation

@MaximusHaximus
Copy link
Contributor

@MaximusHaximus MaximusHaximus commented Oct 12, 2024

Description

NOTE: Because this PR includes moving code around and modifying it, it is likely easiest to review it commit-by-commit.

Our wrapped-keys-lit-actions package currently only exports IIFE-wrapped, bundled code as strings.
In order to allow composition of the logic these LIT actions provides into other LIT Actions (for example, to embed generated wrapped keys in session sig generation), we need to export the LIT action functions themselves.
This PR refactors the wrapped-keys-lit-actions package to facilitate this functionality in a non-breaking way for existing consumers of the package.

Note that it wasn't quite as simple as 'unwrap the IIFE and export the function' because the existing LIT actions called Lit.Actions.setResponse() -- and if they were to be composed into any other LIT actions, this would be undesirable behaviour.

To handle this situation, I extracted all usage of Lit.Actions.setResponse into a litActionHandler function, and modified the LIT action functions to always return -- which they need to if they are to be executed by another LIT action as a Lit.Action.runOnce target.

I think this structure for code is a good best-practice pattern to generally follow when writing LA...

  • Module file with IIFE has /* global xxxxx */ markup, and uses handleLitAction() to handle setResponse calls from the LIT action code it calls
  • Actual LIT action code in separate file, exports a function that takes explicit arguments, this code typically would never call Lit.Actions.setResponse -- it always returns or throws -- like normal JS ;)
    - If child logic inside of a 'raw' LA function stack needs to do the equivalent of an 'early' setResponse(); return , e.g. in order to break out if somethin'g using decryptToSingleNode() and this node isn't that one, then we throw a specific type of error -- an AbortError, defined in this PR, that the litActionHandler() function knows should be caught, and return immediately without calling setResponse See getDecryptedKeyToSingleNode() to see this in action.

This pattern will encourage developers to encapsulate all of their LA coding like a 'standard' app, and leave calling setResponse to the root IIFE that uses litActionHandler. In short, calling setReponse() then return is an anti-pattern that requires LA code is completely flat; as soon as you want to nest any code in sub-functions, it breaks down.

I think we should ship this root handler function and the AbortError type somewhere the js-sdk, so devs don't have to write the boilerplate themselves and we show them the happy path to keeping code that is destined for LIT actions looking similar to code that they would write anywhere else.

Type of change

  • New feature (non-breaking change which adds functionality)
  • 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 wrapped-keys local-tests with code compiled using the refactored actions

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

"types": [],
"allowJs": true,
"checkJs": true
"checkJs": false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this because typechecking on the .js files was failing. We should really convert this entire package to TS ASAP.

…layout to make file purposes explicit from their location in the package
…ngleNode() to throw `AbortError` and perform removal of salt before returning a value to callers
…ptedSolanaPrivateKey() to leverage litActionHandler()
…onWithEncryptedSolanaKey() to leverage litActionHandler() and to be aware of new getDecryptedKeyToSingleNode() behaviour
…thEncryptedSolanaKey() to leverage litActionHandler() and to be aware of new getDecryptedKeyToSingleNode() behaviour
…ptedEthereumPrivateKey() to leverage litActionHandler()
…thEncryptedEthereumKey() to leverage litActionHandler() and to be aware of new getDecryptedKeyToSingleNode() behaviour

- Also removed unused imports in signMessageWithEncryptedSolanaKey
…onWithEncryptedEthereumKey() to leverage litActionHandler() and to be aware of new getDecryptedKeyToSingleNode() behaviour
…Key() to leverage litActionHandler() and to be aware of new getDecryptedKeyToSingleNode() behaviour
…EncryptedKeys() to leverage litActionHandler()
…tions from `wrapped-keys-lit-actions` package to allow composition into other actions
…ot escaped using JSON.stringify() in `litActionHandler()`
@MaximusHaximus MaximusHaximus force-pushed the LIT-3959-raw-wrapped-keys-litaction-functions branch from 1c643e0 to 141f0a0 Compare October 14, 2024 00:20
Copy link
Collaborator

@Ansonhkg Ansonhkg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like writing Lit Action just like regular code! Let's have a follow up PR to support Typescript & Type definitions for Lit.Actions & Lit.Auth

@Ansonhkg
Copy link
Collaborator

I like writing Lit Action just like regular code! Let's have a follow up PR to support Typescript & Type definitions for Lit.Actions & Lit.Auth

#699

Copy link
Contributor

@FedericoAmura FedericoAmura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really like this new approach. Once we have the Lit namespaces mocked all this becomes testeable

Comment on lines +53 to +60
'./src/lib/self-executing-actions/solana/signTransactionWithEncryptedSolanaKey.js',
'./src/lib/self-executing-actions/solana/signMessageWithEncryptedSolanaKey.js',
'./src/lib/self-executing-actions/solana/generateEncryptedSolanaPrivateKey.js',
'./src/lib/self-executing-actions/ethereum/signTransactionWithEncryptedEthereumKey.js',
'./src/lib/self-executing-actions/ethereum/signMessageWithEncryptedEthereumKey.js',
'./src/lib/self-executing-actions/ethereum/generateEncryptedEthereumPrivateKey.js',
'./src/lib/self-executing-actions/common/exportPrivateKey.js',
'./src/lib/self-executing-actions/common/batchGenerateEncryptedKeys.js',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "self-executing-actions" makes me think of a cron of that they don't have to be triggered

Comment on lines +1 to +9
export class AbortError extends Error {
name = 'AbortError';
}

export const rethrowIfAbortError = (err) => {
if (err instanceof AbortError) {
throw err;
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be moved to errors.ts in constants in v7. As followup

Might be a long shot, because nodes are in the middle, but we can think of shared errors between SDK and LAs

@Ansonhkg
Copy link
Collaborator

nit: i think we should re-name wrapped-keys-lit-actions to just lit-actions. wrapped-keys might not be the only thing we dogfood.

@Ansonhkg Ansonhkg merged commit c2cfb89 into master Oct 18, 2024
4 checks passed
@Ansonhkg Ansonhkg deleted the LIT-3959-raw-wrapped-keys-litaction-functions branch October 18, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants