Skip to content

Overhaul #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: v2-old
Choose a base branch
from
Open

Overhaul #94

wants to merge 8 commits into from

Conversation

madschristensen99
Copy link

Pre-Merge Standard Guide Checklist

  • User story/motivation for the guide has been identified
  • User story/motivation is addressed by the guide
  • Prerequisite knowledge is noted at the beginning of the guide, with links to where to learn more if the user does not possess this knowledge
  • Example code has contains all code and components required for a basic user to run it without implicit knowledge of other code/tools that need to be incorporated
  • Example code runs without bugs
  • No pseudocode
  • Full written explanations of all steps required to use a given feature (alongside code)
  • Warnings for when code is only relevant to a given testnet/function/version/etc.
  • Documentation titles and categorization are clear for new users
  • Guide and example code has been reviewed, followed, and successfully run by a peer who has not already worked on the guide/code and is taking on the persona of a target user for the user story/motivation
  • Guide has been reviewed for correct spelling and grammar
  • Project Lead (or another knowledgeable party) for the feature has reviewed the guide/code and checked for accuracy/completeness
  • Final review and sign-off from Product Manager

@madschristensen99 madschristensen99 marked this pull request as ready for review April 14, 2025 18:55
Comment on lines +26 to +51
// Define the Lit Action code for conditional signing
const litActionCode = `
async () => {
try {
// test an access control condition
const testResult = await Lit.Actions.checkConditions({
conditions,
authSig,
chain,
});

if (!testResult) {
LitActions.setResponse({ response: "address does not have 1 or more Wei on Ethereum Mainnet" });
return;
}

const sigShare = await LitActions.signEcdsa({
toSign: dataToSign,
publicKey,
sigName: "sig",
});
} catch (error) {
LitActions.setResponse({ response: error.message });
}
};
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Can we always define the Lit Action code in a file called src/litAction.ts
  2. Can we always define Lit Actions using the pattern:
const _litActionCode = async () => {
    // Lit Action code here...
};

export const litActionCode = `(${_litActionCode.toString()})();`;

This gives us syntax highlighting for the Lit Action code

Comment on lines +11 to +18
// Utility function to get environment variables
const getEnv = (name: string): string => {
const value = process.env[name];
if (!value) {
console.warn(`Environment variable ${name} is not set`);
}
return value || "";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

We should always define util methods in a src/utils.ts file. Our goal with the main code example file (this file) is to be as lean as possible, focusing only on the code that makes this code example unique. We don't want to use to have to filter out boilerplate code to find the code they'll want to copy/paste into their project

Copy link
Contributor

Choose a reason for hiding this comment

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

All code example files should be named src/index.ts for consistency across all the code examples. Basically every code example is going to at least have these three files: src/index.ts, src/utils.ts, and src/litAction.ts with the latter two being dependent on whether the code example actually needs them

Comment on lines +53 to +59
// Result interface for our function
interface ConditionalSigningResult {
success: boolean;
signatureResult?: Record<string, any>;
response?: string;
error?: string;
}
Copy link
Contributor

@spacesailor24 spacesailor24 Apr 17, 2025

Choose a reason for hiding this comment

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

We shouldn't return a custom type from the example functions, we should just return the raw Lit SDK response and then handle the parsing inside the test. We don't want to clutter the code example with code that's not technically required - it can become confusing for the user copy/pasting the example

@spacesailor24 spacesailor24 self-requested a review April 18, 2025 03:42
});

if (!testResult) {
LitActions.setResponse({ response: "address does not have 1 or more Wei on Ethereum Mainnet" });
Copy link
Contributor

Choose a reason for hiding this comment

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

on Ethereum Mainnet

This should use the chain variable since it's configurable

sigName: "sig",
});
} catch (error) {
LitActions.setResponse({ response: error.message });
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of error.message, for all the code examples can you use error.message ? error.message : JSON.stringify(error) - it's what we're doing for the Vincent tools and should just be a pattern we establish for our LA code

Comment on lines +77 to +78
litNetwork: LIT_NETWORK.DatilTest,
debug: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

For all code examples, can we make these ENVs? It makes it easier to test across the different Lit network when we want to debug something happening on a specific Lit network

Comment on lines +87 to +88
network: LIT_NETWORK.DatilTest,
debug: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

ENVs for these as well

Comment on lines +94 to +105
let pkpPublicKey = LIT_PKP_PUBLIC_KEY;
if (!pkpPublicKey) {
console.log("🔄 PKP wasn't provided, minting a new one...");
const pkpInfo = (await litContracts.pkpNftContractUtils.write.mint()).pkp;
pkpPublicKey = pkpInfo.publicKey;
console.log("✅ PKP successfully minted");
console.log(`ℹ️ PKP token ID: ${pkpInfo.tokenId}`);
console.log(`ℹ️ PKP public key: ${pkpPublicKey}`);
console.log(`ℹ️ PKP ETH address: ${pkpInfo.ethAddress}`);
} else {
console.log(`ℹ️ Using provided PKP: ${pkpPublicKey}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is the type of functionality we want to abstract into a util function. It's going to be used by many code examples and isn't directly relevant to the code example which is covering how to conditional signing with a PKP. Really the code to highlight for this code example is in the Lit Action, we just need a PKP public key to send along with the Lit Action request

