RSK `ledger` integration #2577

ahsan-javaiid · 2022-11-08T15:28:17Z

Description

This PR adds ledger integration for RSK
Currently (main branch) is using hardcoded derivation path here in ledger service.
When ledger service connect with RSK app on ledger with eth dpath then RSK app will simply reject it.
So mainly this PR is making derivation path dynamic in ledger service and on UI as well to make it work for RSK

Demo

Type of Change

Fix/ Enhancement / Integration

Testing information

Activate SUPPORT_RSK env flag
Connect with ledger
Activate RSK app on ledger
Good to go
Transaction signing is working

Testing Environment

SUPPORT_RSK=true

Checklist

Git hooks enabled
No lint errors
Git commit is signed

Latest build: extension-builds-2577 (as of Mon, 20 Mar 2023 10:35:42 GMT).

ahsan-javaiid · 2022-11-08T15:32:32Z

cc: @0xDaedalus @alepc253

mhluongo

From @zondax/ledger-icp's docs...

Please:

Do not use in production

Do not use a Ledger device with funds for development purposes.

Have a separate and marked device that is used ONLY for development and testing

We should aim to do this without additional outside dependencies, especially not experimental dependencies.

mhluongo · 2022-11-09T02:45:06Z

ui/pages/Ledger/LedgerImportAccounts.tsx

