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

feat: add WELLDONE extension wallet #477

Merged
merged 19 commits into from Oct 25, 2022
Merged

Conversation

Yoon-Suji
Copy link
Contributor

Description

WELLDONE extension wallet is multi-chain wallet that support NEAR protocol. You can read more information in here.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Type of change.

  • FIX - a PR of this type patches a bug.
  • FEATURE - a PR of this type introduces a new feature.
  • BUILD - a PR of this type introduces build changes.
  • CI - a PR of this type introduces CI changes.
  • DOCS - a PR of this type introduces DOCS improvement.
  • STYLE - a PR of this type introduces style changes.
  • REFACTOR - a PR of this type introduces refactoring.
  • PERFORMANCE - a PR of this type introduces performance changes.
  • TEST - a PR of this type adds more tests.
  • CHORE - a PR introduces other changes than the specified above.

@kujtimprenkuSQA
Copy link
Collaborator

Hey, @Yoon-Suji thank you for submitting this PR.

Could you please add a (docs) Readme.md file to the package like it's done with other packages (wallets) in this repo?

Before reviewing the code I tried to install and test out the functionality of this wallet/extension but for some reason, I am unable to create a new wallet with this extension it always gets stuck in this view:

create-new-wallet

@Yoon-Suji
Copy link
Contributor Author

@kujtimprenkuSQA Hi, I'm sorry for late response😂
I tried it again and it works well. Can you please check the chrome version? And also I added docs in packages.

@kujtimprenkuSQA
Copy link
Collaborator

This is the chrome version I am using on this machine, I think it's the latest version:

Version 106.0.5249.91 (Official Build) (64-bit)

The Welldone wallet is still not working for me, will ask other team members to have a look at it too.

@amirsaran3
Copy link
Collaborator

Hey @Yoon-Suji. I was able to successfully create an account using the WELLDONE extension. I built your branch to test the functionality and I am seeing a couple of bugs:

  1. When refreshing page I get logged out (session is cleared).
  2. signAndSendTransaction and signAndSendTransactions functions are not working for me. I am getting this error which is not letting me to sign transactions:
    image

};
},