Comment on lines +107 to +133
// Handle Capacity Credit
let capacityTokenId = LIT_CAPACITY_CREDIT_TOKEN_ID;
if (!capacityTokenId) {
console.log("🔄 No Capacity Credit provided, minting a new one...");
capacityTokenId = (
await litContracts.mintCapacityCreditsNFT({
requestsPerKilosecond: 10,
daysUntilUTCMidnightExpiration: 1,
})
).capacityTokenIdStr;
console.log(`✅ Minted new Capacity Credit with ID: ${capacityTokenId}`);
} else {
console.log(
`ℹ️ Using provided Capacity Credit with ID: ${capacityTokenId}`
);
}

// Create capacity delegation auth signature
console.log("🔄 Creating capacityDelegationAuthSig...");
const { capacityDelegationAuthSig } =
await litNodeClient.createCapacityDelegationAuthSig({
dAppOwnerWallet: ethersWallet,
capacityTokenId,
delegateeAddresses: [ethersWallet.address],
uses: "1",
});
console.log("✅ Capacity Delegation Auth Sig created");
Copy link
Contributor

Choose a reason for hiding this comment

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

These can also be abstracted into util functions

Comment on lines +163 to +182
// Create auth signature for the action
const authSig = await (async () => {
const toSign = await createSiweMessage({
uri: "http://localhost",
expiration: new Date(Date.now() + 1000 * 60 * 60 * 24).toISOString(), // 24 hours
walletAddress: await ethersWallet.getAddress(),
nonce: await litNodeClient.getLatestBlockhash(),
resources: [
{
resource: new LitActionResource("*"),
ability: LIT_ABILITY.LitActionExecution,
},
],
litNodeClient: litNodeClient,
});
return await generateAuthSig({
signer: ethersWallet,
toSign,
});
})();
Copy link
Contributor

Choose a reason for hiding this comment

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

Another util methods that takes the resources as an array of Lit Resources (type is importable from our types package or auth package)

Comment on lines +219 to +225
return {
success: litActionResult.success,
signatureResult: litActionResult.signatures || {},
response: typeof litActionResult.response === 'string'
? litActionResult.response
: JSON.stringify(litActionResult.response)
};
Copy link
Contributor

Choose a reason for hiding this comment

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

