[DanglingPtr][wallet] Prefer non-nullable states pt.2

This change touches several parts of wallets where we are passing
pointers around that are expected to be in a valid state, and remain so
for the duration of the lifetime of the object receiving them.

Historically, chromium had a guideline that required passing non-owning
references as raw pointers. This has changed for a while now, and there
are even supporting types like `raw_ref` for it. Raw pointers should be
used for cases where flexibility is required about the lifetime of what
is being pointed at. However, we should avoid what is in fact an
anti-pattern, of passing pointers for the lifetime of an object, and
then CHECKing the pointer for its validity.

This change also removes certain uses of `std::unique_ptr` that suffers
of the same issues.

Resolves brave/brave-browser#42016

browser/brave_wallet/asset_discovery_manager_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/test/bind.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
+#include "brave/components/api_request_helper/api_request_helper.h"
 #include "brave/components/brave_wallet/browser/brave_wallet_constants.h"
 #include "brave/components/brave_wallet/browser/brave_wallet_service.h"
 #include "brave/components/brave_wallet/browser/brave_wallet_service_delegate.h"
@@ -29,6 +30,7 @@
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/browser_task_environment.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "services/network/test/test_url_loader_factory.h"
@@ -123,11 +125,15 @@ class AssetDiscoveryManagerUnitTest : public testing::Test {
     simple_hash_client_ =
         std::make_unique<SimpleHashClient>(shared_url_loader_factory_);
     asset_discovery_manager_ = std::make_unique<AssetDiscoveryManager>(
-        shared_url_loader_factory_, wallet_service_.get(), json_rpc_service_,
-        keyring_service_, simple_hash_client_.get(), GetPrefs());
+        shared_url_loader_factory_, *wallet_service_, *json_rpc_service_,
+        *keyring_service_, *simple_hash_client_, GetPrefs());
     wallet_service_observer_ = std::make_unique<
         TestBraveWalletServiceObserverForAssetDiscoveryManager>();
     wallet_service_->AddObserver(wallet_service_observer_->GetReceiver());
+
+    api_request_helper_ =
+        std::make_unique<api_request_helper::APIRequestHelper>(
+            TRAFFIC_ANNOTATION_FOR_TESTS, shared_url_loader_factory_);
   }
 
   PrefService* GetPrefs() { return profile_->GetPrefs(); }
