Fixed after code review #27842

Signed-off-by: Vadym Struts <vstruts@brave.com>
brave · Feb 2, 2023 · 2e131d0 · 2e131d0
1 parent df4e7d7
commit 2e131d0
Show file tree

Hide file tree

Showing 14 changed files with 65 additions and 143 deletions.
diff --git a/browser/brave_wallet/ethereum_provider_impl_unittest.cc b/browser/brave_wallet/ethereum_provider_impl_unittest.cc
@@ -2193,7 +2193,7 @@ TEST_F(EthereumProviderImplUnitTest, EthSubscribeLogsFiltered) {
           const absl::optional<base::Value> req_body_payload =
               base::JSONReader::Read(
                   R"({"id":1,"jsonrpc":"2.0","method":"eth_getLogs","params":
-[{"address":["0x1111"],"fromBlock":"0x2211","toBlock":"0xab65",
+[{"address":["0x1111", "0x1112"],"fromBlock":"0x2211","toBlock":"0xab65",
 "topics":["0x2edc","0xb832","0x8dc8"]}]})",
                   base::JSON_PARSE_CHROMIUM_EXTENSIONS |
                       base::JSONParserOptions::JSON_PARSE_RFC);
@@ -2213,8 +2213,8 @@ TEST_F(EthereumProviderImplUnitTest, EthSubscribeLogsFiltered) {
   // Logs subscription with parameters
   std::string request_payload_json =
       R"({"id":1,"jsonrpc:": "2.0","method":"eth_subscribe",
-          "params": ["logs", {"address": "0x1111", "fromBlock": "0x2211",
-          "toBlock": "0xab65",  "topics":  ["0x2edc", "0xb832", "0x8dc8"]}]})";
+  "params": ["logs", {"address": ["0x1111", "0x1112"], "fromBlock": "0x2211",
+  "toBlock": "0xab65",  "topics":  ["0x2edc", "0xb832", "0x8dc8"]}]})";
   absl::optional<base::Value> request_payload = base::JSONReader::Read(
       request_payload_json, base::JSON_PARSE_CHROMIUM_EXTENSIONS |
                                 base::JSONParserOptions::JSON_PARSE_RFC);

