Skip to content
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

Feature: added versioned transaction solana signature #739

Conversation

kouraf
Copy link
Contributor

@kouraf kouraf commented Dec 13, 2024

Added Support for Versioned Transactions in Solana

Summary of Changes

I have introduced a new feature to the LIT SDK and Actions to support versioned transactions for Solana. Specifically:

  • Added a parameter called versionedTransaction to enable handling of versioned transaction signatures for wrapped keys.
  • Updated the LIT Action to utilize this param.
  • if versionedTransaction is false it uses the legacy transaction

Testing

  • I have tested the action thoroughly, and it works great with the new parameter.
  • For the SDK, while the code changes align with the intended functionality, I haven't been able to verify the implementation entirely. Additional testing might be necessary.

Notes

  • I'm unsure if there's a specific coding style or conventions used in the codebase. If there are, I’d be happy to refactor the code to match the style!
  • I'm available to assist further with these changes or address any feedback to ensure the implementation aligns with your expectations.

Looking forward to hearing your thoughts!

@kouraf kouraf requested a review from Ansonhkg as a code owner December 13, 2024 20:36
@CLAassistant
Copy link

CLAassistant commented Dec 13, 2024

CLA assistant check
All committers have signed the CLA.

- made versionedTransaction optional
- updated with latest master
- applied format rules
@FedericoAmura
Copy link
Contributor

FedericoAmura commented Dec 17, 2024

This is awesome. Thanks @kouraf !

I've made some small changes: formatting and base update, but also made versionedTransaction optional so we don't require consumers to specify it (defaulting to falsy and previous behaviour after all) and we can release this as a minor change.
As a followup we can require it set to true with a versioned transaction, or let the action itself decide based on the type of transaction it receives

BTW, if you can share some of the testing you have done that would be awesome

@kouraf
Copy link
Contributor Author

kouraf commented Dec 18, 2024

Hello! Hello! @FedericoAmura
i just pushed an exemple you can see and test here feel free to let me know your thoughts 😄

I'm not entirely sure about allowing the action itself to decide based on the type of transaction it receives. A few transactions are compatible with both "legacy" and "versioned" formats, and attempting to auto-detect them based on their base64 encoding could lead to errors. However, an interesting feature to consider adding later could be multisigning.

@FedericoAmura
Copy link
Contributor

Hello! Hello! @FedericoAmura i just pushed an exemple you can see and test here feel free to let me know your thoughts 😄

I'm not entirely sure about allowing the action itself to decide based on the type of transaction it receives. A few transactions are compatible with both "legacy" and "versioned" formats, and attempting to auto-detect them based on their base64 encoding could lead to errors. However, an interesting feature to consider adding later could be multisigning.

Thanks for the tests! Looking perfect to me

What I was thinking is that they can pass the wrong versionedTransaction. We can enforce some of that with types but not entirely when using JS instead of TS

Nice addition. Thanks a lot for your contribution!

@FedericoAmura FedericoAmura merged commit 9d336f0 into LIT-Protocol:master Dec 23, 2024
1 check passed
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