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

PolkadotJS hardware wallets support #83

Merged
merged 10 commits into from
Nov 21, 2022
Merged

Conversation

akru
Copy link
Contributor

@akru akru commented Jan 17, 2021

Milestone Delivery Checklist

  • The milestone-delivery-template.md has been copied and updated.
  • The invoice form 📝 has been filled out for this milestone.
  • This pull request is done by the same account, which is responsible for the pull request of the accepted application.
  • In the case of acceptance, the payment will be transferred to the initial BTC payment address.
  • The delivery is according to the milestone delivery guidelines.

Link to the application pull request: w3f/Grants-Program#39

@semuelle
Copy link
Member

semuelle commented Feb 2, 2021

Thanks for the delivery, @akru. We will review both milestones as soon as possible.

@SotaWatanabe
Copy link
Contributor

Hello guys. I am very sure that you guys are very busy right now. But would it be possible to let us know the status of this review? Thank you very much in advance.

@semuelle
Copy link
Member

Hi @SotaWatanabe, sorry for the radio silence. It's the very next project on our agenda. Someone will get in touch with you next week.

@semuelle
Copy link
Member

Hi @SotaWatanabe, the links to the documentation are 404ing; I assume they were moved. Could you update the document?

@akru
Copy link
Contributor Author

akru commented Feb 26, 2021

Hello @semuelle, sorry for the inconvenience, links fixed.

@semuelle
Copy link
Member

Hi @akru, thanks for the fix. No worries.

Just FYI, the review is in progress ([1], [2]) but will take a couple more days. Sorry for the inconvenience.

@SotaWatanabe
Copy link
Contributor

Hello @semuelle I hope you are doing well. I am sure that Web3 Foundation is really busy now since the ecosystem is growing very fast. I don't mean to rush you, but could you share the status of this review since the last reply was on Feb 27th?

@semuelle
Copy link
Member

Hi @SotaWatanabe & @akru, sincere apologies for the delay on this.

The good news is, @Lederstrumpf and I just talked about it last Friday and while my last estimate was way off, I expect this to be done very soon.

@SotaWatanabe
Copy link
Contributor

Hello @semuelle Thank you very much for your reply. That is good to know. Feel free to ask any technical questions here if you have any :D

@Lederstrumpf
Copy link

Hi @akru and @SotaWatanabe,

My sincere apologies that this review has been delayed a lot.
I've now gotten hold of a Trezor and am happy to confirm that the signing process works.

I ran into the following two issues though:

  1. When aborting a transaction on the device, the Sign Transaction pane remains pending. Could this return an error instead and abort the process?
  2. The polkadot.js app crashes after some time, which does not occur with master for me, without throwing an error in the shell output. It's happened at least 5 times during my testing so far, but I have not been able to reproduce deterministically, nor had time to debug.

@hoonsubin
Copy link

@Lederstrumpf thank you for the feedback! We'll work on it ASAP and let you know when the issues are fixed.

@mmagician
Copy link
Contributor

@hoonsubin I hope all is fine on your side. Have you had any progress with the milestone recently?

@hoonsubin
Copy link

hoonsubin commented May 5, 2021

