From e81dd558eed78c9d1f8a30a1b6480ad8e98bdab5 Mon Sep 17 00:00:00 2001 From: Claudio DeSouza Date: Fri, 22 Nov 2024 14:06:04 +0000 Subject: [PATCH] [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: https://github.com/brave/brave-core/pull/26683 https://github.com/brave/brave-core/pull/26688 Resolves https://github.com/brave/brave-browser/issues/42453 --- .../browser/brave_wallet_utils.cc | 100 ++++++++++-------- .../brave_wallet/browser/brave_wallet_utils.h | 5 +- .../brave_wallet/browser/eth_data_builder.cc | 5 +- .../brave_wallet/browser/eth_data_builder.h | 10 +- .../browser/eth_data_builder_unittest.cc | 11 +- .../brave_wallet/browser/json_rpc_service.cc | 3 +- .../browser/json_rpc_service_unittest.cc | 33 +++--- .../unstoppable_domains_dns_resolve.cc | 20 +--- .../browser/unstoppable_domains_dns_resolve.h | 18 +++- 9 files changed, 113 insertions(+), 92 deletions(-) diff --git a/components/brave_wallet/browser/brave_wallet_utils.cc b/components/brave_wallet/browser/brave_wallet_utils.cc index 19707eb14ab1..8665e2615140 100644 --- a/components/brave_wallet/browser/brave_wallet_utils.cc +++ b/components/brave_wallet/browser/brave_wallet_utils.cc @@ -11,6 +11,7 @@ #include #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 +bool EncodeStringArrayInternal(base::span input, + std::string* output) { + // Write count of elements. + bool success = PadHexEncodedParameter( + Uint256ValueToHex(static_cast(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& input, +bool EncodeStringArray(base::span input, std::string* output) { - // Write count of elements. - bool success = PadHexEncodedParameter( - Uint256ValueToHex(static_cast(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 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 index ac41b3a00374..74466f6312d9 100644 --- a/components/brave_wallet/browser/brave_wallet_utils.h +++ b/components/brave_wallet/browser/brave_wallet_utils.h @@ -11,6 +11,7 @@ #include #include +#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> MnemonicToEntropy( bool IsValidMnemonic(base::cstring_view mnemonic); bool EncodeString(std::string_view input, std::string* output); -bool EncodeStringArray(const std::vector& input, +bool EncodeStringArray(base::span input, + std::string* output); +bool EncodeStringArray(base::span 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 index efdb3add9d2b..760f6b1623a1 100644 --- 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 SupportsInterface(eth_abi::Span4 interface) { namespace unstoppable_domains { -std::optional GetMany(const std::vector& keys, - std::string_view domain) { +std::optional GetMany(std::string_view domain, + base::span 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 index d4417e7fc4fb..a325c48d7fe0 100644 --- a/components/brave_wallet/browser/eth_data_builder.h +++ b/components/brave_wallet/browser/eth_data_builder.h @@ -11,7 +11,9 @@ #include #include +#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 GetMany(const std::vector& 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 GetMany( + std::string_view domain, + base::span keys = unstoppable_domains::kRecordKeys); std::vector 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 index f5ae41214cc4..374e329962a8 100644 --- 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 keys = {"crypto.ETH.address"}; - auto data = GetMany(keys, "brave.crypto"); + auto data = GetMany("brave.crypto", + std::to_array({"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( + {"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 index 0395a2d4f41d..ff0a79fed144 100644 --- 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 index 9fb8ebdca68b..eb86f555dcad 100644 --- 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 index 163e9b61af9f..6070dde0aa07 100644 --- 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(RecordKeys::kKeyCount), "Size should match between RecordKeys and kRecordKeys."); } // namespace -GURL ResolveUrl(const std::vector& response) { - if (response.size() != GetRecordKeys().size()) { +GURL ResolveUrl(base::span response) { + if (response.size() != kRecordKeys.size()) { return GURL(); } @@ -64,10 +56,4 @@ GURL ResolveUrl(const std::vector& response) { return GURL(); } -const std::vector& GetRecordKeys() { - static base::NoDestructor> 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 index 1cb04e5501c0..b1998e3c4b1a 100644 --- 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 #include -#include +#include "base/containers/span.h" #include "url/gurl.h" namespace brave_wallet::unstoppable_domains { -const std::vector& GetRecordKeys(); -GURL ResolveUrl(const std::vector& response); +// See +// https://docs.unstoppabledomains.com/developer-toolkit/resolve-domains-browser/browser-resolution-algorithm/ +// for more details. +inline constexpr auto kRecordKeys = std::to_array({ + "dweb.ipfs.hash", + "ipfs.html.value", + "dns.A", + "dns.AAAA", + "browser.redirect_url", + "ipfs.redirect_domain.value", +}); + +GURL ResolveUrl(base::span response); } // namespace brave_wallet::unstoppable_domains