@@ -140,6 +146,7 @@ class AssetDiscoveryManagerUnitTest : public testing::Test {
   std::unique_ptr<TestBraveWalletServiceObserverForAssetDiscoveryManager>
       wallet_service_observer_;
   content::BrowserTaskEnvironment task_environment_;
+  std::unique_ptr<api_request_helper::APIRequestHelper> api_request_helper_;
   std::unique_ptr<ScopedTestingLocalState> local_state_;
   std::unique_ptr<TestingProfile> profile_;
   std::unique_ptr<BraveWalletService> wallet_service_;
@@ -275,8 +282,9 @@ TEST_F(AssetDiscoveryManagerUnitTest, DiscoverAssetsOnAllSupportedChains) {
   task_environment_.FastForwardBy(
       base::Minutes(kAssetDiscoveryMinutesPerRequest));
   std::queue<std::unique_ptr<AssetDiscoveryTask>> tasks;
-  tasks.push(std::make_unique<AssetDiscoveryTask>(nullptr, nullptr, nullptr,
-                                                  nullptr, nullptr));
+  tasks.push(std::make_unique<AssetDiscoveryTask>(
+      *api_request_helper_, *simple_hash_client_, *wallet_service_,
+      *json_rpc_service_, GetPrefs()));
   asset_discovery_manager_->SetQueueForTesting(std::move(tasks));
   TestDiscoverAssetsOnAllSupportedChains({}, true, true, {}, 1);
 

browser/brave_wallet/asset_discovery_task_unittest.cc

@@ -184,8 +184,8 @@ class AssetDiscoveryTaskUnitTest : public testing::Test {
     simple_hash_client_ =
         std::make_unique<SimpleHashClient>(shared_url_loader_factory_);
     asset_discovery_task_ = std::make_unique<AssetDiscoveryTask>(
-        api_request_helper_.get(), simple_hash_client_.get(),
-        wallet_service_.get(), json_rpc_service_, GetPrefs());
+        *api_request_helper_, *simple_hash_client_, *wallet_service_,
+        *json_rpc_service_, GetPrefs());
     wallet_service_observer_ =
         std::make_unique<TestBraveWalletServiceObserverForAssetDiscoveryTask>();
     wallet_service_->AddObserver(wallet_service_observer_->GetReceiver());

components/brave_wallet/browser/account_discovery_manager.cc

@@ -48,8 +48,8 @@ AccountDiscoveryManager::DiscoveryContext::DiscoveryContext(
 AccountDiscoveryManager::DiscoveryContext::~DiscoveryContext() = default;
 
 AccountDiscoveryManager::AccountDiscoveryManager(
-    JsonRpcService* rpc_service,
-    KeyringService* keyring_service,
+    JsonRpcService& rpc_service,
+    KeyringService& keyring_service,
     BitcoinWalletService* bitcoin_wallet_service)
     : json_rpc_service_(rpc_service),
       keyring_service_(keyring_service),
@@ -108,11 +108,6 @@ void AccountDiscoveryManager::AddDiscoveryAccount(
     return;
   }
 
-  if (!json_rpc_service_) {
-    CHECK_IS_TEST();
-    return;
-  }
-
   auto chain_id = context->chain_id;
   auto coin_type = context->coin_type;
   if (context->coin_type == mojom::CoinType::ETH) {

components/brave_wallet/browser/account_discovery_manager.h

@@ -27,8 +27,8 @@ struct DiscoveredBitcoinAccount;
 // transactions.
 class AccountDiscoveryManager {
  public:
-  AccountDiscoveryManager(JsonRpcService* rpc_service,
-                          KeyringService* keyring_service,
+  AccountDiscoveryManager(JsonRpcService& rpc_service,
+                          KeyringService& keyring_service,
                           BitcoinWalletService* bitcoin_wallet_service);
   ~AccountDiscoveryManager();
 
@@ -75,8 +75,8 @@ class AccountDiscoveryManager {
   void ProcessDiscoveryResult(std::unique_ptr<DiscoveryContext> context,
                               bool result);
 
-  raw_ptr<brave_wallet::JsonRpcService> json_rpc_service_;
-  raw_ptr<brave_wallet::KeyringService> keyring_service_;
+  raw_ref<brave_wallet::JsonRpcService> json_rpc_service_;
+  raw_ref<brave_wallet::KeyringService> keyring_service_;
   raw_ptr<brave_wallet::BitcoinWalletService> bitcoin_wallet_service_;
 
   base::WeakPtrFactory<AccountDiscoveryManager> weak_ptr_factory_{this};

components/brave_wallet/browser/account_discovery_manager_unittest.cc

@@ -24,6 +24,7 @@
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -32,7 +33,10 @@ namespace brave_wallet {
 class AccountDiscoveryManagerUnitTest : public testing::Test {
  public:
   AccountDiscoveryManagerUnitTest()
-      : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
+      : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME),
+        shared_url_loader_factory_(
+            base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+                &url_loader_factory_)) {}
 
   ~AccountDiscoveryManagerUnitTest() override = default;
 
@@ -43,6 +47,8 @@ class AccountDiscoveryManagerUnitTest : public testing::Test {
     network_manager_ = std::make_unique<NetworkManager>(&prefs_);
     keyring_service_ =
         std::make_unique<KeyringService>(nullptr, &prefs_, &local_state_);
+    json_rpc_service_ = std::make_unique<JsonRpcService>(
+        shared_url_loader_factory_, network_manager_.get(), &prefs_, nullptr);
     bitcoin_test_rpc_server_ = std::make_unique<BitcoinTestRpcServer>();
     bitcoin_wallet_service_ = std::make_unique<BitcoinWalletService>(
         *keyring_service_, *network_manager_,
@@ -71,8 +77,11 @@ class AccountDiscoveryManagerUnitTest : public testing::Test {
   sync_preferences::TestingPrefServiceSyncable prefs_;
   sync_preferences::TestingPrefServiceSyncable local_state_;
 
+  network::TestURLLoaderFactory url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
   std::unique_ptr<BitcoinTestRpcServer> bitcoin_test_rpc_server_;
   std::unique_ptr<NetworkManager> network_manager_;
+  std::unique_ptr<JsonRpcService> json_rpc_service_;
   std::unique_ptr<KeyringService> keyring_service_;
   std::unique_ptr<BitcoinWalletService> bitcoin_wallet_service_;
   std::unique_ptr<BitcoinHDKeyring> keyring_;
@@ -89,8 +98,8 @@ TEST_F(AccountDiscoveryManagerUnitTest, DiscoverBtcAccountCreatesNew) {
 
   EXPECT_EQ(0u, GetAccountUtils().AllBtcAccounts().size());
 
-  AccountDiscoveryManager discovery_manager(nullptr, keyring_service_.get(),
-                                            bitcoin_wallet_service_.get());
+  AccountDiscoveryManager discovery_manager(
+      *json_rpc_service_, *keyring_service_, bitcoin_wallet_service_.get());
   discovery_manager.StartDiscovery();
   task_environment_.RunUntilIdle();
 
@@ -130,8 +139,8 @@ TEST_F(AccountDiscoveryManagerUnitTest, DiscoverBtcAccountUpdatesExisting) {
             *keyring_service_->GetBitcoinAccountInfo(account_id)
                  ->next_change_address->key_id);
 
-  AccountDiscoveryManager discovery_manager(nullptr, keyring_service_.get(),
-                                            bitcoin_wallet_service_.get());
+  AccountDiscoveryManager discovery_manager(
+      *json_rpc_service_, *keyring_service_, bitcoin_wallet_service_.get());
   discovery_manager.StartDiscovery();
   task_environment_.RunUntilIdle();
 

components/brave_wallet/browser/account_resolver_delegate_impl.cc

@@ -8,7 +8,7 @@
 namespace brave_wallet {
 
 AccountResolverDelegateImpl::AccountResolverDelegateImpl(
-    KeyringService* keyring_service)
+    KeyringService& keyring_service)
     : keyring_service_(keyring_service) {}
 
 mojom::AccountIdPtr AccountResolverDelegateImpl::ResolveAccountId(

components/brave_wallet/browser/account_resolver_delegate_impl.h

@@ -15,15 +15,15 @@ namespace brave_wallet {
 
 class AccountResolverDelegateImpl : public AccountResolverDelegate {
  public:
-  explicit AccountResolverDelegateImpl(KeyringService* keyring_service);
+  explicit AccountResolverDelegateImpl(KeyringService& keyring_service);
 
   mojom::AccountIdPtr ResolveAccountId(
       const std::string* from_account_id,
       const std::string* from_address) override;
   bool ValidateAccountId(const mojom::AccountIdPtr& account_id) override;
 
  private:
-  raw_ptr<KeyringService> keyring_service_ = nullptr;
+  raw_ref<KeyringService> keyring_service_;
 };
 
 }  // namespace brave_wallet

components/brave_wallet/browser/account_resolver_delegate_impl_unittest.cc

@@ -49,7 +49,7 @@ class AccountResolverDelegateImplUnitTest : public testing::Test {
     GetAccountUtils().CreateWallet(kMnemonicDivideCruise, kTestWalletPassword);
 
     resolver_ =
-        std::make_unique<AccountResolverDelegateImpl>(keyring_service_.get());
+        std::make_unique<AccountResolverDelegateImpl>(*keyring_service_);
   }
 
   ~AccountResolverDelegateImplUnitTest() override = default;

components/brave_wallet/browser/asset_discovery_manager.cc

@@ -49,10 +49,10 @@ namespace brave_wallet {
 
 AssetDiscoveryManager::AssetDiscoveryManager(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-    BraveWalletService* wallet_service,
-    JsonRpcService* json_rpc_service,
-    KeyringService* keyring_service,
-    SimpleHashClient* simple_hash_client,
+    BraveWalletService& wallet_service,
+    JsonRpcService& json_rpc_service,
+    KeyringService& keyring_service,
+    SimpleHashClient& simple_hash_client,
     PrefService* prefs)
     : api_request_helper_(std::make_unique<APIRequestHelper>(
           GetAssetDiscoveryManagerNetworkTrafficAnnotationTag(),
@@ -161,8 +161,8 @@ void AssetDiscoveryManager::AddTask(
   auto non_fungible_supported_chains = GetNonFungibleSupportedChains();
 
   auto task = std::make_unique<AssetDiscoveryTask>(
-      api_request_helper_.get(), simple_hash_client_, wallet_service_,
-      json_rpc_service_, prefs_);
+      *api_request_helper_, *simple_hash_client_, *wallet_service_,
+      *json_rpc_service_, prefs_);
   auto callback = base::BindOnce(&AssetDiscoveryManager::FinishTask,
                                  weak_ptr_factory_.GetWeakPtr());
   auto* task_ptr = task.get();

components/brave_wallet/browser/asset_discovery_manager.h

@@ -31,18 +31,17 @@ class BraveWalletService;
 class JsonRpcService;
 class SimpleHashClient;
 class KeyringService;
-class NetworkManager;
 
 class AssetDiscoveryManager : public KeyringServiceObserverBase {
  public:
   using APIRequestHelper = api_request_helper::APIRequestHelper;
   using APIRequestResult = api_request_helper::APIRequestResult;
   AssetDiscoveryManager(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      BraveWalletService* wallet_service,
-      JsonRpcService* json_rpc_service,
-      KeyringService* keyring_service,
-      SimpleHashClient* simple_hash_client,
+      BraveWalletService& wallet_service,
+      JsonRpcService& json_rpc_service,
+      KeyringService& keyring_service,
+      SimpleHashClient& simple_hash_client,
       PrefService* prefs);
 
   AssetDiscoveryManager(const AssetDiscoveryManager&) = delete;
@@ -85,11 +84,10 @@ class AssetDiscoveryManager : public KeyringServiceObserverBase {
 
   std::unique_ptr<APIRequestHelper> api_request_helper_;
   std::queue<std::unique_ptr<AssetDiscoveryTask>> queue_;
-  raw_ptr<BraveWalletService> wallet_service_;
-  raw_ptr<NetworkManager> network_manager_;
-  raw_ptr<JsonRpcService> json_rpc_service_;
-  raw_ptr<KeyringService> keyring_service_;
-  raw_ptr<SimpleHashClient> simple_hash_client_;
+  raw_ref<BraveWalletService> wallet_service_;
+  raw_ref<JsonRpcService> json_rpc_service_;
+  raw_ref<KeyringService> keyring_service_;
+  raw_ref<SimpleHashClient> simple_hash_client_;
   raw_ptr<PrefService> prefs_;
   mojo::Receiver<brave_wallet::mojom::KeyringServiceObserver>
       keyring_service_observer_receiver_{this};

components/brave_wallet/browser/asset_discovery_task.cc

@@ -29,10 +29,10 @@
 
 namespace brave_wallet {
 
-AssetDiscoveryTask::AssetDiscoveryTask(APIRequestHelper* api_request_helper,
-                                       SimpleHashClient* simple_hash_client,
-                                       BraveWalletService* wallet_service,
-                                       JsonRpcService* json_rpc_service,
+AssetDiscoveryTask::AssetDiscoveryTask(APIRequestHelper& api_request_helper,
+                                       SimpleHashClient& simple_hash_client,
+                                       BraveWalletService& wallet_service,
+                                       JsonRpcService& json_rpc_service,
                                        PrefService* prefs)
     : api_request_helper_(api_request_helper),
       simple_hash_client_(simple_hash_client),

components/brave_wallet/browser/asset_discovery_task.h

@@ -35,10 +35,10 @@ class AssetDiscoveryTask {
   using APIRequestHelper = api_request_helper::APIRequestHelper;
   using APIRequestResult = api_request_helper::APIRequestResult;
 
-  AssetDiscoveryTask(APIRequestHelper* api_request_helper,
-                     SimpleHashClient* simple_hash_client,
-                     BraveWalletService* wallet_service,
-                     JsonRpcService* json_rpc_service,
+  AssetDiscoveryTask(APIRequestHelper& api_request_helper,
+                     SimpleHashClient& simple_hash_client,
+                     BraveWalletService& wallet_service,
+                     JsonRpcService& json_rpc_service,
                      PrefService* prefs);
 
   AssetDiscoveryTask(const AssetDiscoveryTask&) = delete;
@@ -141,10 +141,10 @@ class AssetDiscoveryTask {
   static std::optional<SolanaAddress> DecodeMintAddress(
       const std::vector<uint8_t>& data);
 
-  raw_ptr<APIRequestHelper> api_request_helper_;
-  raw_ptr<SimpleHashClient> simple_hash_client_;
-  raw_ptr<BraveWalletService> wallet_service_;
-  raw_ptr<JsonRpcService> json_rpc_service_;
+  raw_ref<APIRequestHelper> api_request_helper_;
+  raw_ref<SimpleHashClient> simple_hash_client_;
+  raw_ref<BraveWalletService> wallet_service_;
+  raw_ref<JsonRpcService> json_rpc_service_;
   raw_ptr<PrefService> prefs_;
   base::WeakPtrFactory<AssetDiscoveryTask> weak_ptr_factory_;
 };

components/brave_wallet/browser/brave_wallet_service.cc

@@ -131,8 +131,7 @@ BraveWalletService::BraveWalletService(
   if (IsZCashEnabled()) {
     zcash_wallet_service_ = std::make_unique<ZCashWalletService>(
         delegate_->GetWalletBaseDirectory().AppendASCII(kZCashDataFolderName),
-        keyring_service(), profile_prefs, network_manager(),
-        url_loader_factory);
+        *keyring_service(), network_manager(), url_loader_factory);
   }
 
   tx_service_ = std::make_unique<TxService>(
@@ -145,8 +144,8 @@ BraveWalletService::BraveWalletService(
 
   simple_hash_client_ = std::make_unique<SimpleHashClient>(url_loader_factory);
   asset_discovery_manager_ = std::make_unique<AssetDiscoveryManager>(
-      url_loader_factory, this, json_rpc_service(), keyring_service(),
-      simple_hash_client_.get(), profile_prefs);
+      url_loader_factory, *this, *json_rpc_service(), *keyring_service(),
+      *simple_hash_client_, profile_prefs);
 
   delegate_->AddObserver(this);
 
@@ -1134,8 +1133,7 @@ void BraveWalletService::OnActiveOriginChanged(
 
 void BraveWalletService::WalletRestored() {
   account_discovery_manager_ = std::make_unique<AccountDiscoveryManager>(
-      json_rpc_service_.get(), keyring_service_.get(),
-      bitcoin_wallet_service_.get());
+      *json_rpc_service_, *keyring_service_, bitcoin_wallet_service_.get());
   account_discovery_manager_->StartDiscovery();
 }
 

components/brave_wallet/browser/ethereum_provider_impl.cc

@@ -1319,7 +1319,7 @@ mojom::AccountIdPtr EthereumProviderImpl::FindAuthenticatedAccountByAddress(
 
 mojom::AccountIdPtr EthereumProviderImpl::FindAccountByAddress(
     const std::string& address) {
-  AccountResolverDelegateImpl resolver(keyring_service_);
+  AccountResolverDelegateImpl resolver(*keyring_service_);
 
   auto account_id = resolver.ResolveAccountId(nullptr, &address);
   if (!account_id || account_id->coin != mojom::CoinType::ETH) {

components/brave_wallet/browser/tx_service.cc

@@ -88,7 +88,7 @@ TxService::TxService(JsonRpcService* json_rpc_service,
   delegate_ = std::make_unique<TxStorageDelegateImpl>(prefs, store_factory_,
                                                       ui_task_runner);
   account_resolver_delegate_ =
-      std::make_unique<AccountResolverDelegateImpl>(&keyring_service);
+      std::make_unique<AccountResolverDelegateImpl>(keyring_service);
 
   tx_manager_map_[mojom::CoinType::ETH] = std::unique_ptr<TxManager>(
       new EthTxManager(*this, json_rpc_service, keyring_service, *delegate_,
@@ -115,7 +115,7 @@ TxService::TxService(JsonRpcService* json_rpc_service,
       CHECK_IS_TEST();
     } else {
       tx_manager_map_[mojom::CoinType::ZEC] = std::make_unique<ZCashTxManager>(
-          *this, zcash_wallet_service, keyring_service, *delegate_,
+          *this, *zcash_wallet_service, keyring_service, *delegate_,
           *account_resolver_delegate_);
     }
   }

components/brave_wallet/browser/zcash/zcash_create_transparent_transaction_task.cc

@@ -54,7 +54,7 @@ void ZCashCreateTransparentTransactionTask::WorkOnTask() {
   }
 
   if (!chain_height_) {
-    zcash_wallet_service_->zcash_rpc()->GetLatestBlock(
+    zcash_wallet_service_->zcash_rpc().GetLatestBlock(
         chain_id_,
         base::BindOnce(&ZCashCreateTransparentTransactionTask::OnGetChainHeight,
                        weak_ptr_factory_.GetWeakPtr()));