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

signAndSendTransaction + signAndSendAllTransactions API #889

Open
wants to merge 7 commits into
base: sign-and-send-all-transactions
Choose a base branch
from

Conversation

0xproflupin
Copy link
Contributor

continued from #841

Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@solana/[email protected] None +2 129 kB jordansexton
npm/@solana/[email protected] None +6 2.26 MB jordansexton
npm/@solana/[email protected] Transitive: environment, eval, filesystem, network, shell +59 27.7 MB jordansexton

🚮 Removed packages: npm/@solana/[email protected], npm/@solana/[email protected], npm/@solana/[email protected]

View full report↗︎

Copy link
Collaborator

@jordaaash jordaaash left a 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.

}

/** Mode for signing and sending transactions. */
export type SolanaSignAndSendTransactionMode = 'parallel' | 'serial';
Copy link
Collaborator

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.

@@ -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)[]>;
Copy link
Collaborator

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.

code: result.reason.code,
message: result.reason.message,
};
return result.status === 'fulfilled' ? result.value : undefined;
Copy link
Collaborator

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);
Copy link
Collaborator

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.

Copy link
Contributor Author

@0xproflupin 0xproflupin Feb 2, 2024

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)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants