[Rootstock] Use network as derivation path

[ahsan-javaiid]

Proposal:

Use network as derivation path to connect keychain with selected network

Reasons

1:

Rootstock uses different derivation path as compared to ethereum. Tally uses only ethereum derivation path while importing or creating a wallet due to which users have to use ethereum dpath and they might not be able see funds on rootstock accounts.

2:

On the accounts screen user can see their wallets(technically keychain) with list of addresses inside it.

1. For example user is connected to ethereum network and imported a keychain using seed phrase. Now if user switches to other network(rootstock) then that wallet(keychain) still remains there. That can cause problem because user may use the same address on other network. But realistically that keychain belongs to ethereum network.

So i think on the accounts screen there should be some labeling to show that this address is originally from ethereum or rootstock network.
or

Discuss if we need to show only those keychains that belongs to selected network or discuss side effects

Suggested solution

Use network as dpath to connect with keychain so that right addresses could be generated

Type of Change

• Fix/ Enhancement / Integration

Testing information

• Choose a network (Rootstock) then do following
• Create wallet will use dpath from network
• Import wallet will use dpath from network

Accounts screen:
May be we have to show only those keychains that belongs to selected network or add some label to highlight which address or keychain corresponds to which network dpath. Check above diagram.

Checklist

• Git hooks enabled
• No lint errors
• Git commit is signed

Latest build: extension-builds-2673 (as of Wed, 25 Jan 2023 14:19:26 GMT).

[ahsan-javaiid]

cc: @alepc253 @0xDaedalus

[Shadowfiend]

Also since this is user-facing, cc @VladUXUI @mr-michael.

[VladUXUI]

Need to digest a little bit more here, as i feel we need a better fix for this. Some addresses might be for Ethereum+RSK, so having the derivation path source isn't ideal and can cause some confusion down the line.
Overall networks should not filter accounts.

Once user has onboarded with ETH, can't we add the derivation paths for RSK? thus user will see the assets in RSK (And vice-versa)

Also this

dpath and they might not be able see funds on rootstock accounts.

can you say a bit more about "might" here?

[ahsan-javaiid]

Need to digest a little bit more here, as i feel we need a better fix for this. Some addresses might be for Ethereum+RSK, so having the derivation path source isn't ideal and can cause some confusion down the line.
Overall networks should not filter accounts.

• Agree with not to filter the accounts. May be showing some label or tooltip to give an idea that this account was imported or generated via "{{network name}}" will work.
• Agree with statement that some address will be for both ETH + RSK. Need to think about users who are using RSK derivation path. They will have different address than ETH. So in that case if they try to import their account via seed phrase, they will not be able to see their funds because tally uses ETH derivation path by default.
• Similarly if user want to create a wallet by generating new seed phrase and want to use RSK dpath their is no option right now. So ultimately ETH dpath will be used to generate the RSK account. This PR is trying to solve this problem by making dpath a network property. But most welcome to discuss better solutions.

Once user has onboarded with ETH, can't we add the derivation paths for RSK? thus user will see the assets in RSK (And vice-versa)

• Yes thats is the purpose of this PR. Currently no way available in tally.
  Unless you flip the flag HIDE_IMPORT_DERIVATION_PATH to activate the dpath selection UI.
  Even enabling that still it's a problem if a user create new wallet. Because there is no UI there. ETH dpath is hardcode when you create new wallet or import an existing one.

dpath and they might not be able see funds on rootstock accounts.
can you say a bit more about "might" here?

• Consider an existing user with RSK address which was derived using RSK dpath
• User have funds in that account
• User imports this account via seed phrase in tally
• Tally will use ETH dpath which is hardcoded right now.
• Once imported user will not see funds because ETH dpath was used the derive the address.
• I mean generated address will be different if you use ETH dpath.

So in this PR i am trying to solve both "create" and "import" wallet flow with right derivation path based on selected network. Nifty wallet uses this kind of approach.

Feel free to share further thoughts and ideas so that we could come with right approach. That's the goal of this PR.

[VladUXUI]

Thanks for the detailed answer @ahsan-javaiid
I checked this out and it look good.
Interestingly i had a mock-up of the import recovery phrase with the derivation path there, maybe from previous discussions.

I'm also good to create new recovery phrase based on what chain you are ok. But this isn't that user friendly, as it's not intuitive.

I also understand your point about showing labels. I was under the impression that most users will just use the same address they have on Ethereum. But showing label for everything isn't that good, since most of them will say Ethereum, but for the user that might not be the case, not everybody knows that Ethereum address = L2, so it might cause some confusion.
To fix this, let's have a separate default name for RSK imported or created accounts.

I need to do more mock-up work and thinking about how to extend this functionality more, so users have a better understanding and handholding while onboarding and beyond. Adding Select Network as first step of onboarding (Even with Ledger) Account selection after recovery phrase (similar to Ledger), this address has assets in this network and so on...

