[CodeHealth][wallet] Remove NoDestructor in GetRecordKeys

This function has been returning a vector of strings, to be used by `GetMany`, and therefore a `NoDestructor` instance is needed. However, the only reason why this has to be a vector of strings is because in the end `EncodeStringArray` requires a vector of strings. This PR changes `EncodeStringArray` to take spans of strings and of string views. This will allows to convert the keys into a `constexpr` value, and avoid the use of `NoDestructor`. In a follow up PR, additional callsites of `EncodeStringArray` may be migrated to the `string_view` variant. This is a follow up of these PRs: #26683 #26688 Resolves brave/brave-browser#42453
brave · Nov 22, 2024 · e81dd55 · e81dd55
1 parent a1cc245
commit e81dd55
Show file tree

Hide file tree

Showing 9 changed files with 113 additions and 92 deletions.
diff --git a/components/brave_wallet/browser/brave_wallet_utils.cc b/components/brave_wallet/browser/brave_wallet_utils.cc
@@ -11,6 +11,7 @@
 #include <utility>
 
 #include "base/check.h"
+#include "base/containers/span.h"
 #include "base/logging.h"
 #include "base/notreached.h"
 #include "base/strings/strcat.h"
@@ -169,6 +170,55 @@ bool ValidateAndFixAssetAddress(mojom::BlockchainTokenPtr& token) {
   return false;
 }
 
