-
Notifications
You must be signed in to change notification settings - Fork 100
chore(solana): upgrade from web3.js to solana kit #327
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
base: main
Are you sure you want to change the base?
Conversation
packages/sdk-provider-solana/src/utils/KeypairWallet.unit.spec.ts
Outdated
Show resolved
Hide resolved
packages/sdk-provider-solana/src/utils/KeypairWallet.int.spec.ts
Outdated
Show resolved
Hide resolved
…rade-v4-to-solana-kit
|
@tomiiide could you please rebase and make the PR only about Solana Kit changes 🙏 It would be hard to review 273 files again 😅 |
| blockHeight = await connection | ||
| .getBlockHeight({ | ||
| commitment: 'confirmed', | ||
| }) | ||
| .send() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't have a block height update here to reduce the number of requests to the RPC. I think it is fine to update it every second for both cases when we send a transaction below. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I separated them to avoid race conditions as we are sharing blockheight between two loops.
The polling loop is updating the blockheight, while the sending loop is reading it.
Yes, we get 2x more rpc calls, but better code quality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does make sense to update once every second when resending, Since the worst case scenario is minimal during a race condition, it just feels dirty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the time we get that race condition, it's probably already an expired transaction anyway. So I understand that it might be a bit cleaner from a code perspective, but I don't see a reason why we need an additional RPC call to get the same block height, so I'd stick with only one call. As a third option, it could be an additional promise for that.
| @@ -0,0 +1,12 @@ | |||
| import { fromVersionedTransaction } from '@solana/compat' | |||
| import { getTransactionCodec, type Transaction } from '@solana/kit' | |||
| import { VersionedTransaction } from '@solana/web3.js' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to avoid having @solana/web3.js installed? What do we use it for?
Moving to Solana Kit was supposed to optimize bundle size 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we can remove it 😁
| "bs58": "^6.0.0" | ||
| "@solana/compat": "^5.0.0", | ||
| "@solana/kit": "^5.0.0", | ||
| "@solana/web3.js": "^1.98.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let's find a way to remove this dependency 🙏😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean just @solana/web3.js right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lifinance/widget#600 (comment)
Do we move the compat logic to the widget , and keep the sdk purely solana kit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use @solana/web3.js as a dependency in v4 for both Widget and SDK, it's an outdated package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should return the adapter like before from the widget, so no versioned txs conversion should happen there. Can we manage this in the SDK without depending on @solana/web3.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we return the adapter like before then, the sdk would still need to convert txns to/from versioned txns as the adapters only handle versioned txn types.
We can switch the adapter types to wallet standard uiWallet type like it's done in the @solana/kit example. but that introduces the problem we had before where we have to manage currentWallet state as it's not done by wallet-standard/react.
There is an official library that does this, and the first version has been released, but we need this PR merged to be able to fully support it.
Therefore my suggestion is to have the transaction compatibility layer live on solana-widget-provider for now, and after framework-kit is mature, we can completely migrate solana wallet management to wallet-standard without issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@effie-ms ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess conversion in solana-widget-provider is fine for now, if there is no workaround yet.
Which Jira task is linked to this PR?
https://lifi.atlassian.net/browse/LF-13181
Why was it implemented this way?
Explain the reasoning behind the implementation. Were there alternative approaches? Why was this solution chosen?
The solana provider specifies a wallet interface to generalize use without depending on a specific wallet / adapter type as before (e.g
SignerWalletAdapterfrom@solana/wallet-adapter-base)All it requires is that the wallet can handle transactions types from
@solana/kitType conversions
Since
signTransactiononly deals with Transaction types from the@solana/kit, if using web3.js, you have to convert to/from transactions and address types. The solana provider exports all utility functions needed for this.An example of how do this is here: lifinance/widget#600
Visual showcase (Screenshots or Videos)
If applicable, attach screenshots, GIFs, or videos to showcase the functionality, UI changes, or bug fixes.
Checklist before requesting a review