@@ -167,7 +168,10 @@ function LedgerAccountList({
                    {balance === null && <div className="balance_loading" />}
                    {balance !== null && (
                      <div className="balance">
-                        {balance} {selectedNetwork.baseAsset.symbol}
+                        {balance}{" "}
+                        {appInfo?.appName === "RSK"


I don't think this is the approach we want here, otherwise we'd do the same thing for Polygon.

Instead, we should make sure that the selected network and Ledger app match. If they don't, we should ask the user if they'd like to switch to the RSK network... or tell them to switch Ledger apps (to either Ethereum or Polygon, based on the selected network).

This edge case probably needs some design thinking @VladUXUI

Can i get a little bit more context here?
Is this part of the onboarding or something else? asking because in onboarding we don't currently have option for users to change networks (although it's on the way).

Just for context - Polygon app on Ledger is really bad (and seems to be just a fork/extension of Ethereum app), was throwing weird errors and it was just a more stable approach to encourage users to use the Ethereum app for both ETH and Polygon. Maybe Rsk app is more stable.

@jagodarybacka the problem is that you have to use the right app with the right network because the Ledger app only works with the right derivation path (ETH/Polygon and RSK have different dpath)

The problem is that we do not have any UI to select network/derivation path when user clicks on "connect to ledger" button. Tally wallet is assuming ethereum network by default here and attempt the connect with ledger using ethereum derivation path. In case of other networks like RSK connection get rejected by ledger app (RSK in this case) because tally wallet is passing hardcoded derivation path here

I can think of below two approaches to solve this issue:
Approach 1: Auto detect which app is running on device and use dPath w.r.t that app
Like in this PR i am auto detecting the active app on ledger and then passing dPath dynamically according to connected app.

Approach 2: Provide a UI to select network or derivation path before connecting to ledger so that user can select right network according to running app on ledger device.

So when user clicks on "connect to ledger" button, then user will be taken to derivation path selection screen instead of directly connecting to ledger as its doing right now and assuming ethereum derivation path

IMO, second approach is better and i can update the pull request with this approach.

Most welcome in case of any other thought's 👍

Provide a UI to select network or derivation path before connecting to ledger

@VladUXUI I remember you were thinking about something similar before right?

Follow-up question both for @alepc253 and @mr-michael - Are we OK turning on RSK without this PR being merged?

@0xDaedalus yes, we are OK. This feature is important but can be supported in next releases. Launching RSK asap is the main goal.

Ready for review.

ahsan-javaiid · 2022-11-14T12:15:44Z

Tally Team 👋

I have updated the pull request with ui based approach. Now before connecting to ledger user will be able to select dPath w.r.t ledger app.

See Demo video here:

Untitled.mp4

Also resolved previous feedback and not using any dependencies
Ready for review and most welcome for any thoughts 🤔

greg-nagy · 2022-11-15T16:10:51Z

background/constants/networks.ts

@@ -85,7 +85,7 @@ export const TEST_NETWORK_BY_CHAIN_ID = new Set(
  [GOERLI].map((network) => network.chainID)
 )

-export const NETWORK_FOR_LEDGER_SIGNING = [ETHEREUM, POLYGON]
+export const NETWORK_FOR_LEDGER_SIGNING = [ETHEREUM, POLYGON, RSK]


We have renamed RSK > ROOTSTOCK in #2625
and this breaks the build and also the tests.

Sorry for this issue!

Are you ok fixing this, or would you prefer me to do it?

The fix patch is attached.
RSKtoROOTSTOCK.patch.txt

greg-nagy · 2022-11-15T16:11:11Z

background/redux-slices/migrations/to-6.ts

@@ -23,6 +23,7 @@ export default (prevState: Record<string, unknown>): NewState => {
      currentDeviceID: null,
      devices: {},
      usbDeviceCount: 0,
+      derivationPath: null,


Probably not necessary but ❤️ the details

VladUXUI · 2022-11-15T16:46:25Z

This is looking awesome! 👏 😍
We will probably update soon with new onboarding, but i'm happy for this to be shipped as is (ui/ux wise)

ahsan-javaiid · 2022-11-17T16:27:04Z

Hi Tally team 👋

Feel free to share if there are any further thoughts on this PR.

Shadowfiend · 2022-11-17T21:10:45Z

ui/pages/Ledger/LedgerSelectDerivationPath.tsx

+const DERIVATION_PATH_TO_NETWORK: { [key: string]: EVMNetwork } = {
+  "m/44'/137'/0'/0": ROOTSTOCK,
+  "m/44'/60'/0'/0": ETHEREUM,
+  "m/44'/1'/0'/0": ETHEREUM,
+  "m/44'/60'/0'": ETHEREUM,
+}


Feels like the ledger lib should have some way to resolve this somewhere...

Thanks for the suggestion @Shadowfiend. Purpose of this mapping is to get the right network against the derivation path that is returned by derivation path dropdown. Here i am setting the network selected by the user.
Objective here is to set the right network based on derivation path selected by user.

Feels like the ledger lib should have some way to resolve this somewhere...

I looked into tally codebase (ledger related files) to find something as you suggested. But i think such mapping or something related is not there unless there is some other intelligent way of achieving this.

Would be great if you can share some hint 🙏

Feels like the ledger lib should have some way to resolve this somewhere...

@Shadowfiend Resolved this concern

ahsan-javaiid · 2022-12-06T06:29:29Z

Hi Tally team 👋

Updated this pull request and implemented the ledger upgrade flow to include dropdown

Reference issue: #2623

Demo:

Demo video:

ledgerdemo.mov

Feel free to review the pull request and share thought's if anything needs to changed. 🙏

kkosiorowska

I left some comments on the code.

@VladUXUI Could you check the UI part? The solution is a little different from the design

background/constants/networks.ts

background/main.ts

background/redux-slices/ledger.ts

ui/pages/Ledger/LedgerPrepare.tsx

ui/pages/Ledger/LedgerSelectNetwork.tsx

ui/components/LedgerMenu/LedgerMenuListItemChild.tsx

kkosiorowska · 2022-12-08T10:17:49Z

ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx

+const DEFAULT_DERIVATION_PATH = "44'/60'/0'/0/0"
+const ROOTSTOCK_DERIVATION_PATH = "44'/137'/0'/0"


We declare these values in several places. Let's make it const so that if there is a change, it takes place in one place.

Just a suggestion to make it part of network. Let me know if this idea makes sense or should i move it somewhere else?

hmm I think it can be a part of the network but probably we don't want to mix this thing. Let's wait for @0xDaedalus opinion.

We already have the derivation path on the Rootstock const 😄 For the default one I think we shouldn't just add it as a global const and use where needed and as a fallback.

ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx

VladUXUI · 2022-12-12T06:14:51Z

This is looking good!
Will build today and test it out, as i see some small UI clean-up needed.
We since added BSC and Avalanche, so this will probably need to be updated a bit.
The logic is quite simple i hope, if it has a Ledger app, then it need a different select

VladUXUI · 2022-12-13T06:33:05Z

Hey hey, so i just tested out the network selector from a ui pov, and added a few clean-up things. Pasting this as an image as it was easier for me to do.

Also i moved the Figma to a public file so you can access it and look at stuff.
https://www.figma.com/file/EiXpBpNz2HcoeFJftgC5oc/Tally-Public?node-id=1209%3A2200

Do let me know if it isn't working

VladUXUI · 2022-12-14T15:25:12Z

Looking very good on my end! Awesome job @ahsan-javaiid!
Will let somebody from dev team to give final say

kkosiorowska · 2022-12-15T08:29:12Z

It looks great after the design improvements! 😀 But I have a small problem with the AVAX network. I cannot connect by selecting this network. I don't have this issue for RSK or BNB chain.

kkosiorowska

We're really close 🚀 I left a few tiny comments.

Let's still wait for @0xDaedalus final feedback. 😀
I wonder about one more thing. BSC and AVAX have feature flags. In this case, should we add the ability to switch off/on these networks?

background/redux-slices/ledger.ts

background/redux-slices/selectors/ledgerSelectors.ts

ui/components/LedgerMenu/LedgerMenuProtocolList.tsx

ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx

kkosiorowska · 2022-12-15T08:30:20Z

ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx

+const DEFAULT_DERIVATION_PATH = "44'/60'/0'/0/0"
+const ROOTSTOCK_DERIVATION_PATH = "44'/137'/0'/0"


hmm I think it can be a part of the network but probably we don't want to mix this thing. Let's wait for @0xDaedalus opinion.

ahsan-javaiid · 2023-01-11T07:36:44Z

Hi Tally team 👋

Users are eager to use Tally wallet with Roostock <> Ledger. I would be happy to do any further changes to get this pull request considered.

Feel free to share further thoughts on this pull request.

alepc253 · 2023-02-07T14:42:49Z

hey @kkosiorowska @jagodarybacka, any update on this feature? is there anything else we could help with from Rootstock team?

jagodarybacka

Sorry for the delay 😭 Awesome job was done here!

From UI testing:

I think this selector should be hidden for now as it is bugged and is not really changing dpath but is going back to network selector sceen
with tabbed onboarding turned on SUPPORT_TABBED_ONBOARDING=true we should support a similar flow 🤔 but it shouldn't block this PR IMO

A couple of comments, mostly non-blocking, besides hiding bugged dpath selector

ui/_locales/en/messages.json

ui/components/LedgerMenu/EcosystemNetworkIcon.tsx

jagodarybacka · 2023-02-08T12:59:04Z

ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx

+const DEFAULT_DERIVATION_PATH = "44'/60'/0'/0/0"
+const ROOTSTOCK_DERIVATION_PATH = "44'/137'/0'/0"


We already have the derivation path on the Rootstock const 😄 For the default one I think we shouldn't just add it as a global const and use where needed and as a fallback.

background/services/ledger/index.ts

ui/components/LedgerMenu/LedgerMenuProtocolListItem.tsx

fixed, no external dependencies

ahsan-javaiid · 2023-02-09T15:47:00Z

Sorry for the delay 😭 Awesome job was done here!

From UI testing:

I think this selector should be hidden for now as it is bugged and is not really changing dpath but is going back to network selector sceen

with tabbed onboarding turned on SUPPORT_TABBED_ONBOARDING=true we should support a similar flow 🤔 but it shouldn't block this PR IMO

A couple of comments, mostly non-blocking, besides hiding bugged dpath selector

Done!

VladUXUI · 2023-02-10T04:56:26Z

With the addition of network selection at the beginning of the screen. Do we still need dpath selection?
Curious how many users have custom dpath in Ledger

jagodarybacka · 2023-02-10T09:18:14Z

@VladUXUI I would say we do need it for the convenience of more advanced users, it would be a nice improvement here. For seed path import there is no way to choose the network there so we can either do it based on dpath selector or network selector 🤔

@ahsan-javaiid ty for fixes!

@0xDaedalus could you take a look - I think it's good to go and we can take care of tabbed onboarding later

VladUXUI · 2023-02-11T07:41:39Z

Yeah, agreed. But i would rather fix than hide?
It looks like it's fixed

ahsan-javaiid · 2023-02-28T10:11:05Z

Hi team: 👋

Just a reminder to consider this pull request.🎗 Feel free to share if anything blocking on this pull request. 🚧

Hope to get this PR considered soon. 🙏

jagodarybacka · 2023-03-02T11:12:13Z

Hey @ahsan-javaiid I checked it yesterday and overall it looks nice. One thing I noticed we missed is that now we are missing Ledger live dpath when Ethereum is selected while importing Ledger. We will figure that out with @VladUXUI.

We are planning to release tabbed onboarding soon so we think its best to merge this PR after the initial release of tabbed onboarding is done 🔜
So no action is needed on your side right now, we will take care of it 🫶

ahsan-javaiid · 2023-03-22T13:01:58Z

Hi Taho Team: 👀 Eyes on this pull request if it can be prioritised. 👀 🙏

Shadowfiend · 2023-05-19T18:35:17Z

Hey folks, I wanted to poke at what is now #3385 before going deep on review here. Now that 3385 is written up, I'm going to try and give this some deep 👀 so we can get over the finish line 💪

ahsan-javaiid requested review from greg-nagy, 0xDaedalus, Shadowfiend and mhluongo as code owners November 8, 2022 15:28

ahsan-javaiid force-pushed the fix/rsk/ledger branch from f7b2260 to c1316e2 Compare November 8, 2022 15:31

mhluongo previously requested changes Nov 9, 2022

View reviewed changes

0xDaedalus added this to the RSK Support milestone Nov 11, 2022

ahsan-javaiid force-pushed the fix/rsk/ledger branch 2 times, most recently from fc7fabf to f6ccb1a Compare November 14, 2022 12:08

greg-nagy reviewed Nov 15, 2022

View reviewed changes

ahsan-javaiid force-pushed the fix/rsk/ledger branch 4 times, most recently from 81abd08 to d69c094 Compare November 15, 2022 17:02

Shadowfiend reviewed Nov 17, 2022

View reviewed changes

ahsan-javaiid mentioned this pull request Nov 30, 2022

[Rootstock] Use network as derivation path #2673

Merged

4 tasks

ahsan-javaiid force-pushed the fix/rsk/ledger branch from d69c094 to 7c1afd3 Compare December 6, 2022 06:21

ahsan-javaiid mentioned this pull request Dec 6, 2022

Ledger: Upgrade Flow to include dropdown #2623

Open

5 tasks

kkosiorowska mentioned this pull request Dec 7, 2022

Add Native Binance Smart Chain Support to Tally Ho #2569

Closed

12 tasks

jagodarybacka requested a review from kkosiorowska December 7, 2022 16:16

kkosiorowska suggested changes Dec 8, 2022

View reviewed changes

ahsan-javaiid force-pushed the fix/rsk/ledger branch 2 times, most recently from fdd260a to 0cb729d Compare December 8, 2022 13:24

jagodarybacka requested a review from kkosiorowska December 14, 2022 16:21

kkosiorowska reviewed Dec 15, 2022

View reviewed changes

ahsan-javaiid force-pushed the fix/rsk/ledger branch from 2bfe6b2 to df317c6 Compare December 16, 2022 10:44

ahsan-javaiid force-pushed the fix/rsk/ledger branch from df317c6 to 84f5555 Compare January 5, 2023 06:19

jagodarybacka self-assigned this Jan 18, 2023

ahsan-javaiid force-pushed the fix/rsk/ledger branch from 9eaa7e4 to 512dc72 Compare January 20, 2023 11:32

jagodarybacka requested changes Feb 8, 2023

View reviewed changes

jagodarybacka requested a review from kkosiorowska February 8, 2023 13:46

ahsan-javaiid force-pushed the fix/rsk/ledger branch from 5ed8a8f to 051d76e Compare February 9, 2023 15:23

jagodarybacka assigned 0xDaedalus Feb 17, 2023

hyphenized mentioned this pull request Mar 9, 2023

Tabbed Onboarding - Additional accounts #2956

Merged

1 task

ahsan-javaiid force-pushed the fix/rsk/ledger branch from 3609d9c to 7534e0e Compare March 20, 2023 10:21

feat: rsk ledger integration

22f1574

ahsan-javaiid force-pushed the fix/rsk/ledger branch from 7534e0e to 22f1574 Compare March 20, 2023 10:32

mr-michael assigned Shadowfiend and unassigned 0xDaedalus and jagodarybacka Jun 12, 2023

		const DEFAULT_DERIVATION_PATH = "44'/60'/0'/0/0"
		const ROOTSTOCK_DERIVATION_PATH = "44'/137'/0'/0"

RSK ledger integration #2577

Are you sure you want to change the base?

RSK ledger integration #2577

Conversation

ahsan-javaiid commented Nov 8, 2022 • edited by github-actions bot

Description

Demo

Type of Change

Testing information

Testing Environment

Checklist

ahsan-javaiid commented Nov 8, 2022

mhluongo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsan-javaiid commented Nov 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladUXUI commented Nov 15, 2022

ahsan-javaiid commented Nov 17, 2022

Choose a reason for hiding this comment

ahsan-javaiid Nov 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsan-javaiid commented Dec 6, 2022

kkosiorowska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladUXUI commented Dec 12, 2022

VladUXUI commented Dec 13, 2022

VladUXUI commented Dec 14, 2022

kkosiorowska commented Dec 15, 2022

kkosiorowska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsan-javaiid commented Jan 11, 2023

alepc253 commented Feb 7, 2023

jagodarybacka left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsan-javaiid commented Feb 9, 2023

VladUXUI commented Feb 10, 2023

jagodarybacka commented Feb 10, 2023

VladUXUI commented Feb 11, 2023

ahsan-javaiid commented Feb 28, 2023

jagodarybacka commented Mar 2, 2023

ahsan-javaiid commented Mar 22, 2023

Shadowfiend commented May 19, 2023

RSK `ledger` integration #2577

RSK `ledger` integration #2577

ahsan-javaiid commented Nov 8, 2022 •

edited by github-actions bot

ahsan-javaiid Nov 18, 2022 •

edited

jagodarybacka left a comment •

edited