@mmagician Sorry for the silence.
Regarding the progress for the custom signature page, most of the issue regarding error handling with MetaMask is fixed. However, error handling with Ledger is still not fixed. There seems to be an issue with either MM itself, or the Ledger API (or Ledger bridge implementation in MM), as error callbacks (e.g., canceling signature requests from the device) on Ledger cannot be captured by the client app for unknown reasons, hence the infinite loading bug when the user cancels the signature from Ledger.
Error messages from MM are correctly captured, so I assume it has to do with rejections from Ledger being dropped by the MM provider. (Relevant line of code: https://github.com/MetaMask/eth-ledger-bridge-keyring/blob/main/index.js#L264)

I am still working on this issue, but due to other tasks I have to prioritize, it may take longer than I expect. I will see what I can do before the end of this month. If I cannot find a simple solution, I'll take a hacky approach and add a signature request timeout function to manually throw an error.

@mmagician
Copy link
Contributor

Thanks for the update and the explanation. That makes sense - from my point of view it's better to take the time and implement a proper, long-term solution. Good luck and keep us posted!

@semuelle
Copy link
Member

semuelle commented Jul 5, 2021

Hi @hoonsubin, any news on this?

@hoonsubin
Copy link

Hi @hoonsubin, any news on this?

@semuelle Sorry for the late response and the continued delay in the delivery. Our team was focusing on the Kusama Parachain auction for the past couple of months. Having said that, our auction has ended successfully now and we will be able to actively work on this task (finally)!
I will keep you posted and aim for delivering the task within this month, or early next month at the latest.

Thank you.

@hoonsubin
Copy link

@semuelle sorry for the long silence. I have noticed that the latest update of MetaMask fails to solve the issue I mentioned above. I have extended my investigation from last week, but this issue is definitely with the MetaMask Ledger iframe provider. So I have opened an issue on their repository hoping for some answers.
MetaMask/metamask-extension#11680

Again, I apologize once again for the delay, but if this problem is solved within MetaMask, the application should automatically be fixed as well. I will stay in touch with MetaMask and Ledger bridge provider project and update my pull request on polkadot-js/apps and send a final review request when I can confirm that it works!

@semuelle
Copy link
Member

Thanks for the update. No rush from our side. Keep us posted.

@alxs
Copy link
Contributor

alxs commented Jul 30, 2021

@hoonsubin thank you for your dedication. If you're done with the Trezor part, we can also evaluate milestone 1 separately and compensate you for it independently of the issues above. Feel free to move its delivery file to a separate pull request and we'll look into it.

@alxs
Copy link
Contributor

alxs commented Oct 19, 2021

@hoonsubin checking in again - any updates?

@hoonsubin
Copy link

@alxs

I'm terribly sorry for the extremely late response. Our team was busy with dApps Staking/Store page and the upcoming Polkadot Parachain Auction with our mainnet, and I must have missed this issue from my notifications...

So in conclusion, the latest MetaMask update and the Ledger RPC bridge still did not address this problem. I noticed that MM will not be able to solve this issue any time soon so I decided to quickly add a wrapper function to the request method (polkadot-js/apps@16c79a5#diff-9064ff163ec77275ea4ae6dbfc6d5def43c41e723493f404cd65e752b3d09426).
I noticed that the polkadot portal has had some major changes that are giving a lot of build errors, but running the app locally with yarn start seems to work. I will fix the build/style/type errors and make the CI pass, but can you first test the functionality and confirm if it works well now?
The error timeout is 30 seconds for now, but this value can be adjusted.

@hoonsubin
Copy link

@hoonsubin I'm back at work. Happy to review the delivery again as soon as it's fixed, but it doesn't look like you made any changes since our last exchange.

As I recall, we could not recreate the issue you were encountering. May I request a second opinion from another reviewer's device?

@alxs
Copy link
Contributor

alxs commented Oct 19, 2022

@hoonsubin we only have one Trezor in the office for testing. Even if it worked with another device, the success rate should be 100% for us to accept the milestone. Did you try replicating my set-up as I mentioned here? If you're not interested in fixing this, we can always cancel the Trezor milestone and settle for Ledger integration.

@hoonsubin
Copy link

@hoonsubin we only have one Trezor in the office for testing. Even if it worked with another device, the success rate should be 100% for us to accept the milestone. Did you try replicating my set-up as I mentioned here? If you're not interested in fixing this, we can always cancel the Trezor milestone and settle for Ledger integration.

Okay. I tried emulating your environment, but still to no avail. Sadly, our team is preparing for some major releases, so I cannot guarantee that we will have enough people to work on this task.
Since I do not wish to delay this milestone any longer, I think it makes sense that we cancel the Trezor milestone and only deliver the Ledger milestone.
I recall that I could cancel the milestone by editing the original proposal. Can you guide me through this process and further action points after we settle on the Ledger delivery?

Thank you

@alxs
Copy link
Contributor

alxs commented Oct 19, 2022

That makes sense, thank you for your understanding. The amendment process is described here, but it boils down to editing the existing proposal and submitting a PR. Once it's been accepted, I'll test the Ledger integration one last time and we should be good to go.

@hoonsubin
Copy link

That makes sense, thank you for your understanding. The amendment process is described here, but it boils down to editing the existing proposal and submitting a PR. Once it's been accepted, I'll test the Ledger integration one last time and we should be good to go.

Opened a PR w3f/Grants-Program#1225

@alxs
Copy link
Contributor

alxs commented Oct 20, 2022

@hoonsubin sorry to say Ledger isn't working for me either. I also tried locally on my machine (macOS) with freshly cloned apps page and collator node, and still get the same error.

@alxs
Copy link
Contributor

alxs commented Nov 7, 2022

@hoonsubin we would appreciate it if you could commit to a timeline on this. If you can't look into it in the coming weeks, could you please submit another amendment with an estimated delivery date?

@gluneau
Copy link
Contributor

gluneau commented Nov 8, 2022

Hello @alxs, I've been testing this custom signature PR and I'm currently getting Invalid Transaction: Transaction has a bad signature, we acknowledged the problem and we are in the process of fixing this.

@gluneau
Copy link
Contributor

gluneau commented Nov 16, 2022

Good day @alxs, i've refactored the polkadot-js apps to take Hoon's implementation of the custom signature. You can see the changes for reference.

I've tested it on ledger and trezor and works great.

I think I've been able to simplify the testing scenario by using the Shiden parachain RPCs.
So when you're ready, you can test your local web app on a live chain.

You'll need to seed your account, to do so after the first signature, copy the address it gives you and send it to me. (Mine is ZECSjh5SZjHW9zGek3TzTnn7vCbxgncDFbGZVKSCi4arEyw for reference) I'll send you SDN to test.

image

To run the test, you can checkout my cs-nov branch and yarn start it.
Also notice there are 18 zeros needed to make a whole number.

Have fun!

@alxs
Copy link
Contributor

alxs commented Nov 17, 2022

@gluneau thanks for looking into this. Sounds good, could you send me some SDN to bMvcmyxGTBKwYvghXqEtCwr9Zpxib44ZPTmduQFiXgDer3G? When you're done, please also update the delivery file to point to the correct implementation of the custom-signatures pallet and apps page.

@niklabh
Copy link
Contributor

niklabh commented Nov 17, 2022

Sent 50 SDN https://shiden.subscan.io/extrinsic/2764904-2

@alxs
Copy link
Contributor

alxs commented Nov 17, 2022

Thanks @niklabh. @gluneau I confirm it is now working with Ledger. Congratulations. Let me know when you've updated the links in the delivery file and I'll take one final look. Also, if you could provide the documentation on a GitHub repository, that would make it easier for us to clone it and save it in our archive (no need to create a separate one, but I assume you will want to move it somewhere more permanent?).

@gluneau
Copy link
Contributor

gluneau commented Nov 18, 2022

Hello @alxs, the delivery document has been updated with new links and a new documentation repo.

Good day!

@alxs
Copy link
Contributor

alxs commented Nov 21, 2022

Thank you @akru @niklabh @gluneau. With this, I'm happy to let you know that your milestone has been accepted! You can find my evaluation notes here. One last thing: could you please also add a (Apache 2.0) LICENSE file to the documentation repository?

Please submit your invoice using the current form and let me know when you're done. Make sure to include the same Kusama address as in your application and the amount denominated in USD(T).

@alxs alxs merged commit e378903 into w3f:master Nov 21, 2022
@github-actions
Copy link

Congratulations on completing the first milestone of this grant! As part of the Grants Program, we want to help grant recipients acknowledge their grants publicly. To that end, we’ve created a badge for projects that successfully deliver their first milestone. Note that it must only be used within the context of the delivered work, so please do not display it on your team or project's homepage unless accompanied by a short description of the grant.

Furthermore, you're now welcome to announce the grant publicly. Please remember to observe the foundation’s guidelines in doing so. In case you haven't done so yet, you may also reach out to [email protected] for feedback on your announcement and cross-promotion.

Thank you for your contribution and good luck with the remaining milestones, if any! As usual, please let us know if you run into any delays by leaving a comment on the application PR, or directly submitting an amendment.

@gluneau
Copy link
Contributor

gluneau commented Nov 21, 2022

Thanks @alxs, the licenses was added to the docs.

@hilarious23
Copy link

Hi, we need to update the address to receive the grant.
I heard the payment would be made with USDT-statemint.
In this case, we would like to use 146iwtFpePG4GqBfNpMTDRgqfwNjj248JpvDKLyFPM1KSsa.
Thank you.

@hoonsubin
Copy link

Hi, we need to update the address to receive the grant. I heard the payment would be made with USDT-statemint. In this case, we would like to use 146iwtFpePG4GqBfNpMTDRgqfwNjj248JpvDKLyFPM1KSsa. Thank you.

I'll open a PR to reflect these changes.

@hoonsubin
Copy link

Thank you @akru @niklabh @gluneau. With this, I'm happy to let you know that your milestone has been accepted! You can find my evaluation notes here. One last thing: could you please also add a (Apache 2.0) LICENSE file to the documentation repository?

Please submit your invoice using the current form and let me know when you're done. Make sure to include the same Kusama address as in your application and the amount denominated in USD(T).

Filled in the form!

@alxs
Copy link
Contributor

alxs commented Dec 19, 2022

Thanks, could you please include the Kusama address above in the invoice and denominate it in USDT?

@hoonsubin
Copy link

Thanks, could you please include the Kusama address above in the invoice and denominate it in USDT?

Updated the address on the grant w3f/Grants-Program#1382. The form will be updated shortly.

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.

10 participants