diff --git a/components/brave_wallet/browser/asset_discovery_manager.cc b/components/brave_wallet/browser/asset_discovery_manager.cc
@@ -305,7 +305,7 @@ void AssetDiscoveryManager::OnGetEthTokenRegistry(
   filtering.Set("toBlock", to_block);
   filtering.Set("address", std::move(contract_addresses_to_search));
   filtering.Set("topics", std::move(topics));
-  json_rpc_service_->EthGetLogs(chain_id, base::Value(std::move(filtering)),
+  json_rpc_service_->EthGetLogs(chain_id, std::move(filtering),
                                 std::move(callback));
 }
 

diff --git a/components/brave_wallet/browser/eth_logs_tracker.cc b/components/brave_wallet/browser/eth_logs_tracker.cc
@@ -32,9 +32,9 @@ bool EthLogsTracker::IsRunning() const {
 }
 
 void EthLogsTracker::AddSubscriber(const std::string& subscription_id,
-                                   const base::Value& params) {
-  eth_logs_subscription_info_.insert(
-      std::pair<std::string, base::Value>(subscription_id, params.Clone()));
+                                   base::Value::Dict filter) {
+  eth_logs_subscription_info_.insert(std::pair<std::string, base::Value::Dict>(
+      subscription_id, std::move(filter)));
 }
 
 void EthLogsTracker::RemoveSubscriber(const std::string& subscription_id) {
@@ -54,7 +54,7 @@ void EthLogsTracker::GetLogs() {
 
   for (auto const& esi : std::as_const(eth_logs_subscription_info_)) {
     json_rpc_service_->EthGetLogs(
-        chain_id, esi.second,
+        chain_id, esi.second.Clone(),
         base::BindOnce(&EthLogsTracker::OnGetLogs, weak_factory_.GetWeakPtr(),
                        esi.first));
   }

diff --git a/components/brave_wallet/browser/eth_logs_tracker.h b/components/brave_wallet/browser/eth_logs_tracker.h
@@ -45,7 +45,7 @@ class EthLogsTracker {
   bool IsRunning() const;
 
   void AddSubscriber(const std::string& subscription_id,
-                     const base::Value& params);
+                     base::Value::Dict filter);
   void RemoveSubscriber(const std::string& subscription_id);
 
   void AddObserver(Observer* observer);
@@ -62,7 +62,7 @@ class EthLogsTracker {
   base::RepeatingTimer timer_;
   raw_ptr<JsonRpcService> json_rpc_service_ = nullptr;
 
-  std::map<std::string, base::Value> eth_logs_subscription_info_;
+  std::map<std::string, base::Value::Dict> eth_logs_subscription_info_;
 
   base::ObserverList<Observer> observers_;
 

diff --git a/components/brave_wallet/browser/eth_requests.cc b/components/brave_wallet/browser/eth_requests.cc
@@ -336,27 +336,8 @@ std::string eth_getFilterLogs(const std::string& filter_id) {
   return GetJsonRpcString("eth_getFilterLogs", filter_id);
 }
 
-std::string eth_getLogs(const std::string& from_block_quantity_tag,
-                        const std::string& to_block_quantity_tag,
-                        base::Value::List addresses,
-                        base::Value::List topics,
-                        const std::string& block_hash) {
+std::string eth_getLogs(base::Value::Dict filter_options) {
   base::Value::List params;
-  base::Value::Dict filter_options;
-  // The `address` filter option accepts either a single address, or a list of
-  // addresses (See spec:
-  // https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getlogs). At
-  // time of writing Infura's documentation suggests they only support a single
-  // address, however we have verified they also support a list.
-  if (!addresses.empty()) {
-    filter_options.Set("address", std::move(addresses));
-  }
-  AddKeyIfNotEmpty(&filter_options, "fromBlock", from_block_quantity_tag);
-  AddKeyIfNotEmpty(&filter_options, "toBlock", to_block_quantity_tag);
-  if (!topics.empty()) {
-    filter_options.Set("topics", std::move(topics));
-  }
-  AddKeyIfNotEmpty(&filter_options, "blockhash", block_hash);
   params.Append(std::move(filter_options));
   base::Value::Dict dictionary =
       GetJsonRpcDictionary("eth_getLogs", std::move(params));

diff --git a/components/brave_wallet/browser/eth_requests.h b/components/brave_wallet/browser/eth_requests.h
@@ -190,11 +190,7 @@ std::string eth_getFilterChanges(const std::string& filter_id);
 // Returns an array of all logs matching filter with given id.
 std::string eth_getFilterLogs(const std::string& filter_id);
 // Returns an array of all logs matching a given filter object.
-std::string eth_getLogs(const std::string& from_block_quantity_tag,
-                        const std::string& to_block_quantity_tag,
-                        base::Value::List addresses,
-                        base::Value::List topics,
-                        const std::string& block_hash);
+std::string eth_getLogs(base::Value::Dict filter_options);
 // Returns the hash of the current block, the seedHash, and the boundary
 // condition to be met (“target”).
 std::string eth_getWork();

diff --git a/components/brave_wallet/browser/eth_requests_unittest.cc b/components/brave_wallet/browser/eth_requests_unittest.cc
@@ -337,10 +337,17 @@ TEST(EthRequestUnitTest, eth_getLogs) {
 
   base::Value::List addresses;
   addresses.Append(base::Value("0x8888f1f195afa192cfee860698584c030f4c9db1"));
+
+  base::Value::Dict filtering;
+  filtering.Set("fromBlock", "0x1");
+  filtering.Set("toBlock", "0x2");
+  filtering.Set("address", std::move(addresses));
+  filtering.Set("topics", std::move(topics));
+  filtering.Set(
+      "blockhash",
+      "0xb903239f8543d04b5dc1ba6579132b143087c68db1b2168786408fcbce568238");
   ASSERT_EQ(
-      eth_getLogs(
-          "0x1", "0x2", std::move(addresses), std::move(topics),
-          "0xb903239f8543d04b5dc1ba6579132b143087c68db1b2168786408fcbce568238"),
+      eth_getLogs(std::move(filtering)),
       R"({"id":1,"jsonrpc":"2.0","method":"eth_getLogs","params":[{"address":["0x8888f1f195afa192cfee860698584c030f4c9db1"],"blockhash":"0xb903239f8543d04b5dc1ba6579132b143087c68db1b2168786408fcbce568238","fromBlock":"0x1","toBlock":"0x2","topics":["0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b",["0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b","0x0000000000000000000000000aff3454fce5edbc8cca8697c15331677e6ebccc"]]}]})");  // NOLINT
 }
 

diff --git a/components/brave_wallet/browser/ethereum_provider_impl.cc b/components/brave_wallet/browser/ethereum_provider_impl.cc
@@ -578,9 +578,11 @@ void EthereumProviderImpl::RecoverAddress(const std::string& message,
                           false);
 }
 
-void EthereumProviderImpl::EthSubscribe(const base::Value& params,
-                                        RequestCallback callback,
-                                        base::Value id) {
+void EthereumProviderImpl::EthSubscribe(
+    const std::string& event_type,
+    absl::optional<base::Value::Dict> filter,
+    RequestCallback callback,
+    base::Value id) {
   const auto generateHexBytes = [](std::vector<std::string>& subscriptions) {
     std::vector<uint8_t> bytes(16);
     crypto::RandBytes(&bytes.front(), bytes.size());
@@ -589,60 +591,22 @@ void EthereumProviderImpl::EthSubscribe(const base::Value& params,
     return std::tuple<bool, std::string>{subscriptions.size() == 1, hex_bytes};
   };
 
-  const auto is_event_type = [&](const std::string& eq_val) {
-    if (!params.is_list()) {
-      return false;
-    }
-
-    const auto& list = params.GetList();
-    auto it = std::find_if(list.begin(), list.end(), [&](const auto& item) {
-      return item.is_string() && eq_val == item.GetString();
-    });
-
-    if (it != list.end()) {
-      return true;
-    }
-
-    return false;
-  };
-
-  if (is_event_type(kEthSubscribeNewHeads)) {
+  if (event_type == kEthSubscribeNewHeads) {
     const auto gen_res = generateHexBytes(eth_subscriptions_);
     if (std::get<0>(gen_res)) {
       eth_block_tracker_.Start(
           base::Seconds(kBlockTrackerDefaultTimeInSeconds));
     }
     std::move(callback).Run(std::move(id), base::Value(std::get<1>(gen_res)),
                             false, "", false);
-  } else if (is_event_type(kEthSubscribeLogs)) {
+  } else if (event_type == kEthSubscribeLogs && filter) {
     const auto gen_res = generateHexBytes(eth_log_subscriptions_);
 
-    const auto get_filter_options = [&]() {
-      if (!params.is_list()) {
-        return base::Value();
-      }
-
-      const auto& list = params.GetList();
-      auto it = std::find_if(list.begin(), list.end(), [](const auto& item) {
-        return item.is_dict() && (nullptr != item.GetDict().Find("address") ||
-                                  nullptr != item.GetDict().Find("topics") ||
-                                  nullptr != item.GetDict().Find("fromBlock") ||
-                                  nullptr != item.GetDict().Find("toBlock") ||
-                                  nullptr != item.GetDict().Find("blockHash"));
-      });
-      if (it == list.end()) {
-        return base::Value();
-      }
-
-      return base::Value(it->Clone());
-    };
-
     if (std::get<0>(gen_res)) {
       eth_logs_tracker_.Start(base::Seconds(kLogTrackerDefaultTimeInSeconds));
     }
 
-    eth_logs_tracker_.AddSubscriber(std::get<1>(gen_res),
-                                    std::move(get_filter_options()));
+    eth_logs_tracker_.AddSubscriber(std::get<1>(gen_res), std::move(*filter));
 
     std::move(callback).Run(std::move(id), base::Value(std::get<1>(gen_res)),
                             false, "", false);
@@ -1279,13 +1243,16 @@ void EthereumProviderImpl::CommonRequestOrSendAsync(base::ValueView input_value,
   } else if (method == kWeb3ClientVersion) {
     Web3ClientVersion(std::move(callback), std::move(id));
   } else if (method == kEthSubscribe) {
-    base::Value params;
-    if (!ParseEthSubscribeParams(normalized_json_request, &params)) {
+    std::string event_type;
+    base::Value::Dict filter;
+    if (!ParseEthSubscribeParams(normalized_json_request, &event_type,
+                                 &filter)) {
       SendErrorOnRequest(error, error_message, std::move(callback),
                          std::move(id));
       return;
     }
-    EthSubscribe(std::move(params), std::move(callback), std::move(id));
+    EthSubscribe(event_type, std::move(filter), std::move(callback),
+                 std::move(id));
   } else if (method == kEthUnsubscribe) {
     std::string subscription_id;
     if (!ParseEthUnsubscribeParams(normalized_json_request, &subscription_id)) {

diff --git a/components/brave_wallet/browser/ethereum_provider_impl.h b/components/brave_wallet/browser/ethereum_provider_impl.h
@@ -88,7 +88,8 @@ class EthereumProviderImpl final
                       RequestCallback callback,
                       base::Value id);
 
-  void EthSubscribe(const base::Value& params,
+  void EthSubscribe(const std::string& event_type,
+                    absl::optional<base::Value::Dict> filter,
                     RequestCallback callback,
                     base::Value id);
   void EthUnsubscribe(const std::string& subscription_id,

diff --git a/components/brave_wallet/browser/json_rpc_service.cc b/components/brave_wallet/browser/json_rpc_service.cc
@@ -2168,7 +2168,7 @@ void JsonRpcService::GetERC1155TokenBalance(
 }
 
 void JsonRpcService::EthGetLogs(const std::string& chain_id,
-                                const base::Value& params,
+                                base::Value::Dict filter_options,
                                 EthGetLogsCallback callback) {
   auto network_url = GetNetworkURL(prefs_, chain_id, mojom::CoinType::ETH);
   if (!network_url.is_valid()) {
@@ -2178,57 +2178,11 @@ void JsonRpcService::EthGetLogs(const std::string& chain_id,
     return;
   }
 
-  const auto load_params = [&](std::string& from_block, std::string& to_block,
-                               std::string& block_hash,
-                               base::Value::List* address,
-                               base::Value::List* topics) {
-    auto* filtering_opts = params.GetIfDict();
-    if (filtering_opts == nullptr) {
-      return;
-    }
-
-    const base::Value* from_block_val = filtering_opts->Find("fromBlock");
-    if (from_block_val != nullptr && from_block_val->is_string()) {
-      from_block = from_block_val->GetString();
-    }
-
-    const base::Value* to_block_val = filtering_opts->Find("toBlock");
-    if (to_block_val != nullptr && to_block_val->is_string()) {
-      to_block = *(to_block_val->GetIfString());
-    }
-
-    const base::Value* block_hash_val = filtering_opts->Find("blockHash");
-    if (block_hash_val != nullptr && block_hash_val->is_string()) {
-      block_hash = *(block_hash_val->GetIfString());
-    }
-
-    const auto* contract_addresses_str = filtering_opts->FindString("address");
-    if (contract_addresses_str != nullptr) {
-      address->Append(*contract_addresses_str);
-    }
-
-    const base::Value::List* contract_addresses =
-        filtering_opts->FindList("address");
-    if (contract_addresses != nullptr) {
-      *address = contract_addresses->Clone();
-    }
-
-    const base::Value::List* topics_list = filtering_opts->FindList("topics");
-    if (topics_list != nullptr) {
-      *topics = topics_list->Clone();
-    }
-  };
-
-  std::string from_block, to_block, block_hash;
-  base::Value::List contract_addresses, topics;
-  load_params(from_block, to_block, block_hash, &contract_addresses, &topics);
   auto internal_callback =
       base::BindOnce(&JsonRpcService::OnEthGetLogs,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback));
-  RequestInternal(
-      eth::eth_getLogs(from_block, to_block, std::move(contract_addresses),
-                       std::move(topics), block_hash),
-      true, network_url, std::move(internal_callback));
+  RequestInternal(eth::eth_getLogs(std::move(filter_options)), true,
+                  network_url, std::move(internal_callback));
 }
 
 void JsonRpcService::OnEthGetLogs(EthGetLogsCallback callback,

diff --git a/components/brave_wallet/browser/json_rpc_service.h b/components/brave_wallet/browser/json_rpc_service.h
@@ -330,7 +330,7 @@ class JsonRpcService : public KeyedService, public mojom::JsonRpcService {
                       GetEthTokenUriCallback callback);
 
   void EthGetLogs(const std::string& chain_id,
-                  const base::Value& params,
+                  base::Value::Dict filter_options,
                   EthGetLogsCallback callback);
 
   void OnEthGetLogs(EthGetLogsCallback callback,

diff --git a/components/brave_wallet/browser/json_rpc_service_unittest.cc b/components/brave_wallet/browser/json_rpc_service_unittest.cc
@@ -1166,7 +1166,7 @@ class JsonRpcServiceUnitTest : public testing::Test {
     params.Set("address", std::move(contract_addresses));
     params.Set("topics", std::move(topics));
     json_rpc_service_->EthGetLogs(
-        chain_id, base::Value(std::move(params)),
+        chain_id, std::move(params),
         base::BindLambdaForTesting(
             [&](const std::vector<Log>& logs, base::Value rawlogs,
                 mojom::ProviderError error, const std::string& error_message) {

diff --git a/components/brave_wallet/common/eth_request_helper.cc b/components/brave_wallet/common/eth_request_helper.cc
@@ -677,17 +677,31 @@ bool ParseEthSendRawTransactionParams(const std::string& json,
   return true;
 }
 
-bool ParseEthSubscribeParams(const std::string& json, base::Value* params) {
-  if (params == nullptr) {
+bool ParseEthSubscribeParams(const std::string& json,
+                             std::string* event_type,
+                             base::Value::Dict* filter) {
+  if (filter == nullptr) {
     return false;
   }
 
-  auto params_value = GetParamsList(json);
-  if (!params_value) {
+  auto list = GetParamsList(json);
+  if (!list || list->empty() || list->size() > 2) {
     return false;
   }
 
-  *params = base::Value(std::move(*params_value));
+  if (!(*list)[0].is_string()) {
+    return false;
+  }
+
+  *event_type = (*list)[0].GetString();
+
+  if (list->size() == 2) {
+    if (!(*list)[1].is_dict()) {
+      return false;
+    }
+
+    *filter = std::move((*list)[1].GetDict());
+  }
 
   return true;
 }

diff --git a/components/brave_wallet/common/eth_request_helper.h b/components/brave_wallet/common/eth_request_helper.h
@@ -74,7 +74,9 @@ bool ParseRequestPermissionsParams(
 
 bool ParseEthSendRawTransactionParams(const std::string& json,
                                       std::string* signed_transaction);
-bool ParseEthSubscribeParams(const std::string& json, base::Value* params);
+bool ParseEthSubscribeParams(const std::string& json,
+                             std::string* event_type,
+                             base::Value::Dict* filter);
 bool ParseEthUnsubscribeParams(const std::string& json,
                                std::string* subscription_id);