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

Supporting the new SDK with wallet adapter #193

Merged
merged 15 commits into from
Nov 1, 2023
Merged

Supporting the new SDK with wallet adapter #193

merged 15 commits into from
Nov 1, 2023

Conversation

xbtmatt
Copy link
Contributor

@xbtmatt xbtmatt commented Oct 28, 2023

Description

This is the initial attempt at supporting the new SDK with the wallet adapter.

Namely, there is now a submitTransaction (TBD: new name) function where you can submit BCS inputs and it converts it to a BCS payload that's compatible with the v1 SDK. It then relays this call to the signAndSubmitBCSTransaction function that already exists.

This facilitates signing and submitting a transaction using the v2 SDK without having to wait on wallets to implement any code for it.

The wallet adapter is still extremely limited in what it can do- this is an initial attempt at getting a single transaction to submit.

Building

To use a local ts-sdk version with exported types (such as from the PR mentioned below), you must update the version in @aptos-labs/ts-sdk to link your local dependency, specifically in the

apps/nextjs-example/package.json
and
packages/wallet-adapter-core/package.json

"@aptos-labs/ts-sdk": "^0.0.1",
"@aptos-labs/ts-sdk": "/YOUR/LOCAL/PATH/",

Testing

go to the nextjs-example app, yarn run dev, and try to submit a transaction in the optional section at the bottom of the dapp. It will be for the v2 SDK

It should succeed. This relies on: #139 from the wallet adapter.

apps/nextjs-example/pages/index.tsx Outdated Show resolved Hide resolved
apps/nextjs-example/pages/index.tsx Show resolved Hide resolved
packages/wallet-adapter-core/src/WalletCore.ts Outdated Show resolved Hide resolved
packages/wallet-adapter-core/src/WalletCore.ts Outdated Show resolved Hide resolved
apps/nextjs-example/pages/index.tsx Show resolved Hide resolved
packages/wallet-adapter-core/src/WalletCore.ts Outdated Show resolved Hide resolved
packages/wallet-adapter-core/src/WalletCore.ts Outdated Show resolved Hide resolved
packages/wallet-adapter-core/src/conversion.ts Outdated Show resolved Hide resolved
packages/wallet-adapter-core/src/conversion.ts Outdated Show resolved Hide resolved
@xbtmatt xbtmatt marked this pull request as ready for review October 30, 2023 21:39
Copy link
Contributor

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

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

Did we have anything else to get into the wallet adapter @0xmaayan for a release, otherwise, I suggest we get this in and just start iterating quickly on fixing what we want in the interface.

The sponsored transaction stuff as well as other pieces are quickly showing that they need a unified interface, and a split interface on the backend (as you mentioned in the other PR), and if we are going to deprecate the multiple functions, we'll need to drive towards that.

Thoughts?

@@ -49,7 +50,7 @@ export interface WalletContextState {
): Promise<any>;
signMessage(message: SignMessagePayload): Promise<SignMessageResponse | null>;
signMessageAndVerify(message: SignMessagePayload): Promise<boolean>;

submitTransaction(transaction: InputGenerateTransactionData): Promise<any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay... we need to make this better.

any doesn't cut it anymore. We should at least make it return a hash string or an object with a hash string so it can be extended accordingly.

@0xmaayan
Copy link
Collaborator

Did we have anything else to get into the wallet adapter @0xmaayan for a release, otherwise, I suggest we get this in and just start iterating quickly on fixing what we want in the interface.

The sponsored transaction stuff as well as other pieces are quickly showing that they need a unified interface, and a split interface on the backend (as you mentioned in the other PR), and if we are going to deprecate the multiple functions, we'll need to drive towards that.

Thoughts?

I think we need to move towards v2, it consolidates lots of the weird things we are doing here trying to support existing adapter, sdk v1 and sdk v2.
The new sdk provide fewer and easier interfaces to generate/sign/submit transaction

@gregnazario
Copy link
Contributor

Did we have anything else to get into the wallet adapter @0xmaayan for a release, otherwise, I suggest we get this in and just start iterating quickly on fixing what we want in the interface.
The sponsored transaction stuff as well as other pieces are quickly showing that they need a unified interface, and a split interface on the backend (as you mentioned in the other PR), and if we are going to deprecate the multiple functions, we'll need to drive towards that.
Thoughts?

I think we need to move towards v2, it consolidates lots of the weird things we are doing here trying to support existing adapter, sdk v1 and sdk v2. The new sdk provide fewer and easier interfaces to generate/sign/submit transaction

Let's get this in @xbtmatt, and let's get it rolling on wallet adapter V2 with more functionality.

Copy link
Contributor

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

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

Please make an interface for the output of submitTransaction of {hash: string}

@0xmaayan
Copy link
Collaborator

0xmaayan commented Nov 1, 2023

@xbtmatt please run pnpm run changeset so we can add it to version packages PR

@xbtmatt xbtmatt merged commit 7acfa69 into main Nov 1, 2023
4 checks passed
@xbtmatt xbtmatt deleted the wallet-adapter-v2 branch November 1, 2023 20:14
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.

3 participants