feat: Upgrade Ledger libraries and activate Clear Signing #2280
Conversation
|
Thanks for the fix! I'm still getting error results with this PR. |
Yes, the PR has been merged for the new Ethereum app version, but I don't think we have released it yet, I'll make sure to ping you here once it's done so you can test it again before merging anything 👍 EDIT: Just checked with the team, release is expected to be in roughly a month, as we're shipping a lot of new improvements on the EIP712 clear signing with it. I'll revive this PR as soon as it's ready, thanks for your patience ! 🙏 |
|
Let me know when this PR is ready for merge and release to production! |
|
Hi @vvvvvv1vvvvvv ! |
|
@lambertkevin are there any updates on that, or expected release date? |
Release for the new nano app is expected to be in about a month, after that we'll be able to merge this PR 👍 |
vvvvvv1vvvvvv
added
hardware wallet
external contributor
dependency changed
|
@lambertkevin any update on this? |
|
@vvvvvv1vvvvvv @heisenberg-2077 looks like it is finally released https://github.com/LedgerHQ/app-ethereum/blob/master/CHANGELOG.md |
|
I tested this PR after updating my ledger. The first run of signEIP712Message is ok, but after that it throws an exception Ledger NanoX V1.0 |
|
@heisenberg-2077 looks like there is one more update available (1.11.1) which addresses the issue you have described. |
|
I've got ETH v1.11.1 with Rabby 0.92.84. While trying to send coins on ERC20 and other networks, nothing changed. |
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
|
@vvvvvv1vvvvvv Is there anything that still prevents merging this? BTW, will this PR affect Rabby mobile? Would love to see clear sign on mobile as well. |
Answer from Ledger side |
|
@lambertkevin any updates on this? @heisenberg-2077 @vvvvvv1vvvvvv maybe merge it as is? I'm running a custom build of this branch, it works pretty well. When ledger will finally finish their minor tweaks, they can make a new PR. |
what exactly is working well? is the fork public on Github? |
|
We still waiting for update from Ledger team :( |
|
@0x398 We're still finding some bugs here and there and we're trying to address them as fast as possible. We have a release a new release scheduled this month, and hopefully this one should be good for an update of this PR 👍 |
Hi Rabby team !
According to the initiative of Ledger to activate clear signing on all possible Wallets with implementations of our stack, this PR should reactive everything related to:
Issue raised by @heisenberg-2077 here has been fixed (sorry for the long delay 🙏) with this PR: LedgerHQ/app-ethereum#559.
The next version of the Ethereum app will have this fix, and I'll ping you as soon as it's in prod 👍
Feel free to tell me if I need to add some tests somewhere 👌
Thanks !