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.

Sign up for GitHub

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

Jump to bottom

Use separate keyrings and correct root paths for filecoin mainnet and non-mainnet accounts #23241

Closed
spylogsster opened this issue Jun 3, 2022 · 4 comments · Fixed by brave/brave-core#13895
Closed

Use separate keyrings and correct root paths for filecoin mainnet and non-mainnet accounts #23241

spylogsster opened this issue Jun 3, 2022 · 4 comments · Fixed by brave/brave-core#13895
Assignees
@cypt4
Labels
feature/web3/wallet/core feature/web3/wallet/filecoin feature/web3/wallet Integrating Ethereum+ wallet support OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Yes release-notes/include
Milestone
1.43.x - Release

Comments

@spylogsster
Copy link

https://bravesoftware.slack.com/archives/C023VS4HJ6Q/p1654279051831389
@yrliou yrliou changed the title Generate different private keys for FIL accounts with same index in different networks Use the correct root path for filecoin testnet and localhost accounts Jun 3, 2022
@yrliou
Copy link
Member

yrliou commented Jun 3, 2022 •
edited
Loading

We should

  1. revise GetKeyringIdForCoin in keyring_service.cc to use separate keyring (different keyring ID) for filecoin mainnet and non-mainnet accounts. Can use GetCurrentChainId from brave_wallet_utils.cc for the current network info.
  2. revise GetRootPath in keyring_service.cc to return correct and different root path for filecoin mainnet and non-mainnet keyrings.

cc @darkdh

@yrliou yrliou changed the title Use the correct root path for filecoin testnet and localhost accounts Use separate keyring and correct root path for filecoin testnet and localhost accounts Jun 3, 2022
@yrliou yrliou changed the title Use separate keyring and correct root path for filecoin testnet and localhost accounts Use separate keyrings and correct root paths for filecoin testnet and localhost accounts Jun 3, 2022
@yrliou yrliou changed the title Use separate keyrings and correct root paths for filecoin testnet and localhost accounts Use separate keyrings and correct root paths for filecoin mainnet and non-mainnet accounts Jun 3, 2022
@darkdh
Copy link
Member

darkdh commented Jun 3, 2022 •
edited
Loading

we also need to revert most of the codes from brave/brave-core#13402 because it was storing wrong account meta in prefs like this
https:/brave/brave-core/pull/13402/files#diff-3e48700569942cee9d32a5ec77c480f38e04bb5fb20cdf16c52ff2afded6f96eR47-R70
same applies to brave/brave-core#13523 for imported and hardware accounts
Those belong to kFilecoinTestKeyringId(filecoin-test) which is m/44'/1'/0'/index

@darkdh darkdh mentioned this issue Jun 3, 2022
Merged
25 tasks
@yrliou yrliou added the priority/P2 A bad problem. We might uplift this to the next planned release. label Jun 3, 2022
@spylogsster spylogsster mentioned this issue Jun 4, 2022
Merged
7 tasks
@yrliou yrliou added this to the 1.40.x - Beta milestone Jun 6, 2022
cypt4 added a commit to brave/brave-core that referenced this issue Jun 9, 2022
@cypt4
Use separate keyrings for filecoin
2940b6c 
Resolves brave/brave-browser#23241

1: Use different keyrings for filecoin testnet & mainnet
2: Use different root path
3: Extend interface to allow network customization from the ui
cypt4 added a commit to brave/brave-core that referenced this issue Jun 9, 2022
@cypt4
Use separate keyrings for filecoin
7690244 
Resolves brave/brave-browser#23241

1: Use different keyrings for filecoin testnet & mainnet
2: Use different root path
3: Extend interface to allow network customization from the ui
@yrliou yrliou mentioned this issue Jun 9, 2022
Closed
@yrliou yrliou added priority/P3 The next thing for us to work on. It'll ride the trains. and removed release/blocking priority/P2 A bad problem. We might uplift this to the next planned release. labels Jun 9, 2022
@yrliou yrliou removed this from the 1.40.x - Beta milestone Jun 9, 2022
@yrliou
Copy link
Member

yrliou commented Jun 9, 2022

Remove release/blocking since we are going to disable testnets instead first #23367.