+template <typename StringType>
+bool EncodeStringArrayInternal(base::span<const StringType> input,
+                               std::string* output) {
+  // Write count of elements.
+  bool success = PadHexEncodedParameter(
+      Uint256ValueToHex(static_cast<uint256_t>(input.size())), output);
+  if (!success) {
+    return false;
+  }
+
+  // Write offsets to array elements.
+  size_t data_offset = input.size() * 32;  // Offset to first element.
+  std::string encoded_offset;
+  success =
+      PadHexEncodedParameter(Uint256ValueToHex(data_offset), &encoded_offset);
+  if (!success) {
+    return false;
+  }
+  *output += encoded_offset.substr(2);
+
+  for (size_t i = 1; i < input.size(); i++) {
+    // Offset for ith element =
+    //     offset for i-1th + 32 * (count for i-1th) +
+    //     32 * ceil(i-1th.size() / 32.0) (length of encoding for i-1th).
+    std::string encoded_offset_for_element;
+    size_t rows = std::ceil(input[i - 1].size() / 32.0);
+    data_offset += (rows + 1) * 32;
+
+    success = PadHexEncodedParameter(Uint256ValueToHex(data_offset),
+                                     &encoded_offset_for_element);
+    if (!success) {
+      return false;
+    }
+    *output += encoded_offset_for_element.substr(2);
+  }
+
+  // Write count and encoding for array elements.
+  for (const auto& entry : input) {
+    std::string encoded_string;
+    success = EncodeString(entry, &encoded_string);
+    if (!success) {
+      return false;
+    }
+    *output += encoded_string.substr(2);
+  }
+
+  return true;
+}
+
 }  // namespace
 
 bool IsEndpointUsingBraveWalletProxy(const GURL& url) {
@@ -299,52 +349,14 @@ bool EncodeString(std::string_view input, std::string* output) {
   return true;
 }
 
-bool EncodeStringArray(const std::vector<std::string>& input,
+bool EncodeStringArray(base::span<const std::string> input,
                        std::string* output) {
-  // Write count of elements.
-  bool success = PadHexEncodedParameter(
-      Uint256ValueToHex(static_cast<uint256_t>(input.size())), output);
-  if (!success) {
-    return false;
-  }
-
-  // Write offsets to array elements.
-  size_t data_offset = input.size() * 32;  // Offset to first element.
-  std::string encoded_offset;
-  success =
-      PadHexEncodedParameter(Uint256ValueToHex(data_offset), &encoded_offset);
-  if (!success) {
-    return false;
-  }
-  *output += encoded_offset.substr(2);
-
-  for (size_t i = 1; i < input.size(); i++) {
-    // Offset for ith element =
-    //     offset for i-1th + 32 * (count for i-1th) +
-    //     32 * ceil(i-1th.size() / 32.0) (length of encoding for i-1th).
-    std::string encoded_offset_for_element;
-    size_t rows = std::ceil(input[i - 1].size() / 32.0);
-    data_offset += (rows + 1) * 32;
-
-    success = PadHexEncodedParameter(Uint256ValueToHex(data_offset),
-                                     &encoded_offset_for_element);
-    if (!success) {
-      return false;
-    }
-    *output += encoded_offset_for_element.substr(2);
-  }
-
-  // Write count and encoding for array elements.
-  for (const auto& entry : input) {
-    std::string encoded_string;
-    success = EncodeString(entry, &encoded_string);
-    if (!success) {
-      return false;
-    }
-    *output += encoded_string.substr(2);
-  }
+  return EncodeStringArrayInternal(input, output);
+}
 
-  return true;
+bool EncodeStringArray(base::span<const std::string_view> input,
+                       std::string* output) {
+  return EncodeStringArrayInternal(input, output);
 }
 
 bool DecodeString(size_t offset, std::string_view input, std::string* output) {

diff --git a/components/brave_wallet/browser/brave_wallet_utils.h b/components/brave_wallet/browser/brave_wallet_utils.h
@@ -11,6 +11,7 @@
 #include <string>
 #include <vector>
 
+#include "base/containers/span.h"
 #include "base/strings/cstring_view.h"
 #include "brave/components/brave_wallet/common/brave_wallet.mojom.h"
 #include "brave/components/brave_wallet/common/brave_wallet_types.h"
@@ -43,7 +44,9 @@ std::unique_ptr<std::vector<uint8_t>> MnemonicToEntropy(
 bool IsValidMnemonic(base::cstring_view mnemonic);
 
 bool EncodeString(std::string_view input, std::string* output);
-bool EncodeStringArray(const std::vector<std::string>& input,
+bool EncodeStringArray(base::span<const std::string> input,
+                       std::string* output);
+bool EncodeStringArray(base::span<const std::string_view> input,
                        std::string* output);
 
 bool DecodeString(size_t offset, std::string_view input, std::string* output);

diff --git a/components/brave_wallet/browser/eth_data_builder.cc b/components/brave_wallet/browser/eth_data_builder.cc
@@ -11,6 +11,7 @@
 
 #include "base/check.h"
 #include "base/containers/fixed_flat_map.h"
+#include "base/containers/span.h"
 #include "base/logging.h"
 #include "base/notreached.h"
 #include "base/strings/strcat.h"
@@ -313,8 +314,8 @@ std::vector<uint8_t> SupportsInterface(eth_abi::Span4 interface) {
 
 namespace unstoppable_domains {
 
-std::optional<std::string> GetMany(const std::vector<std::string>& keys,
-                                   std::string_view domain) {
+std::optional<std::string> GetMany(std::string_view domain,
+                                   base::span<const std::string_view> keys) {
   const std::string function_hash =
       GetFunctionHash("getMany(string[],uint256)");
 

diff --git a/components/brave_wallet/browser/eth_data_builder.h b/components/brave_wallet/browser/eth_data_builder.h
@@ -11,7 +11,9 @@
 #include <string_view>
 #include <vector>
 
+#include "base/containers/span.h"
 #include "base/values.h"
+#include "brave/components/brave_wallet/browser/unstoppable_domains_dns_resolve.h"
 #include "brave/components/brave_wallet/common/brave_wallet.mojom.h"
 #include "brave/components/brave_wallet/common/brave_wallet_types.h"
 #include "brave/components/brave_wallet/common/eth_abi_utils.h"
@@ -95,8 +97,12 @@ namespace unstoppable_domains {
 inline constexpr uint8_t kGetManySelector[] = {0x1b, 0xd8, 0xcc, 0x1a};
 
 // Get mutiple record values mapped with keys of the target domain.
-std::optional<std::string> GetMany(const std::vector<std::string>& keys,
-                                   std::string_view domain);
+// Keys are defaulted to `unstoppable_domains::RecordKeys` as this is the only
+// value that is used in production, with the argument only being provided for
+// tests.
+std::optional<std::string> GetMany(
+    std::string_view domain,
+    base::span<const std::string_view> keys = unstoppable_domains::kRecordKeys);
 
 std::vector<std::string> MakeEthLookupKeyList(std::string_view symbol,
                                               std::string_view chain_id);

diff --git a/components/brave_wallet/browser/eth_data_builder_unittest.cc b/components/brave_wallet/browser/eth_data_builder_unittest.cc
@@ -187,8 +187,8 @@ TEST(EthCallDataBuilderTest, SupportsInterface) {
 namespace unstoppable_domains {
 
 TEST(EthCallDataBuilderTest, GetMany) {
-  std::vector<std::string> keys = {"crypto.ETH.address"};
-  auto data = GetMany(keys, "brave.crypto");
+  auto data = GetMany("brave.crypto",
+                      std::to_array<std::string_view>({"crypto.ETH.address"}));
   EXPECT_TRUE(data);
   EXPECT_EQ(data,
             "0x1bd8cc1a"
@@ -205,9 +205,10 @@ TEST(EthCallDataBuilderTest, GetMany) {
             // Encoding of "crypto.ETH.address"
             "63727970746f2e4554482e616464726573730000000000000000000000000000");
 
-  keys = {"dweb.ipfs.hash", "ipfs.html.value", "browser.redirect_url",
-          "ipfs.redirect_domain.value"};
-  data = GetMany(keys, "brave.crypto");
+  data = GetMany("brave.crypto",
+                 std::to_array<std::string_view>(
+                     {"dweb.ipfs.hash", "ipfs.html.value",
+                      "browser.redirect_url", "ipfs.redirect_domain.value"}));
   EXPECT_TRUE(data);
   EXPECT_EQ(data,
             "0x1bd8cc1a"

diff --git a/components/brave_wallet/browser/json_rpc_service.cc b/components/brave_wallet/browser/json_rpc_service.cc
@@ -1700,8 +1700,7 @@ void JsonRpcService::UnstoppableDomainsResolveDns(
     return;
   }
 
-  auto data = unstoppable_domains::GetMany(unstoppable_domains::GetRecordKeys(),
-                                           domain);
+  auto data = unstoppable_domains::GetMany(domain);
   if (!data) {
     std::move(callback).Run(
         std::nullopt, mojom::ProviderError::kInvalidParams,

diff --git a/components/brave_wallet/browser/json_rpc_service_unittest.cc b/components/brave_wallet/browser/json_rpc_service_unittest.cc
@@ -3177,10 +3177,11 @@ class UDGetManyCallHandler : public EthCallHandler {
         eth_abi::TupleEncoder().AddStringArray(result_strings));
   }
 
-  void AddItem(const std::string& domain,
-               const std::string& key,
-               const std::string& value) {
-    items_.push_back(Item{domain, key, value});
+  void AddItem(std::string_view domain,
+               std::string_view key,
+               std::string_view value) {
+    items_.push_back(
+        Item{std::string(domain), std::string(key), std::string(value)});
   }
 
   void Reset() {
@@ -3637,24 +3638,24 @@ TEST_F(UnstoppableDomainsUnitTest, ResolveDns_ManyCalls) {
                               "QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR"),
           mojom::ProviderError::kSuccess, ""));
 
-  auto& keys = unstoppable_domains::GetRecordKeys();
-  ASSERT_EQ(6u, keys.size());
+  ASSERT_EQ(6u, unstoppable_domains::kRecordKeys.size());
   // This will resolve brave.crypto requests.
   eth_mainnet_getmany_call_handler_->AddItem(
-      "brave.crypto", keys[0],
+      "brave.crypto", unstoppable_domains::kRecordKeys[0],
       "QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR");
-  eth_mainnet_getmany_call_handler_->AddItem("brave.crypto", keys[5],
-                                             "https://brave.com");
-  polygon_getmany_call_handler_->AddItem("brave.crypto", keys[5],
-                                         "https://brave.com");
+  eth_mainnet_getmany_call_handler_->AddItem(
+      "brave.crypto", unstoppable_domains::kRecordKeys[5], "https://brave.com");
+  polygon_getmany_call_handler_->AddItem(
+      "brave.crypto", unstoppable_domains::kRecordKeys[5], "https://brave.com");
 
   // This will resolve brave.x requests.
   polygon_getmany_call_handler_->AddItem(
-      "brave.x", keys[0], "QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR");
-  polygon_getmany_call_handler_->AddItem("brave.x", keys[5],
-                                         "https://brave.com");
-  eth_mainnet_getmany_call_handler_->AddItem("brave.x", keys[5],
-                                             "https://brave.com");
+      "brave.x", unstoppable_domains::kRecordKeys[0],
+      "QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR");
+  polygon_getmany_call_handler_->AddItem(
+      "brave.x", unstoppable_domains::kRecordKeys[5], "https://brave.com");
+  eth_mainnet_getmany_call_handler_->AddItem(
+      "brave.x", unstoppable_domains::kRecordKeys[5], "https://brave.com");
 
   EXPECT_EQ(0, eth_mainnet_getmany_call_handler_->calls_number());
   EXPECT_EQ(0, polygon_getmany_call_handler_->calls_number());

diff --git a/components/brave_wallet/browser/unstoppable_domains_dns_resolve.cc b/components/brave_wallet/browser/unstoppable_domains_dns_resolve.cc
@@ -5,7 +5,7 @@
 
 #include "brave/components/brave_wallet/browser/unstoppable_domains_dns_resolve.h"
 
-#include "base/no_destructor.h"
+#include "base/containers/span.h"
 #include "brave/components/ipfs/ipfs_utils.h"
 
 namespace brave_wallet::unstoppable_domains {
@@ -21,21 +21,13 @@ enum RecordKeys {
   kKeyCount,
 };
 
-// See
-// https://docs.unstoppabledomains.com/developer-toolkit/resolve-domains-browser/browser-resolution-algorithm/
-// for more details.
-constexpr const char* kRecordKeys[] = {
-    "dweb.ipfs.hash", "ipfs.html.value",      "dns.A",
-    "dns.AAAA",       "browser.redirect_url", "ipfs.redirect_domain.value",
-};
-
 static_assert(std::size(kRecordKeys) ==
                   static_cast<size_t>(RecordKeys::kKeyCount),
               "Size should match between RecordKeys and kRecordKeys.");
 }  // namespace
 
-GURL ResolveUrl(const std::vector<std::string>& response) {
-  if (response.size() != GetRecordKeys().size()) {
+GURL ResolveUrl(base::span<const std::string> response) {
+  if (response.size() != kRecordKeys.size()) {
     return GURL();
   }
 
@@ -64,10 +56,4 @@ GURL ResolveUrl(const std::vector<std::string>& response) {
   return GURL();
 }
 
-const std::vector<std::string>& GetRecordKeys() {
-  static base::NoDestructor<std::vector<std::string>> keys(
-      std::begin(kRecordKeys), std::end(kRecordKeys));
-  return *keys;
-}
-
 }  // namespace brave_wallet::unstoppable_domains
diff --git a/components/brave_wallet/browser/unstoppable_domains_dns_resolve.h b/components/brave_wallet/browser/unstoppable_domains_dns_resolve.h
@@ -6,15 +6,27 @@
 #ifndef BRAVE_COMPONENTS_BRAVE_WALLET_BROWSER_UNSTOPPABLE_DOMAINS_DNS_RESOLVE_H_
 #define BRAVE_COMPONENTS_BRAVE_WALLET_BROWSER_UNSTOPPABLE_DOMAINS_DNS_RESOLVE_H_
 
+#include <array>
 #include <string>
-#include <vector>
 
+#include "base/containers/span.h"
 #include "url/gurl.h"
 
 namespace brave_wallet::unstoppable_domains {
 
-const std::vector<std::string>& GetRecordKeys();
-GURL ResolveUrl(const std::vector<std::string>& response);
+// See
+// https://docs.unstoppabledomains.com/developer-toolkit/resolve-domains-browser/browser-resolution-algorithm/
+// for more details.
+inline constexpr auto kRecordKeys = std::to_array<std::string_view>({
+    "dweb.ipfs.hash",
+    "ipfs.html.value",
+    "dns.A",
+    "dns.AAAA",
+    "browser.redirect_url",
+    "ipfs.redirect_domain.value",
+});
+
+GURL ResolveUrl(base::span<const std::string> response);
 
 }  // namespace brave_wallet::unstoppable_domains