Skip to content

Commit

Permalink
Review fix for support filecoin testnet
Browse files Browse the repository at this point in the history 
Resolves brave/brave-browser#23241
  • Loading branch information
@cypt4
cypt4 committed Jul 6, 2022
1 parent aa7e0e8 commit 9ec8862
Show file tree
Hide file tree
Showing 5 changed files with 143 additions and 40 deletions.
105 changes: 96 additions & 9 deletions browser/brave_wallet/keyring_service_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "base/test/scoped_feature_list.h"
#include "brave/browser/brave_wallet/json_rpc_service_factory.h"
#include "brave/components/brave_wallet/browser/brave_wallet_constants.h"
#include "brave/components/brave_wallet/browser/brave_wallet_utils.h"
#include "brave/components/brave_wallet/browser/fil_transaction.h"
#include "brave/components/brave_wallet/browser/filecoin_keyring.h"
#include "brave/components/brave_wallet/browser/hd_keyring.h"
Expand Down Expand Up @@ -475,6 +476,13 @@ class KeyringServiceUnitTest : public testing::Test {
return success;
}

static void UpdateNameForHardwareAccount(KeyringService* service,
const std::string& address,
const std::string& account_name,
mojom::CoinType coin) {
service->UpdateNameForHardwareAccountSync(address, account_name, coin);
}