@yrliou yrliou added priority/P2 A bad problem. We might uplift this to the next planned release. and removed priority/P3 The next thing for us to work on. It'll ride the trains. labels Jun 10, 2022
cypt4 added a commit to brave/brave-core that referenced this issue Jun 21, 2022
@cypt4
Use separate keyrings for filecoin testnet and mainnet
0c4a82c 
Resolves brave/brave-browser#23241
@cypt4 cypt4 mentioned this issue Jun 22, 2022
Merged
25 tasks
cypt4 added a commit to brave/brave-core that referenced this issue Jun 23, 2022
@cypt4
Add filecoin testnet support
5f8d7c5 
Resolves brave/brave-browser#23368
Resolves brave/brave-browser#23241
Add separate keyring id for filecoin.
Add ui tweaks to support new keyring.
cypt4 added a commit to brave/brave-core that referenced this issue Jul 1, 2022
@cypt4
Add filecoin testnet support
a7260aa 
Resolves brave/brave-browser#23368
Resolves brave/brave-browser#23241
Add separate keyring id for filecoin.
Add ui tweaks to support new keyring.
cypt4 added a commit to brave/brave-core that referenced this issue Jul 5, 2022
@cypt4
Add filecoin testnet support
aa7e0e8 
Resolves brave/brave-browser#23368
Resolves brave/brave-browser#23241
Add separate keyring id for filecoin.
Add ui tweaks to support new keyring.
cypt4 added a commit to brave/brave-core that referenced this issue Jul 6, 2022
@cypt4
Review fix for support filecoin testnet
9ec8862 
Resolves brave/brave-browser#23241
cypt4 added a commit to brave/brave-core that referenced this issue Jul 7, 2022
@cypt4
Review fix for support filecoin testnet
4f03999 
Resolves brave/brave-browser#23241
@brave-builds brave-builds added this to the 1.43.x - Nightly milestone Jul 12, 2022
@srirambv srirambv added the QA/Yes label Aug 3, 2022
@srirambv
Copy link
Contributor

Verification passed on

Brave 1.43.81 Chromium: 104.0.5112.102 (Official Build) (64-bit)
Revision 8e5396254975ef939f2ef7d0bd334e48a052b536-refs/branch-heads/5112@{#1478}
OS Linux
  • Verified steps from brave/brave-core#13895
  • Verified Filecoin mainnet accounts have a different naming convention starting with f
  • Verified Filecoin testnet accounts have a different naming convention starting with t
  • Verified when no Filecoin account is created, selecting Filecoin (Mainnet/Testnet) prompts to create an account on the selected network
  • Verified able to create both Mainnet and Testnet accounts and are listed one below the other irrespective of how its created
  • Verified transaction works on Mainnet
  • Encountered #24824, #24946, #24947 & #24949

Verification passed on

Brave 1.43.81 Chromium: 104.0.5112.102 (Official Build) (64-bit)
Revision 8e5396254975ef939f2ef7d0bd334e48a052b536-refs/branch-heads/5112@{#1478}
OS Windows 11 Version 21H2 (Build 22000.795)
  • Verified steps from brave/brave-core#13895
  • Verified Filecoin mainnet accounts have a different naming convention starting with f
  • Verified Filecoin testnet accounts have a different naming convention starting with t
  • Verified when no Filecoin account is created, selecting Filecoin (Mainnet/Testnet) prompts to create an account on the selected network
  • Verified able to create both Mainnet and Testnet accounts and are listed one below the other irrespective of how its created
  • Verified transaction works on Mainnet
  • Encountered #24824, #24946, #24947 & #24949

Verification passed on

Brave 1.43.81 Chromium: 104.0.5112.102 (Official Build) (arm64)
Revision 8e5396254975ef939f2ef7d0bd334e48a052b536-refs/branch-heads/5112@{#1478}
OS macOS Version 12.4 (Build 21F79)
  • Verified steps from brave/brave-core#13895
  • Verified Filecoin mainnet accounts have a different naming convention starting with f
  • Verified Filecoin testnet accounts have a different naming convention starting with t
  • Verified when no Filecoin account is created, selecting Filecoin (Mainnet/Testnet) prompts to create an account on the selected network
  • Verified able to create both Mainnet and Testnet accounts and are listed one below the other irrespective of how its created
  • Verified transaction works on Mainnet
  • Encountered #24824, #24946, #24947 & #24949

@LaurenWags LaurenWags mentioned this issue Aug 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cypt4 cypt4

Labels
feature/web3/wallet/core feature/web3/wallet/filecoin feature/web3/wallet Integrating Ethereum+ wallet support OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Yes release-notes/include
Projects
Web3
Archived in project
Milestone
1.43.x - Release
Development

Successfully merging a pull request may close this issue.

Add filecoin testnet support brave/brave-core
7 participants
@bbondy @spylogsster @yrliou @darkdh @srirambv @brave-builds @cypt4