-
Notifications
You must be signed in to change notification settings - Fork 960
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
signAndSendTransaction + signAndSendAllTransactions API #889
base: sign-and-send-all-transactions
Are you sure you want to change the base?
signAndSendTransaction + signAndSendAllTransactions API #889
Conversation
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@solana/wallet-standard-features@1.1.0, npm/@solana/wallet-standard-util@1.1.0, npm/@solana/wallet-standard-wallet-adapter-react@1.1.0 |
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.
Some of the changes are good, but I think we have to pay careful attention to the failure cases of the code, which will be common and varied.
packages/core/base/src/adapter.ts
Outdated
} | ||
|
||
/** Mode for signing and sending transactions. */ | ||
export type SolanaSignAndSendTransactionMode = 'parallel' | 'serial'; |
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 would prefer to re-export this from the @solana/wallet-standard-features
package.
packages/core/base/src/adapter.ts
Outdated
@@ -61,7 +60,7 @@ export interface WalletAdapterProps<Name extends string = string> { | |||
transactions: TransactionOrVersionedTransaction<this['supportedTransactionVersions']>[], | |||
connection: Connection, | |||
options?: SignAndSendTransactionOptions | |||
): Promise<(TransactionSignature | SignAndSendAllTransactionsError)[]>; | |||
): Promise<(TransactionSignature | undefined)[]>; |
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.
Returning undefined
is generally not ideal because the code may early return or simply return implicitly. Returning null
is better because it's explicit.
Unfortunately returning either undefined
or null
isn't great, because it gives the caller no information about the failure.
What is an app supposed to do with this result? I don't think we can kick the can on having an error of some kind.
packages/core/base/src/adapter.ts
Outdated
code: result.reason.code, | ||
message: result.reason.message, | ||
}; | ||
return result.status === 'fulfilled' ? result.value : undefined; |
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.
Same here.
); | ||
sendOptions.preflightCommitment = sendOptions.preflightCommitment || connection.commitment; | ||
|
||
const { signatures } = await wallet.signAndSendAllTransactions(transactions, sendOptions); |
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 don't think we can call this without feature detection because there's no guarantee of what version of the wallet the user has that supports this method. If it's older, it'll just null reference with nothing for the app to do.
We should consider having/calling a super
method for this that will do signAllTransactions
and send them after, in the same way we have sendTransaction
in the base class if only signTransaction
is supported.
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 suppose we don't even need to add an implementation for signAndSendAll in individual wallet adapters (other than the unsafe burner, where we won't have to do feature detection)?
continued from #841