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

Add versioned transaction methods to adapter interface #558

Merged
merged 39 commits into from
Sep 15, 2022

Conversation

jstarry
Copy link
Contributor

@jstarry jstarry commented Sep 12, 2022

Changes:

  • Add supportedTransactionVersions property to the adapter interface so that wallets can declare whether they support versioned transactions and list the versions they support.
  • Add generic typing for signTransaction and signAllTransactions which detects supported transaction versions for each adapter and adjusts parameter and return types appropriately.
  • Add generic typing for sendTransaction which detects supported transaction versions for each adapter and adjusts parameter types appropriately

@jstarry jstarry requested a review from jordaaash September 12, 2022 19:16
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.

Approach looks good, but with the Wallet Standard approaching imminently, I don't want to add to the public API if it can be avoided. Let me know what you think!

packages/core/base/src/signer.ts Outdated Show resolved Hide resolved
packages/core/base/src/signer.ts Outdated Show resolved Hide resolved
packages/core/base/src/adapter.ts Outdated Show resolved Hide resolved
packages/core/react/src/WalletProvider.tsx Outdated Show resolved Hide resolved
packages/wallets/blocto/src/adapter.ts Outdated Show resolved Hide resolved
packages/core/base/src/adapter.ts Outdated Show resolved Hide resolved
Comment on lines +8 to +13
signTransaction<T extends TransactionOrVersionedTransaction<this['supportedTransactionVersions']>>(
transaction: T
): Promise<T>;
signAllTransactions<T extends TransactionOrVersionedTransaction<this['supportedTransactionVersions']>>(
transactions: T[]
): Promise<T[]>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think these types are very reasonable now. By inferring SupportedTransactionVersions from the actual implementer, and making the methods return the type they were called with, existing code that expects signTransaction(transaction: Transaction): Promise<Transaction> will just work.

@@ -240,10 +247,10 @@ export const WalletProvider: FC<WalletProviderProps> = ({
);

// Sign a transaction if the wallet supports it
const signTransaction = useMemo(
const signTransaction: SignerWalletAdapterProps['signTransaction'] | undefined = useMemo(
Copy link
Collaborator

@jordaaash jordaaash Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is still not perfect in the sense that a dev will see (transaction: Transaction | VersionedTransaction) as the parameter type, so they have to know to check the adapter for supportedTransactionVersions. But it is functional and nonbreaking, because it will always be expected to return whichever type it was called with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems totally fine to me

@@ -185,13 +186,13 @@ export class AlphaWalletAdapter extends BaseMessageSignerWalletAdapter {
}
}

async signTransaction(transaction: Transaction): Promise<Transaction> {
async signTransaction<T extends Transaction>(transaction: T): Promise<T> {
Copy link
Collaborator

@jordaaash jordaaash Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know that transaction will only ever be Transaction now because supportedTransactionVersions is null, but we still have to implement it this way because the interface declares it like this. All this means is that if a subtype of Transaction was passed to signTransaction, the wallet should return the same subtype. In practice, this doesn't matter because there are no subclasses of Transaction in web3.js to worry about.

try {
const wallet = this._wallet;
if (!wallet) throw new WalletNotConnectedError();

try {
return (await wallet.signTransaction(transaction)) || transaction;
return ((await wallet.signTransaction(transaction)) as T) || transaction;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we have to perform the one ugly bit, where we cast Transaction as T even though we know it will be valid.

@jstarry
Copy link
Contributor Author

jstarry commented Sep 15, 2022

The last round of changes look solid to me. I just built the example app with some test code that called signTransaction from useWallet and API type resolution works great. Your call but I say ship it now and we can iterate quickly on if there are any minor issues that pop up

@jordaaash
Copy link
Collaborator

Sounds good, I'll test a bit and merge tomorrow.

@jordaaash jordaaash merged commit b109484 into anza-xyz:master Sep 15, 2022
@jstarry jstarry deleted the versioned-tx branch September 15, 2022 18:40
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