But that is on me atm and should not block this release.

Happy to ok this with the following changes:

• Add label to imported or created accounts that use RSK dpath as starting point (similar to what you suggested)

• Change derivation path default selection.
  Default one should not be Ledger Live in case of an import, we don't want users to encourage or even educate users that they can import their Ledger recovery phrase.
  So let's have Bip 44 as default (this will need a rename)
  But Ledger Live should be the default one in case of Ledger Onboarding

Let me know if these make sense to you all and if there is anything i'm missing here

[ahsan-javaiid]

Thanks VladT for the ideas and suggestions to push this pull request 👍

Update from my side:

• Added label to created and imported accounts via Rootstock network
• Demo:

• How to test: Create or import wallet having Rootstock network selected, then go to accounts screen, users will see the Rootstock label as suggested by VladT

• I think the second part of VladT suggestion is ultimately pointing towards Ledger: Upgrade Flow to include dropdown #2623

• So Next i will work on Ledger: Upgrade Flow to include dropdown #2623 which i believe cover the ledger part. I will target this on ledger integration pull request.

Tally tech team 👋 Feel free to share thoughts or review this pull request.

[ahsan-javaiid]

Hi Tally team 👋

This PR is ready for review. Other than that ledger flow upgraded to use dropdown:
Issue: #2623
PR: #2577

This covers both points mentioned by VladT

Consider these pull requests:
#2673
#2577

Let me know if it makes sense or anything needs to be changed.

[ahsan-javaiid]

Hi Tally team 👋

Let me know if you get a chance to review this pull request. There are benefits of keeping derivation path inside network:

• Will get rid of derivation path management at multiple places in code and checks
  • Currently derivation path are declared at multiple places in code and used accordingly
• Derivation path will automatically applied based on selected network
• Ledger connection will be very easy to maintain for networks using different derivation paths
• Easy to differentiate accounts screen w.r.t networks using different dpaths

Most welcome if tally team want to share any further thoughts.

[jagodarybacka]

Code-wise I think it is ok ✅

I don't like the UX and the onboarding flow while importing existing wallets.
When user is importing a wallet for the first time there is no way currently to get to the rsk derivation path flow. The user is stuck with the Ethereum default derivation path and you can't "change" the path once the wallet is imported - at least not in any intuitive way. I think we have to allow users to choose the path they want to import during onboarding to make sense of it. Otherwise, it will be like a "hidden" solution.
We may consider allowing importing the same wallet with two derivation paths (I haven't checked if this is possible technically).

That being said - a lot more prototyping and design work is needed to make it perfect so I'm ok with merging it at this stage as this is not breaking anything. What do you think team?

One small improvement that can be done if we will agree to merge it soon - we can add a warning on the onboarding page that the derivation path that is being used is different from the Ethereum default one and to change it the user has to change the network. (cc @VladUXUI)

[jagodarybacka]

Can we make new variables in background/constants... for these derivation paths (so we are not using plain strings)?

[VladUXUI]

Code-wise I think it is ok ✅

I don't like the UX and the onboarding flow while importing existing wallets. When user is importing a wallet for the first time there is no way currently to get to the rsk derivation path flow. The user is stuck with the Ethereum default derivation path and you can't "change" the path once the wallet is imported - at least not in any intuitive way. I think we have to allow users to choose the path they want to import during onboarding to make sense of it. Otherwise, it will be like a "hidden" solution. We may consider allowing importing the same wallet with two derivation paths (I haven't checked if this is possible technically).

That being said - a lot more prototyping and design work is needed to make it perfect so I'm ok with merging it at this stage as this is not breaking anything. What do you think team?

One small improvement that can be done if we will agree to merge it soon - we can add a warning on the onboarding page that the derivation path that is being used is different from the Ethereum default one and to change it the user has to change the network. (cc @VladUXUI)

There actually is @jagodarybacka, what this PR introduces is also a derivation path selector. Need to flip off HIDE_IMPORT_DERIVATION_PATH.
We usually use Support/Show/Enable + turn on, so maybe this is why you didn't see it

[jagodarybacka]

@VladUXUI I remember that we have this hidden dropdown for derivation paths 😉 Right now this PR is not flipping the flag but I think it would benefit from turning it on. Right now if we will merge it the derivation path dropdown will still be hidden but the behaviour of making the selected network-derivation path link will go live.

So my point is - we can flip the derivation path selector flag or we can inform our users during the onboarding process that the wallet import will use the derivation path based on the current network so this is not as opaque as it is now

[VladUXUI]

Ohhh, in that case let's flip Derivation Path on. The default derivation path when adding a second account can still be defaulted to current network.
But adding derivation Path would indeed help solve some of the issues with onboarding

[jagodarybacka]

As the derivation path selector is not working out of the box we can unblock this and improve it later by fixing the selector #2809

[0xDaedalus]

Sorry for the delay here - LGTM!