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 support for SKY Wallet #398

Merged
merged 21 commits into from
Jun 29, 2022
Merged

Add support for SKY Wallet #398

merged 21 commits into from
Jun 29, 2022

Conversation

skywalletadmin
Copy link
Contributor

This PR adds support for SKY Wallet
Currently SKY Wallet works as a DApp Browser Implementation inside the SKY Wallet mobile app. Support for Chrome and Safari coming soon.

@jordaaash
Copy link
Collaborator

Hey, I want to address the lack of activity on your PR. Thank you for your patience!

Here's a followup on the state of wallet-adapter, the issues that individual adapters have in getting integrated with applications, and how we intend to improve things: #384 (comment)

Let's discuss in that thread to keep it in one place.

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.

While the medium-term goal is to move away from requiring adapters for every wallet, I want to respect the time you took to create this adapter, and review it now.

Once a new standard is introduced, let's work together to migrate away from dependency on using an adapter.

packages/starter/example/components/ContextProvider.tsx Outdated Show resolved Hide resolved
packages/wallets/skywallet/package.json Outdated Show resolved Hide resolved
packages/wallets/skywallet/package.json Outdated Show resolved Hide resolved
packages/wallets/skywallet/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/skywallet/src/adapter.ts Outdated Show resolved Hide resolved
@skywalletadmin
Copy link
Contributor Author

Thanks Jordan for the review on this PR. We have resolved all your comments and pushed the new changes. Looking forward to seeing SKY Wallet integrated with this PR. Please let me know if any other changes are required. Thanks!

@skywalletadmin
Copy link
Contributor Author

Hi Jordan,
Thanks for reviewing the PR again.
We've fixed the following issues -

  1. ContextProvider class has been reverted out of the PR.
  2. sendTransaction implementation has been corrected
  3. dependencies have been corrected in package.json

Please let us know if there's any other changes we should make. Appreciate your support!

packages/wallets/skywallet/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/skywallet/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/skywallet/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/skywallet/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/skywallet/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/skywallet/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/wallets/package.json Show resolved Hide resolved
@skywalletadmin
Copy link
Contributor Author

Hi Jordan,
Thanks for your review on this PR. We have addressed and fixed all the review comments.
Please let us know if there are any other changes to this. Much appreciated!

packages/wallets/skywallet/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/walletconnect/lib/cjs/adapter.js Outdated Show resolved Hide resolved
packages/wallets/wallets/src/index.ts Outdated Show resolved Hide resolved
@skywalletadmin
Copy link
Contributor Author

Hi Jordan,
Thanks for your review! We've fixed the issues and made the changes. Please let us know if there's any thing else. Thanks!

@jordaaash jordaaash merged commit 40d6835 into anza-xyz:master Jun 29, 2022
@jordaaash
Copy link
Collaborator

Published:

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

Successfully merging this pull request may close these issues.

2 participants