Solana auto discovery #16530

nvonpentz · 2023-01-04T19:53:35Z

Resolves brave/brave-browser#27246
Security review - https://github.com/brave/security/issues/1161

Summary of changes

Adds support for automatic 'discovery' of Solana assets, meaning when new Solana accounts are added or when the frontend calls DiscoverAssetsOnAllSupportedChains(), we issue getTokenAccountsByOwner RPC calls to fetch the associated SPL token mint addresses owned by each address, checks if those tokens are in our registry, and adds them as visible asset if so.

This PR integrates this logic in the AssetDiscoveryManager which already has similar logic for Ethereum asset discovery.

I squashed to one commit, so I created this list to guide review:

Add JsonRpcService::GetSolanaTokenAccountsByOwner methods that implement the getTokenAccountsByOwner method. 1 RPC request will return all the tokens owned by a single address.
1. Implement getTokenAccountsByOwner in components/brave_wallet/browser/solana_requests.cc to create the request bodies for the RPC call
2. Modify Rust JSON converter function convert_uint64_in_object_array_to_string to support conversions of fields that are not at the top level of the JSON object in the array.
Implement core Solana asset discovery logic in AssetDiscoveryManager
1. Implement AssetDiscoveryManager::DiscoverSolAssets which takes a list of Solana account addresses, and calls JsonRpcService::GetSolanaTokenAccountsByOwner for each concurrently using a barrier callback
2. Implement AssetDiscoveryManager::OnGetSolanaTokenAccountsByOwner to handle the result of an individual RPC calls by decoding the data (base64, then borsh) to get the mint addresses returned in the RPC call, calls the barrier callback with the decoded list of mint addresses
  1. AssetDiscoveryManager::DecodeMintAddress does the borsh decoding to get the address
