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 OntoWallet adapter #284

Closed
wants to merge 11 commits into from

Conversation

PieroDaVinci
Copy link
Contributor

No description provided.

@PieroDaVinci
Copy link
Contributor Author

Can you merge code please?

@PieroDaVinci
Copy link
Contributor Author

@jordansexton have any questions about PR ? Please check and commit, Thanks

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.

Looks good, thanks for the changes! I had only one minor question.


interface OntoWallet {
isOnto?: boolean;
signTransaction(transaction: Transaction): Promise<Transaction>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this used? It seems like the interface below is using request with sol_sign.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jordansexton Yes,used in Solana Dapp and App Wallet interaction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @PieroDaVinci, the issue is that signTransaction is declared here, but on line 139 below:

const response = await wallet.request({ method: 'sol_sign', params: [transaction] });

Since only wallet.request seems to be used, I'm wondering if this method declaration is needed.

@PieroDaVinci
Copy link
Contributor Author

resolve the conflict

@PieroDaVinci
Copy link
Contributor Author

@jordansexton Hi guys, ONTO Web3 Wallet is the most popular wallet and have Millions users,our wallet have many users of Solana,so,please check and commit~!Thanks

@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.

@anza-xyz anza-xyz deleted a comment from PieroDaVinci Jun 22, 2022
@anza-xyz anza-xyz deleted a comment from PieroDaVinci Jun 22, 2022
@anza-xyz anza-xyz deleted a comment from PieroDaVinci Jun 22, 2022
@anza-xyz anza-xyz deleted a comment from PieroDaVinci Jun 22, 2022
@anza-xyz anza-xyz deleted a comment from PieroDaVinci Jun 22, 2022
@anza-xyz anza-xyz deleted a comment from PieroDaVinci Jun 22, 2022
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.


interface OntoWallet {
isOnto?: boolean;
signTransaction(transaction: Transaction): Promise<Transaction>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @PieroDaVinci, the issue is that signTransaction is declared here, but on line 139 below:

const response = await wallet.request({ method: 'sol_sign', params: [transaction] });

Since only wallet.request seems to be used, I'm wondering if this method declaration is needed.


export const OntoWalletName = 'ONTO' as WalletName;

export class Coin98WalletAdapter extends BaseSignerWalletAdapter {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was copied from Coin98, please fix and review for any other copied sections.

| [torus](https://github.com/solana-labs/wallet-adapter/tree/master/packages/wallets/torus) | Adapter for [Torus](https://tor.us) | [`@solana/wallet-adapter-torus`](https://npmjs.com/package/@solana/wallet-adapter-torus) |
| [walletconnect](https://github.com/solana-labs/wallet-adapter/tree/master/packages/wallets/walletconnect) \* | Adapter for [WalletConnect](https://walletconnect.org) | [`@solana/wallet-adapter-walletconnect`](https://npmjs.com/package/@solana/wallet-adapter-walletconnect) |
| [onto](https://github.com/solana-labs/wallet-adapter/tree/master/packages/wallets/onto) | Adapter for [onto](https://onto.app) | [`@solana/wallet-adapter-onto`](https://npmjs.com/package/@solana/wallet-adapter-onto) |

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also merge in the latest from the master branch? It looks like the README changes may be out of sync here.

@jordaaash
Copy link
Collaborator

Closing this PR since there's been no activity for over a month. Please let me know if you've made the changes above and I'll reopen!

@jordaaash jordaaash closed this Jul 27, 2022
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.

3 participants