So we should just return litActionResult here, or just return litNodeClient.executeJs(...

console.error("Error in conditional signing:", error);
return {
success: false,
error: error?.message || String(error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also do error.message ? error.message : JSON.stringify(error) here

Comment on lines +135 to +161
// Get session signatures
const sessionSigs = await litNodeClient.getSessionSigs({
chain: CHAIN_TO_CHECK_CONDITION_ON,
capabilityAuthSigs: [capacityDelegationAuthSig],
expiration: new Date(Date.now() + 1000 * 60 * 60 * 24).toISOString(), // 24 hours
resourceAbilityRequests: [
{
resource: new LitActionResource("*"),
ability: LIT_ABILITY.LitActionExecution,
},
],
authNeededCallback: async ({ resourceAbilityRequests, expiration, uri }) => {
const toSign = await createSiweMessage({
uri: uri || "",
expiration: expiration || "",
resources: resourceAbilityRequests || [],
walletAddress: await ethersWallet.getAddress(),
nonce: await litNodeClient.getLatestBlockhash(),
litNodeClient: litNodeClient,
});

return await generateAuthSig({
signer: ethersWallet,
toSign,
});
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you something similar to console.log("✅ Capacity Delegation Auth Sig created"); after every major thing that happens in the code example? This help us find where to look at what went wrong while debugging - also just a nice status update when the user runs the code example

Copy link
Contributor

Choose a reason for hiding this comment

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

Apply the same comments from exampleLitActions/src/conditionalSigning.ts to this file

Comment on lines +40 to +51
// Define type for a basic access control condition compatible with Lit Protocol
type LitAccessControlCondition = {
contractAddress: string;
standardContractType: string;
chain: string;
method: string;
parameters: string[];
returnValueTest: {
comparator: string;
value: string;
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure this is already a defined type from the Lit SDK types package, or one of our packages

// Message to encrypt
const messageText = 'Hello world';

console.log("Encrypting message:", messageText);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace these types of console.logs with the style: console.log("✅ Capacity Delegation Auth Sig created");. So use the emojis:

  • ✅ After something was done successfully, basically a status update that a step was successful
  • 🔄 Use before a request that's likely to take some time to resolve e.g. sending a tx on-chain, minting a PKP, executing a LA
  • ℹ️ For important info worth calling out e.g. using a provided PKP instead of minting a new one, or minting PKP info

This should be our standard for these code examples, and each example should use this pattern

Comment on lines +168 to +174
return {
response: {
response: decryptedMessage || messageText, // Fallback to original text if response is empty
originalResponse: executeResponse // Keep the original response for debugging
},
success: true
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Just return litNodeClient.executeJs(...

Comment on lines +164 to +166
// Disconnect from the Lit Node network
console.log("Disconnecting from Lit Node network...");
await litNodeClient.disconnect();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should happen within a finally clause like in exampleLitActions/src/conditionalSigning.ts

Comment on lines +175 to +182
} catch (error) {
console.error("Error in encryptDecrypt:", error);
return {
response: null,
success: false,
error
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should catch the error since we're not really doing any error handling - this would also automatically fail the test instead of us having to check response.success

Copy link
Contributor

Choose a reason for hiding this comment

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

Apply the same comments from exampleLitActions/src/conditionalSigning.ts to this file

Comment on lines +107 to +108
publicKey:
"0x02e5896d70c1bc4b4844458748fe0f936c7919d7968341e391fb6d82c258192e64",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be hardcoded, we should either use a PKP given to us as an ENV, or we should mint a new PKP like we do in exampleLitActions/src/conditionalSigning.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

Update test to handle raw litNodeClient.executeJs response to determine if the code executed successfully or not

Comment on lines +10 to +13
const originalMessage = 'Hello world';

try {
const result = await encryptDecrypt();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should pass the data to encrypt as a function parameter instead of hardcoding it

Comment on lines +12 to +18
before(function() {
// Skip tests if required environment variables are missing
if (!process.env.ETHEREUM_PRIVATE_KEY) {
console.warn('⚠️ Tests require ETHEREUM_PRIVATE_KEY environment variable');
this.skip();
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't skip the tests an error should be thrown, and the getEnv util functions should catch/handle this

* Execute conditional signing with Lit Protocol
* @returns Result of the conditional signing operation
*/
export const conditionalSigning = async (): Promise<ConditionalSigningResult> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Each code example should just be named runExample, this help us standardize the tests a little

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants