Add plural sign transaction hooks #1105
Conversation
Export plural versions of useSignTransaction and useSignAndSendTransaction. Closes anza-xyz#1104
|
steveluscher
left a comment
There was a problem hiding this comment.
Before we move forward, lets think about whether wallets are ready to handle multi-signing workflows from a UI perspective. Right now I don't know of a wallet that has a sensible UI/workflow for multi-transaction signing sessions. #1104 (comment)
|
Talked to @mcintyre94 about this. I didn't know that some apps were already using the API in this way. I'll review this today. |
|
OK, maybe today. |
|
OK, maybe today, but no guarantees. |
| `View transactions: https://explorer.solana.com/tx/${firstSignature}?cluster=devnet and https://explorer.solana.com/tx/${secondSignature}?cluster=devnet`, | ||
| ); | ||
| } catch (e) { | ||
| console.error('Failed to send transactions', e); |
There was a problem hiding this comment.
Note for future readers: this is a lie. The underlying API does not let you know which transaction failed to send; some of them might, in fact, have landed.
| throw new WalletStandardError(WALLET_STANDARD_ERROR__FEATURES__WALLET_ACCOUNT_CHAIN_UNSUPPORTED, { | ||
| address: uiWalletAccount.address, | ||
| chain, | ||
| featureName: SolanaSignAndSendTransaction, |
There was a problem hiding this comment.
Oh dear. Nice find!
This needs to be a separate PR with an associated pnpm changeset note.
| if (inputs.length === 0) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
These need to be in a separate PR with its associated ‘does not call the wallet's underlying ... implementation’ test.
steveluscher
left a comment
There was a problem hiding this comment.
Thanks for this work, and sorry it's taken me so long.
I'm really struggling with this API. I understand the need for it now, but I think we should end up with one API and deprecate the old one. I think this looks like:
- Mark the singular versions with
@deprecated Use {plural version} - Export the plural versions
- Rewrite all of the tests and examples to use the plural versions instead
- Release 5.{y+1}.0
- Delete the singular implementation
- Release 6.0.0
Would you be OK with this @mcintyre94?
|
I can see the argument for just exporting the plural version, since it covers both use cases and mirrors the wallet-standard feature. It also mirrors the signers APIs which are all multi-transaction. That said, I think the single transaction use case is going to be used overwhelmingly more common, and it is easier if you don't need to use a plural API and unpack the result from an array. The cost of providing the single API is very small, since it's just a wrapper around the plural one in the current implementation. Most importantly though, given the limitations with error handling for multiple transactions, there is a semantic difference (in practice) between sending one transaction and sending multiple. If you're dealing with one then you can understand its success/error status, if you send multiple then an error means some might have landed. Apps that do send multiple will need to treat these cases as meaningfully different. So I think it might be worth keeping the single transaction versions, with their more predictable behaviour, for the typical case of an app sending only one transaction. And then we have a clear place to document the issues with multiple transactions on the plural API. |
Export plural versions of useSignTransaction and useSignAndSendTransaction.
Problem
Wallet-standard allows requesting a wallet to sign multiple transactions. But we only export a hook capable of signing one, eg useSignTransaction.
The actual implementation of this uses a hook that can accept multiple transactions.
Summary of Changes
Closes #1104