Skip to content

Conversation

@Ansonhkg
Copy link
Collaborator

@Ansonhkg Ansonhkg commented Sep 25, 2024

Description

Fix wrong encoding - it should be base58 instead of base64.

As per @spacesailor24 comment for the following Lit Action:

const transaction = Transaction.from(
  Buffer.from(unsignedTransaction.serializedTransaction, 'base64')
);

transaction.sign(solanaKeyPair);
const signature = transaction.signature.toString('base64');

if (broadcast) {
  const solanaConnection = new Connection(
    clusterApiUrl(unsignedTransaction.chain),
    'confirmed'
  );
  const transactionSignature = await solanaConnection.sendRawTransaction(
    transaction.serialize()
  );

  Lit.Actions.setResponse({ response: transactionSignature });
} else {
  Lit.Actions.setResponse({ response: signature });
}

When broadcast is true, transactionSignature will be base58 encoded, but when false, signature is base64 encoded

@Ansonhkg Ansonhkg added the 🐞 Bug Fix Something isn't working label Sep 25, 2024
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

👍 good catch!

@FedericoAmura
Copy link
Contributor

FedericoAmura commented Sep 25, 2024

This made me realize a few things considering these LA maintenance

  1. Probably we need a way to update those LAs on publish? And a test to validate that the LA in wrapped-keys-lit-actions is the same as the CID in `wrapped-keys``
  2. Updating any LA changes its CID, so this change also needs to update that here with the updated one
  3. We also have this repo serving the LAs unbundled code in an express app. If that is used I think we could update it to serve the bundled version from the SDK, or unbundled from this repo to avoid duplicating work there

cc. @MaximusHaximus

@Ansonhkg
Copy link
Collaborator Author

This made me realize a few things considering these LA maintenance

  1. Probably we need a way to update those LAs on publish? And a test to validate that the LA in wrapped-keys-lit-actions is the same as the CID in `wrapped-keys``
  2. Updating any LA changes its CID, so this change also needs to update that here with the updated one
  3. We also have this repo serving the LAs unbundled code in an express app. If that is used I think we could update it to serve the bundled version from the SDK, or unbundled from this repo to avoid duplicating work there

cc. @MaximusHaximus

re: ... express app. If that is used ...

No, it’s not used - It was the initial idea for bundling the LA code, but we disregarded it because we don’t want to maintain another microservice.

@MaximusHaximus
Copy link
Contributor

MaximusHaximus commented Sep 25, 2024

This made me realize a few things considering these LA maintenance

  1. Probably we need a way to update those LAs on publish? And a test to validate that the LA in wrapped-keys-lit-actions is the same as the CID in `wrapped-keys``
  2. Updating any LA changes its CID, so this change also needs to update that here with the updated one
  3. We also have this repo serving the LAs unbundled code in an express app. If that is used I think we could update it to serve the bundled version from the SDK, or unbundled from this repo to avoid duplicating work there

cc. @MaximusHaximus

Yes - I was thinking the same thing regarding making our publishing workflow more repeatable and consistent.

We currently handle publishing these as an entirely manual process, but now that this is its own package it would be awesome to get the publishing to IPFS codified into the project scripts for this package.

What do you think about adding publishing script to this package, and during build+publish to NPM, we actually compute the CID of these actions after bundling them, and generate the LitActionCidRepository in this package, and upload them to IPFS?

We can then....

  • Expose litActionCidRepository just like we do litActionRepository from this package
  • Import litActionCidRepository from this package inside of wrapped-keys, instead of using the constant that is currently embedded in that package
  • Ensure that updates to the LIT actions are always picked up by the wrapped-keys package as long as the package bumps its dependency version in wrapped-keys to the appropriate NPM version of this package.

With tree shaking, people using the CID repository still wouldn't end up with the action source code in their app, even if it was a dependency from wrapped-keys.

We could even replicate the setLitActionCode() pattern.

  • Even though we internally import the litActionCidRepository from this package as a default behaviour, we could allow it to be mutated using a new config method -- e.g. setLitActionCids() -- which could be called by consumers of the primary package
  • This would allow wrapped-keys consumers to use their own custom actions to handle wrapped-keys operations without being forced to bundle their custom actions source into their app bundle. 🤔

We can do all this without it being a breaking change for existing consumers because the lit action CID repository is an internal detail of wrapped-keys (not public).

Copy link
Contributor

@spacesailor24 spacesailor24 left a comment

Choose a reason for hiding this comment

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

I just found out that ethers includes base58 en/decoding, so we don't have to import bs58

Ansonhkg and others added 2 commits October 1, 2024 13:55
…onWithSolanaEncryptedKey.js

Co-authored-by: Wyatt Barnes <[email protected]>
Signed-off-by: Anson <[email protected]>
…onWithSolanaEncryptedKey.js

Co-authored-by: Wyatt Barnes <[email protected]>
Signed-off-by: Anson <[email protected]>
@Ansonhkg
Copy link
Collaborator Author

Ansonhkg commented Oct 1, 2024

I just found out that ethers includes base58 en/decoding, so we don't have to import bs58

Good catch! Thanks!!

@Ansonhkg Ansonhkg requested a review from spacesailor24 October 1, 2024 12:56
@Ansonhkg Ansonhkg merged commit c12c427 into master Oct 1, 2024
3 checks passed
@Ansonhkg Ansonhkg deleted the feature/lit-3597-js-sdk-bug-wrapped-keys-solana-sign-tx-wrong-signature branch October 1, 2024 13:35
MaximusHaximus added a commit that referenced this pull request Oct 8, 2024
…tion test to expect base58 signature response (#652)
MaximusHaximus added a commit that referenced this pull request Oct 9, 2024
…tion test to expect base58 signature response (#652)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐞 Bug Fix Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants