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.

Sign up for GitHub

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

Jump to bottom

Wallet vue adapter #363

Merged
merged 18 commits into from
Sep 4, 2024
Merged

Wallet vue adapter #363

merged 18 commits into from
Sep 4, 2024

Conversation

ildarH
Copy link
Contributor

@ildarH ildarH commented Jul 12, 2024

A new wallet adapter for Vue 3 that can be easily integrated with both Vue 3 and Nuxt 3 applications.

This PR adds wallet adapter and example for Nuxt 3

@ildarH ildarH requested a review from a team as a code owner July 12, 2024 15:43
0xmaayan
0xmaayan reviewed Aug 12, 2024
View reviewed changes
Copy link
Collaborator

@0xmaayan 0xmaayan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for this amazing contribution! I tried the demo vue dapp locally, but could run it only when I removed mSafe wallet from the wallets list - any idea why?

@0xmaayan
Copy link
Collaborator

Thank you for this amazing contribution! I tried the demo vue dapp locally, but could run it only when I removed mSafe wallet from the wallets list - any idea why?

Ok, it looks like it is because @msafe/aptos-wallet-adapter is not part of the dapp dependencies array. Actually non of the wallet packages are in the dependencies array - why?

@ildarH
Copy link
Contributor Author

ildarH commented Aug 15, 2024

Thank you for this amazing contribution! I tried the demo vue dapp locally, but could run it only when I removed mSafe wallet from the wallets list - any idea why?

Ok, it looks like it is because @msafe/aptos-wallet-adapter is not part of the dapp dependencies array. Actually non of the wallet packages are in the dependencies array - why?

Hey! I've updated. Sorry for the misunderstanding. Can you check please?

@ildarH ildarH requested a review from 0xmaayan August 15, 2024 12:05
@ildarH ildarH force-pushed the wallet-vue-adapter branch from 65e3479 to 5e5a0d6 Compare August 15, 2024 13:40
@0xmaayan
Copy link
Collaborator

Overall LGTM. Thank you for this contribution!

Some notes:

  1. We are missing the transaction hash in the link after successfully submitting a transaction (attached screenshot)
  2. Is there a reason you didn't include Antd and MUI wallet selectors in the demo app?
  3. When the wallet network is set to Mainnet we should show a warnning message as we have on the nextjs demo app
  4. Given this is a community contribution, and to provide the best support, we should lock the @aptos-labs/wallet-adapter-core version to the latest version this Provider supports. This is to ensure we dont break anything in this package when we do core package upgrades
image

@ildarH
Update reviewer
96d8d48 
* fix transaction hash in nuxt example
* add mainnet warning alert
* lock wallet-adapter-core version
@ildarH
Copy link
Contributor Author

ildarH commented Aug 26, 2024

Thank you for your feedback! I've added fixes.
Regarding the second point, I can include some of the libraries, such as Antd (it has vue version for sure) a bit later in a separate pull request.

@ildarH @0xmaayan
Update packages/wallet-adapter-vue/package.json
91ff2a0 
Co-authored-by: Maayan <maayan@aptoslabs.com>
0xmaayan
0xmaayan approved these changes Aug 27, 2024
View reviewed changes
Copy link
Collaborator

@0xmaayan 0xmaayan left a comment

Choose a reason for hiding this comment

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

LGTM!

@0xmaayan
Copy link
Collaborator

@ildarH build is failing, could you run pnpm install on the adapter root folder, and then make sure build succeed with pnpm turbo run build?
https://github.com/aptos-labs/aptos-wallet-adapter/actions/runs/10563236480/job/29324042334?pr=363

@0xmaayan 0xmaayan merged commit 1cbe660 into aptos-labs:main Sep 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xmaayan 0xmaayan 0xmaayan approved these changes

@kaw2k kaw2k Awaiting requested review from kaw2k kaw2k is a code owner

@hardsetting hardsetting Awaiting requested review from hardsetting hardsetting is a code owner

@kent-white kent-white Awaiting requested review from kent-white kent-white is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ildarH @0xmaayan