static bool AddFilecoinAccount(KeyringService* service,
const std::string& account_name,
const std::string& network) {
Expand All @@ -500,7 +508,8 @@ class KeyringServiceUnitTest : public testing::Test {
imported_accounts[i].import_payload, imported_accounts[i].network);
ASSERT_TRUE(address.has_value());
EXPECT_EQ(address.value(), imported_accounts[i].address);

// ImportFilecoinAccount waits only until account is added,
// But there are still mojo tasks we need to wait
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(service->IsKeyringCreated(keyring_id));
if (i == 0) {
Expand Down Expand Up @@ -1241,7 +1250,8 @@ TEST_F(KeyringServiceUnitTest, LockAndUnlock) {
ASSERT_NE(
service.CreateKeyring(brave_wallet::mojom::kFilecoinKeyringId, "brave"),
nullptr);
ASSERT_TRUE(AddAccount(&service, "FIL Account 1", mojom::CoinType::FIL));
ASSERT_TRUE(
AddFilecoinAccount(&service, "FIL Account 1", mojom::kFilecoinMainnet));
ASSERT_NE(
service.CreateKeyring(brave_wallet::mojom::kSolanaKeyringId, "brave"),
nullptr);
Expand Down Expand Up @@ -1827,11 +1837,10 @@ TEST_F(KeyringServiceUnitTest, GetPrivateKeyForKeyringAccount) {
}

TEST_F(KeyringServiceUnitTest, GetMainKeyringIdForCoin) {
EXPECT_EQ(KeyringService::GetMainKeyringIdForCoin(mojom::CoinType::FIL),
mojom::kFilecoinKeyringId);
EXPECT_EQ(KeyringService::GetMainKeyringIdForCoin(mojom::CoinType::SOL),
EXPECT_FALSE(KeyringService::GetKeyringIdForCoinNonFIL(mojom::CoinType::FIL));
EXPECT_EQ(*KeyringService::GetKeyringIdForCoinNonFIL(mojom::CoinType::SOL),
mojom::kSolanaKeyringId);
EXPECT_EQ(KeyringService::GetMainKeyringIdForCoin(mojom::CoinType::ETH),
EXPECT_EQ(*KeyringService::GetKeyringIdForCoinNonFIL(mojom::CoinType::ETH),
mojom::kDefaultKeyringId);
}

Expand Down Expand Up @@ -2100,7 +2109,10 @@ TEST_F(KeyringServiceUnitTest, HardwareAccounts) {
EXPECT_TRUE(observer.AccountsChangedFired());
observer.Reset();
for (const auto& account : new_accounts) {
auto keyring_id = KeyringService::GetMainKeyringIdForCoin(account->coin);
auto keyring_id =
account->coin == mojom::CoinType::FIL
? brave_wallet::GetFilecoinKeyringId(*account->network)
: *KeyringService::GetKeyringIdForCoinNonFIL(account->coin);
auto path = keyring_id + ".hardware." + account->device_id +
".account_metas." + account->address;
ASSERT_TRUE(
Expand Down Expand Up @@ -2597,6 +2609,79 @@ TEST_F(KeyringServiceUnitTest, GetSetAutoLockMinutes) {
EXPECT_EQ(kAutoLockMinutesMax, GetAutoLockMinutes(&service));
}

TEST_F(KeyringServiceUnitTest, UpdateNameForHardwareAccountSync) {
KeyringService service(json_rpc_service(), GetPrefs());

ASSERT_TRUE(CreateWallet(&service, "brave"));

std::vector<mojom::HardwareWalletAccountPtr> new_accounts;
new_accounts.push_back(mojom::HardwareWalletAccount::New(
"0x111", "m/44'/60'/1'/0/0", "name 1", "Ledger", "device1",
mojom::CoinType::ETH, absl::nullopt));
new_accounts.push_back(mojom::HardwareWalletAccount::New(
"0x264", "m/44'/461'/0'/0/0", "name 2", "Ledger", "device1",
mojom::CoinType::FIL, mojom::kFilecoinMainnet));
new_accounts.push_back(mojom::HardwareWalletAccount::New(
"0xEA0", "m/44'/60'/2'/0/0", "name 3", "Ledger", "device2",
mojom::CoinType::ETH, absl::nullopt));
new_accounts.push_back(mojom::HardwareWalletAccount::New(
"0xFIL", "m/44'/461'/2'/0/0", "filecoin 1", "Ledger", "device2",
mojom::CoinType::FIL, mojom::kFilecoinMainnet));
new_accounts.push_back(mojom::HardwareWalletAccount::New(
"0x222", "m/44'/60'/3'/0/0", "name 4", "Ledger", "device1",
mojom::CoinType::ETH, absl::nullopt));
new_accounts.push_back(mojom::HardwareWalletAccount::New(
"0xFILTEST", "m/44'/1'/2'/0/0", "filecoin testnet 1", "Ledger", "device2",
mojom::CoinType::FIL, mojom::kFilecoinTestnet));

service.AddHardwareAccounts(std::move(new_accounts));

UpdateNameForHardwareAccount(&service, "0x111", "name 1 changed",
mojom::CoinType::ETH);
UpdateNameForHardwareAccount(&service, "0xFIL", "filecoin 1 changed",
mojom::CoinType::FIL);
UpdateNameForHardwareAccount(&service, "0xFILTEST",
"filecoin testnet 1 changed",
mojom::CoinType::FIL);

{
base::RunLoop run_loop;
service.GetKeyringInfo(
mojom::kDefaultKeyringId,
base::BindLambdaForTesting([&](mojom::KeyringInfoPtr keyring_info) {
EXPECT_FALSE(keyring_info->account_infos[1]->address.empty());
EXPECT_EQ(keyring_info->account_infos[1]->name, "name 1 changed");
run_loop.Quit();
}));
run_loop.Run();
}

{
base::RunLoop run_loop;
service.GetKeyringInfo(
mojom::kFilecoinKeyringId,
base::BindLambdaForTesting([&](mojom::KeyringInfoPtr keyring_info) {
EXPECT_FALSE(keyring_info->account_infos[1]->address.empty());
EXPECT_EQ(keyring_info->account_infos[1]->name, "filecoin 1 changed");
run_loop.Quit();
}));
run_loop.Run();
}

{
base::RunLoop run_loop;
service.GetKeyringInfo(
mojom::kFilecoinTestnetKeyringId,
base::BindLambdaForTesting([&](mojom::KeyringInfoPtr keyring_info) {
EXPECT_FALSE(keyring_info->account_infos[0]->address.empty());
EXPECT_EQ(keyring_info->account_infos[0]->name,
"filecoin testnet 1 changed");
run_loop.Quit();
}));
run_loop.Run();
}
}

TEST_F(KeyringServiceUnitTest, SetDefaultKeyringHardwareAccountName) {
KeyringService service(json_rpc_service(), GetPrefs());

Expand Down Expand Up @@ -2780,9 +2865,11 @@ TEST_F(KeyringServiceUnitTest, AddFilecoinAccounts) {
{
ASSERT_TRUE(CreateWallet(&service, "brave"));
#if BUILDFLAG(IS_ANDROID)
ASSERT_FALSE(AddAccount(&service, "FIL account1", mojom::CoinType::FIL));
ASSERT_FALSE(
AddFilecoinAccount(&service, "FIL account1", mojom::kFilecoinTestnet));
#else
ASSERT_TRUE(AddAccount(&service, "FIL account1", mojom::CoinType::FIL));
ASSERT_TRUE(
AddFilecoinAccount(&service, "FIL account1", mojom::kFilecoinTestnet));
#endif
service.Reset();
}
Expand Down
66 changes: 37 additions & 29 deletions components/brave_wallet/browser/keyring_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,10 @@ void KeyringService::Bind(
}

// static
std::string KeyringService::GetMainKeyringIdForCoin(mojom::CoinType coin) {
absl::optional<std::string> KeyringService::GetKeyringIdForCoinNonFIL(
mojom::CoinType coin) {
if (coin == mojom::CoinType::FIL) {
return mojom::kFilecoinKeyringId;
return absl::nullopt;
} else if (coin == mojom::CoinType::SOL) {
return mojom::kSolanaKeyringId;
}
Expand Down Expand Up @@ -843,18 +844,13 @@ void KeyringService::AddFilecoinAccount(const std::string& account_name,
void KeyringService::AddAccount(const std::string& account_name,
mojom::CoinType coin,
AddAccountCallback callback) {
std::string keyring_id = GetMainKeyringIdForCoin(coin);
if (IsFilecoinKeyringId(keyring_id)) {
if (!IsFilecoinEnabled()) {
std::move(callback).Run(false);
return;
}
if (!LazilyCreateKeyring(keyring_id)) {
VLOG(1) << "Unable to create Filecoin keyring";
std::move(callback).Run(false);
return;
}
} else if (keyring_id == mojom::kSolanaKeyringId) {
auto keyring_id = GetKeyringIdForCoinNonFIL(coin);
if (!keyring_id) {
NOTREACHED() << "AddFilecoinAccount must be used";
std::move(callback).Run(false);
return;
}
if (*keyring_id == mojom::kSolanaKeyringId) {
if (!IsSolanaEnabled()) {
std::move(callback).Run(false);
return;
Expand All @@ -865,9 +861,9 @@ void KeyringService::AddAccount(const std::string& account_name,
return;
}
}
auto* keyring = GetHDKeyringById(keyring_id);
auto* keyring = GetHDKeyringById(*keyring_id);
if (keyring) {
AddAccountForKeyring(keyring_id, account_name);
AddAccountForKeyring(*keyring_id, account_name);
}

NotifyAccountsChanged();
Expand Down Expand Up @@ -957,20 +953,28 @@ void KeyringService::ImportAccount(const std::string& account_name,
const std::string& private_key,
mojom::CoinType coin,
ImportAccountCallback callback) {
std::string keyring_id = GetMainKeyringIdForCoin(coin);
if (account_name.empty() || private_key.empty() || !encryptors_[keyring_id]) {
auto keyring_id = GetKeyringIdForCoinNonFIL(coin);

if (!keyring_id) {
NOTREACHED() << "ImportFilecoinAccount must be used";
std::move(callback).Run(false, "");
return;
}

if (account_name.empty() || private_key.empty() ||
!encryptors_[*keyring_id]) {
std::move(callback).Run(false, "");
return;
}

std::vector<uint8_t> private_key_bytes;
if (keyring_id == mojom::kDefaultKeyringId) {
if (*keyring_id == mojom::kDefaultKeyringId) {
if (!base::HexStringToBytes(private_key, &private_key_bytes)) {
std::move(callback).Run(false, "");
return;
}
} else if (keyring_id == mojom::kSolanaKeyringId) {
if (!LazilyCreateKeyring(keyring_id)) {
} else if (*keyring_id == mojom::kSolanaKeyringId) {
if (!LazilyCreateKeyring(*keyring_id)) {
VLOG(1) << "Unable to create Solana keyring";
std::move(callback).Run(false, "");
return;
Expand All @@ -990,7 +994,7 @@ void KeyringService::ImportAccount(const std::string& account_name,
}

auto address =
ImportAccountForKeyring(keyring_id, account_name, private_key_bytes);
ImportAccountForKeyring(*keyring_id, account_name, private_key_bytes);
if (!address) {
std::move(callback).Run(false, "");
return;
Expand Down Expand Up @@ -1097,22 +1101,22 @@ std::string KeyringService::GetImportedKeyringId(
const std::string& address) const {
return coin_type == mojom::CoinType::FIL
? FindImportedFilecoinKeyringId(address).value_or(kKeyringNotFound)
: GetMainKeyringIdForCoin(coin_type);
: GetKeyringIdForCoinNonFIL(coin_type).value_or(kKeyringNotFound);
}

std::string KeyringService::GetHardwareKeyringId(
mojom::CoinType coin_type,
const std::string& address) const {
return coin_type == mojom::CoinType::FIL
? FindHardwareFilecoinKeyringId(address).value_or(kKeyringNotFound)
: GetMainKeyringIdForCoin(coin_type);
: GetKeyringIdForCoinNonFIL(coin_type).value_or(kKeyringNotFound);
}

std::string KeyringService::GetKeyringId(mojom::CoinType coin_type,
const std::string& address) const {
return coin_type == mojom::CoinType::FIL
? FindFilecoinKeyringId(address).value_or(kKeyringNotFound)
: GetMainKeyringIdForCoin(coin_type);
: GetKeyringIdForCoinNonFIL(coin_type).value_or(kKeyringNotFound);
}

std::string KeyringService::GetKeyringIdForNetwork(
Expand All @@ -1122,7 +1126,7 @@ std::string KeyringService::GetKeyringIdForNetwork(
? (network == mojom::kFilecoinMainnet
? mojom::kFilecoinKeyringId
: mojom::kFilecoinTestnetKeyringId)
: GetMainKeyringIdForCoin(coin_type);
: GetKeyringIdForCoinNonFIL(coin_type).value_or(kKeyringNotFound);
}

HDKeyring* KeyringService::GetHDKeyringById(
Expand Down Expand Up @@ -1549,8 +1553,12 @@ bool KeyringService::HasPendingUnlockRequest() const {

absl::optional<std::string> KeyringService::GetSelectedAccount(
mojom::CoinType coin) const {
const base::Value* value = GetPrefForKeyring(prefs_, kSelectedAccount,
GetMainKeyringIdForCoin(coin));
auto keyring_id = GetKeyringIdForCoinNonFIL(coin);
if (!keyring_id) {
NOTREACHED() << "GetFilecoinSelectedAccount must be used";
}
const base::Value* value =
GetPrefForKeyring(prefs_, kSelectedAccount, *keyring_id);
if (!value)
return absl::nullopt;
std::string address = value->GetString();
Expand Down Expand Up @@ -1891,7 +1899,7 @@ bool KeyringService::UpdateNameForHardwareAccountSync(
const std::string& address,
const std::string& name,
mojom::CoinType coin) {
auto keyring_id = GetMainKeyringIdForCoin(coin);
auto keyring_id = GetHardwareKeyringId(coin, address);
base::Value* hardware_keyrings =
GetPrefForKeyringUpdate(prefs_, kHardwareAccounts, keyring_id);
for (auto devices : hardware_keyrings->DictItems()) {
Expand Down
4 changes: 3 additions & 1 deletion components/brave_wallet/browser/keyring_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ class KeyringService : public KeyedService, public mojom::KeyringService {
const absl::optional<std::string> name,
const absl::optional<std::string> address,
const std::string& id);
static std::string GetMainKeyringIdForCoin(mojom::CoinType coin);
static absl::optional<std::string> GetKeyringIdForCoinNonFIL(
mojom::CoinType coin);
static std::string GetAccountNameForKeyring(PrefService* prefs,
const std::string& account_path,
const std::string& id);
Expand Down Expand Up @@ -294,6 +295,7 @@ class KeyringService : public KeyedService, public mojom::KeyringService {
friend class KeyringServiceAccountDiscoveryUnitTest;
friend class EthTxManagerUnitTest;
friend class FilTxManagerUnitTest;
friend class KeyringServiceUnitTest;

absl::optional<std::string> FindImportedFilecoinKeyringId(
const std::string& address) const;
Expand Down
3 changes: 3 additions & 0 deletions components/brave_wallet/common/brave_wallet.mojom
View file
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ interface KeyringService {
IsLocked() => (bool isLocked);

// Adds an account to the keyring.
// For filecoin strictly use AddFilecoinAccount.
AddAccount(string accountName, CoinType coin) => (bool success);

// Adds an account to the filecoin(keyring is choosed by network).
Expand All @@ -503,6 +504,7 @@ interface KeyringService {

// Imports an account with a specific private key to the corresponding keyring
// privateKey can be coin preferred encoding, ex. hex or base58
// For filecoin strictly use ImportFilecoinAccount.
ImportAccount(string accountName, string privateKey, CoinType coin)
=> (bool success, string address);

Expand Down Expand Up @@ -550,6 +552,7 @@ interface KeyringService {
NotifyUserInteraction();

// Obtains the selected account
// For filecoin strictly use GetFilecoinSelectedAccount
GetSelectedAccount(CoinType coin) => (string? address);

// Obtains the selected filecoin account
Expand Down
5 changes: 4 additions & 1 deletion components/brave_wallet_ui/common/async/lib.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,10 @@ export function refreshKeyringInfo () {

// Get default accounts for each CoinType
const defaultAccounts = await Promise.all(SupportedCoinTypes.map(async (coin: BraveWallet.CoinType) => {
const defaultAccount = await keyringService.getSelectedAccount(coin)
const chainId = await jsonRpcService.getChainId(coin)
const defaultAccount = coin == BraveWallet.CoinType.FIL ?
await keyringService.getFilecoinSelectedAccount(chainId.chainId) :
await keyringService.getSelectedAccount(coin)
const defaultAccountAddress = defaultAccount.address
return walletInfo.accountInfos.find((account) => account.address.toLowerCase() === defaultAccountAddress?.toLowerCase()) ?? {} as BraveWallet.AccountInfo
}))
Expand Down

0 comments on commit 9ec8862

Please sign in to comment.