3. AssetDiscoveryManager::MergeDiscoveredSolanaAssets is called with a list of lists of discovered mint addresses (for for each address). It dedupes the addresses, then fetches the Sol token Registry
4. AssetDiscoveryManager::OnGetSolanaTokenRegistry cross references the returned mint addresses with the list of token registry, and adds the token if it's in the registry
5. Calls AssetDiscoveryManager::CompleteDiscoverAssets to trigger notifications and/or callbacks for tests
Trigger Solana asset discovery when Solana accounts are added
1. Modify KeyringService such that NotifyAccountsAdded() is also called when SOL accounts are added
2. Add a mojom::CoinType param to DiscoverAssetsOnAllSupportedChainsAccountsAdded, and if it's SOL, call DiscoverSolAssets
Trigger Solana asset discovery in BraveWalletService::DiscoverAssetsOnAllSupportedChains (this is what's called by frontends)
1. Modify BraveWalletService::DiscoverAssetsOnAllSupportedChains such that it also fetches the users' Solana addresses to perform asset discovery with (calls AssetDiscoveryManager::DiscoverAssetsOnAllSupportedChainsRefresh)
2. Modify AssetDiscoveryManager::DiscoverAssetsOnAllSupportedChainsRefresh such that it takes a map of mojom::CoinType -> vector of addresses, representing the addresses for each CoinType the user has, and now call DiscoverSolAssets as well as DiscoverEthAssets (eth)

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Set up an account on Solana mainnet that has tokens listed our token registry, but are not added as visible assets. Add this account using each of these methods, and verify the asset is now a visible asset:

Create wallet
Restore wallet
Add account (standard way)
Import private key
Import hardware account

When frontends have implemented calling the DiscoverAssetsOnAllSupportedChains() upon page refresh, then Solana tokens should automatically be added because of the work in this PR, but I don't believe there's a way to test that now, so that should be tested when those changes land.

nvonpentz · 2023-01-11T19:06:39Z

components/brave_wallet/browser/asset_discovery_manager.cc

+      base::BindOnce(&AssetDiscoveryManager::MergeDiscoveredSolanaAssets,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     triggered_by_accounts_added));
+  for (const auto& account_address : account_addresses) {


Should we limit to a fixed number of addresses? Thinking of edge cases where the wallet might have thousands Solana accounts.

I'll need to go back and find the conversation, but I remember having a conversation with @darkdh about how there was an optimized way to limit the number of accounts we checked. It may have just been a naive solution like only check upto a list of 100, but I can't remember. Let me dig a bit more and get back to you on this for a recommendation because I agree that we don't want to be checking 100s or 1000s of accounts.

From the looks of it this is only addresses in our wallet and most usage encourages people to keep assets in the same account rather than split to many different ones, so I don't think this edge case is a concern.

kdenhartog

No blockers, but left a few comments as nits

kdenhartog · 2023-01-13T03:15:59Z

components/brave_wallet/browser/asset_discovery_manager.cc

+// static
+absl::optional<std::string> AssetDiscoveryManager::DecodeMintAddress(
+    const std::vector<uint8_t>& data) {
+  if (data.size() < 32) {


think this should be != unless theres a case where we expect it's a bit longer that I'm not thinking of

It is a bit longer :)
https://github.com/solana-labs/solana-program-library/blob/f97a3dc7cf0e6b8e346d473a8c9d02de7b213cfd/token/program/src/state.rs#L86-L105
@nvonpentz please add a comment that we are parsing this.

I actually already implemented some code to parse that struct but needed different field

brave-core/components/brave_wallet/browser/sns_resolver_task.cc

Lines 150 to 167 in ca4b76e

static absl::optional<SplAccountData> FromBytes(

base::span<const uint8_t> data_span) {

// https://github.com/solana-labs/solana-program-library/blob/f97a3dc7cf0e6b8e346d473a8c9d02de7b213cfd/token/program/src/state.rs#L129

const size_t kSplAccountDataSize = 165;

absl::optional<SplAccountData> result;

if (data_span.size() != kSplAccountDataSize)

return result;

result.emplace();

// https://github.com/solana-labs/solana-program-library/blob/f97a3dc7cf0e6b8e346d473a8c9d02de7b213cfd/token/program/src/state.rs#L133

const size_t owner_offset = 32;

auto address = SolanaAddress::FromBytes(

data_span.subspan(owner_offset, kSolanaPubkeySize));

if (!address)

return absl::nullopt;

result->owner = *address;

return result;

We should extract all these parsing to some utility place one day.

kdenhartog · 2023-01-13T03:28:07Z

components/brave_wallet/browser/asset_discovery_manager.cc

+      base::BindOnce(&AssetDiscoveryManager::MergeDiscoveredSolanaAssets,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     triggered_by_accounts_added));
+  for (const auto& account_address : account_addresses) {


From the looks of it this is only addresses in our wallet and most usage encourages people to keep assets in the same account rather than split to many different ones, so I don't think this edge case is a concern.

supermassive · 2023-01-16T05:51:03Z

components/brave_wallet/browser/solana_response_parser.cc

+bool ParseGetTokenAccountsByOwner(
+    const base::Value& json_value,
+    std::vector<absl::optional<SolanaAccountInfo>>* accounts) {
+  DCHECK(accounts);
+
+  auto result = ParseResultDict(json_value);
+  if (!result)
+    return false;
+
+  const auto* account_dicts_list = result->FindList("value");
+  if (!account_dicts_list)
+    return false;
+
+  // For each account dict, get the "account" key and parse the value as an
+  // SolanaAccountInfo
+  for (const auto& account_dict : *account_dicts_list) {
+    const auto* account_dict_value = account_dict.GetIfDict();
+    if (!account_dict_value)
+      return false;
+
+    const auto* account_value = account_dict_value->Find("account");
+    if (!account_value)
+      return false;
+
+    absl::optional<SolanaAccountInfo> account_info;
+    if (!ParseGetAccountInfoPayload(account_value->GetDict(), &account_info))
+      return false;
+
+    accounts->push_back(std::move(account_info));
+  }
+
+  return true;
+}
+


It seems to me we should return std::vector<SolanaAccountInfo>* here(no optional)
ParseGetAccountInfoPayload also called to parse getAccountInfo response which may have no account found for pubkey https://docs.solana.com/developing/clients/jsonrpc-api#results
For ParseGetTokenAccountsByOwner null accounts are not expected and we should just skip them when populating found accounts list.

supermassive · 2023-01-16T06:14:17Z

components/brave_wallet/browser/asset_discovery_manager.cc

+
+void AssetDiscoveryManager::MergeDiscoveredSolanaAssets(
+    bool triggered_by_accounts_added,
+    const std::vector<std::vector<std::string>>&


I'd suggest using SolanaAddress instead of string so code around becomes a bit more typesafe and clear

supermassive · 2023-01-16T06:26:43Z

components/brave_wallet/browser/asset_discovery_manager.cc

+  std::vector<uint8_t> pub_key_bytes(data.begin(), data.begin() + 32);
+  std::string pub_key = Base58Encode(pub_key_bytes);
+  return pub_key;


Please return as optional<SolanaAddress>.
Nit: avoid temporary vector here and use base::span

supermassive · 2023-01-19T04:19:12Z

components/brave_wallet/browser/asset_discovery_manager.cc

+  base::flat_set<std::string> discovered_mint_addresses;
+  for (const auto& discovered_contract_address_list :
+       all_discovered_contract_addresses) {
+    for (const auto& discovered_contract_address :
+         discovered_contract_address_list) {
+      discovered_mint_addresses.insert(discovered_contract_address.ToBase58());
+    }
+  }
+


Complexity of this is O(n^2).
Makes sense to collect everything in std::vector and convert inplace to flat_set when binding to OnGetSolanaTokenRegistry callback

supermassive

LGTM with one comment

* Add JsonRpcService::GetSolanaTokenAccountsByOwner * Implement AssetDiscoveryManager::DiscoverSolAssets * Modify KeyringService such NotifyAccountsAdded is called for SOL accounts * Update BraveWalletService::DiscoverAssetsOnAllSupportedChains such to pass a list of SOL addresses to use in asset discovery * Modify AssetDiscoveryManager::DiscoverAssetsOnAllSupportedChainsAccounts added, and AssetDiscoveryManager::DiscoverAssetsOnAllSupportedChainsRefresh to initiate asset discovery for SOL and ETH chains

github-actions bot assigned nvonpentz Jan 4, 2023

nvonpentz force-pushed the solana-auto-discovery branch 5 times, most recently from 53e81a8 to d22fdc1 Compare January 10, 2023 19:17

nvonpentz marked this pull request as ready for review January 11, 2023 19:03

nvonpentz requested a review from a team as a code owner January 11, 2023 19:03

nvonpentz force-pushed the solana-auto-discovery branch from eda0ab3 to aba79e0 Compare January 11, 2023 19:03

nvonpentz commented Jan 11, 2023

View reviewed changes

nvonpentz force-pushed the solana-auto-discovery branch 3 times, most recently from e5d7570 to 8f439f3 Compare January 13, 2023 18:15

kdenhartog approved these changes Jan 16, 2023

View reviewed changes

supermassive reviewed Jan 16, 2023

View reviewed changes

nvonpentz force-pushed the solana-auto-discovery branch from 8f439f3 to 03265e3 Compare January 18, 2023 16:11

supermassive reviewed Jan 19, 2023

View reviewed changes

supermassive approved these changes Jan 19, 2023

View reviewed changes

nvonpentz force-pushed the solana-auto-discovery branch from 03265e3 to 50fb38d Compare January 20, 2023 16:24

nvonpentz force-pushed the solana-auto-discovery branch from 50fb38d to a5cb516 Compare January 20, 2023 18:37

nvonpentz merged commit 0945438 into master Jan 20, 2023

nvonpentz deleted the solana-auto-discovery branch January 20, 2023 20:16

github-actions bot added this to the 1.49.x - Nightly milestone Jan 20, 2023

srirambv mentioned this pull request Feb 22, 2023

Auto discovering Solana assets brave/brave-browser#27246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solana auto discovery #16530

Solana auto discovery #16530

nvonpentz commented Jan 4, 2023 •

edited

Loading

nvonpentz Jan 11, 2023

kdenhartog Jan 12, 2023

kdenhartog Jan 13, 2023

kdenhartog left a comment

kdenhartog Jan 13, 2023

supermassive Jan 16, 2023

kdenhartog Jan 13, 2023

supermassive Jan 16, 2023 •

edited

Loading

supermassive Jan 16, 2023

supermassive Jan 16, 2023 •

edited

Loading

supermassive Jan 19, 2023

supermassive left a comment

	static absl::optional<SplAccountData> FromBytes(
	base::span<const uint8_t> data_span) {
	// https://github.com/solana-labs/solana-program-library/blob/f97a3dc7cf0e6b8e346d473a8c9d02de7b213cfd/token/program/src/state.rs#L129
	const size_t kSplAccountDataSize = 165;
	absl::optional<SplAccountData> result;

	if (data_span.size() != kSplAccountDataSize)
	return result;

	result.emplace();
	// https://github.com/solana-labs/solana-program-library/blob/f97a3dc7cf0e6b8e346d473a8c9d02de7b213cfd/token/program/src/state.rs#L133
	const size_t owner_offset = 32;
	auto address = SolanaAddress::FromBytes(
	data_span.subspan(owner_offset, kSolanaPubkeySize));
	if (!address)
	return absl::nullopt;
	result->owner = *address;
	return result;

Solana auto discovery #16530

Solana auto discovery #16530

Conversation

nvonpentz commented Jan 4, 2023 • edited Loading

Summary of changes

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdenhartog 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

supermassive Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supermassive Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supermassive left a comment

Choose a reason for hiding this comment

nvonpentz commented Jan 4, 2023 •

edited

Loading

supermassive Jan 16, 2023 •

edited

Loading

supermassive Jan 16, 2023 •

edited

Loading