async signAndSendTransaction({ signerId, receiverId, actions }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should consider using the signTransactions function in packages\wallet-utils\src\lib\sign-transactions.ts when signing to avoid using these "helper function" like _validateAccessKey, convertTransaction and convertActions. All of the above functions are used in signTransactions. This way we avoid duplicating code.

You will have to provide a signer: Signer object to the signTransactions function. You can check out the nightly package and see how they have done it.

Copy link
Contributor Author

@Yoon-Suji Yoon-Suji Oct 5, 2022

Choose a reason for hiding this comment

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

We implemented a method of signing before sending a transaction on its own inside the wallet. So we use it instead of using the signTransactions function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, I changed the signAndSendTransaction method to using signTransactions function. But the signAndSendTransactions method still uses the dapp:sendTransaction method (the wallet signs and sends transactions) because we do not currently support signing multiple transactions. This code will change after the wallet has been updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, we're going to have a look at.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @Yoon-Suji , I'm unable to create or import wallet as well. Getting stuck on "Create a wallet" screen
image

Tried it on two machines with Chrome Versions 106.0.5249.119 (Official Build) (64-bit) and 106.0.5249.103 (Official Build) (64-bit). The same goes for Brave browser so you might want to investigate.

Small thing I've noticed with password rule text... It says "write more than 8 letters" instead of "write 8 or more characters" if you want to change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @DamirSQA , We are trying to reproduce the exceptional situation where the error occurs.
So I would really appreciate it if you could send me the console log by right-clicking on the Create a Wallet screen and clicking the inspect button.
Thanks for reporting the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're welcome, here is the log

image

Copy link
Contributor Author

@Yoon-Suji Yoon-Suji Oct 21, 2022

Choose a reason for hiding this comment

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

Thanks, @DamirSQA We updated our wallet in chrome web store.
Could you turn off all the other wallet extensions and try again?
If it works well, I would appreciate it if you could tell me the type of extension you are using.
If it still doesn't work, Could you send me the console log one more time, please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing extensions didn't make a difference. On some devices it does work, on the others does not.

@AmmarHumackicSQA
Copy link
Collaborator

Hi @Yoon-Suji , thanks for submitting the PR. As a reminder, the security statement needs to be shown either on the GitHub profile/repo or the website. You can check it here again. Thanks!

@Yoon-Suji
Copy link
Contributor Author

Hi @AmmarHumackicSQA
We added the security statement to our GitHub organization profile! you can check it here

@Yoon-Suji
Copy link
Contributor Author

Hi @amirsaran3
First of all, thanks for letting us know about the bug.
There was an issue with the RPC node we use, which is why the signAndSendTransaction and signAndSendTransactions methods failed. We fixed it and uploaded the updated wallet to the chrome webstore. Could you please delete the current version of WELLDONE extension wallet and re-download it?
And I fixed some of the code by reflecting on your comments. Please check it!

@amirsaran3
Copy link
Collaborator

Hi @amirsaran3 First of all, thanks for letting us know about the bug. There was an issue with the RPC node we use, which is why the signAndSendTransaction and signAndSendTransactions methods failed. We fixed it and uploaded the updated wallet to the chrome webstore. Could you please delete the current version of WELLDONE extension wallet and re-download it? And I fixed some of the code by reflecting on your comments. Please check it!

Hey @Yoon-Suji. I tested your new version and signing works. However wallets should have a function that allows us to just sign the transaction and return the signature. Is it possible to add near:signTransaction and near:signTransactions request methods?

@Yoon-Suji
Copy link
Contributor Author

Yoon-Suji commented Oct 11, 2022

Hi @amirsaran3 First of all, thanks for letting us know about the bug. There was an issue with the RPC node we use, which is why the signAndSendTransaction and signAndSendTransactions methods failed. We fixed it and uploaded the updated wallet to the chrome webstore. Could you please delete the current version of WELLDONE extension wallet and re-download it? And I fixed some of the code by reflecting on your comments. Please check it!

Hey @Yoon-Suji. I tested your new version and signing works. However wallets should have a function that allows us to just sign the transaction and return the signature. Is it possible to add near:signTransaction and near:signTransactions request methods?

Hey @amirsaran3. Welldone wallet has the dapp:sign method, that allows you to sign messages and transactions. I also added a signer:Signer object as you suggested and used it to verifyOwner function, please check!

@amirsaran3
Copy link
Collaborator

@Yoon-Suji After latest changes I get this error when trying to sign message:
image
Also there is no info about the dapp:sign method in your docs (https://docs.welldonestudio.io/wallet/developer-guide/provider-api/method).

@Yoon-Suji
Copy link
Contributor Author

@amirsaran3
This is the issue that the old query was deprecated in view_access_key RPC api during the testnet upgrade process. NEAR replied that it would revert for users who use old query, but we modified the wallet in a way that does not use old query according to NEAR's initial intention. But the testnet is currently down, so if it operates again, the modifications will be reflected after the test.

And also I'll add a description of the dapp:sign method to our docs. Could you leave the same comment on our Github issue? We want the docs github issue activated :)

@Yoon-Suji
Copy link
Contributor Author

Hi @amirsaran3 , we checked again and it seems that all works well with WELLDONE version 0.0.25 (latest version updated in the store) Could you try again? And we updated our docs too!

@Yoon-Suji
Copy link
Contributor Author

hey, we implemented the signTransaction function.

If the signMessage method in Signer takes arguments as a transaction, it signs Transaction, if the method takes arguments as a message, it signs message. So I also changed signAndSendTransaction method to using signTransactions in packages\wallet-utils\src\lib\sign-transactions.ts

But we are not supported signing multiple transactions because currently the Welldone wallet was ready to send multiple transactions, but it is not ready to sign multiple transactions. So the signAndSendTransactions method still using dapp:sendTransaction method which is signing and sending transactions at once in the wallet. If required, we will include multiple transaction signing in the next wallet release.

@DamirSQA DamirSQA merged commit 4315b03 into near:dev Oct 25, 2022
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.

None yet

6 participants