Return IDS_WALLET_INVALID_PARAMETERS if duplicate NFT IDs are passed …

…to JsonRpcService::GetNftMetadatas (#26171) * Return IDS_WALLET_INVALID_PARAMETERS if duplicate NFT IDs are passed to JsonRpcService::GetNftMetadatas * wip * Use base::StrCat * Format --------- Co-authored-by: Nick von Pentz <[email protected]>
brave · Oct 28, 2024 · 8fbee3c · 8fbee3c
1 parent 4408d6f
commit 8fbee3c
Show file tree

Hide file tree

Showing 4 changed files with 103 additions and 2 deletions.
diff --git a/components/brave_wallet/browser/json_rpc_service.cc b/components/brave_wallet/browser/json_rpc_service.cc
@@ -17,6 +17,7 @@
 #include "base/functional/bind.h"
 #include "base/no_destructor.h"
 #include "base/notreached.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "brave/components/brave_wallet/browser/blockchain_registry.h"
@@ -310,6 +311,18 @@ bool ValidateNftIdentifiers(
     return false;
   }
 
+  // Check for duplicates using a set to track unique identifiers
+  base::flat_set<std::string> seen_identifiers;
+  for (const auto& nft : nft_identifiers) {
+    // Create a unique string identifier combining contract, token id, and chain
+    std::string unique_id =
+        base::StrCat({nft->contract_address, nft->token_id, nft->chain_id});
+    if (!seen_identifiers.insert(unique_id).second) {
+      error_message = l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS);
+      return false;
+    }
+  }
+
   if (coin == mojom::CoinType::ETH) {
     for (auto& nft_identifier : nft_identifiers) {
       auto checksum_address =

diff --git a/components/brave_wallet/browser/json_rpc_service_unittest.cc b/components/brave_wallet/browser/json_rpc_service_unittest.cc
@@ -8061,6 +8061,25 @@ TEST_F(JsonRpcServiceUnitTest, GetNftMetadatas) {
                       l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
   nft_identifiers = std::vector<mojom::NftIdentifierPtr>();
 
+  // If there are duplicate NFTs it returns invalid params.
+  auto duplicate_nft1 = mojom::NftIdentifier::New();
+  duplicate_nft1->chain_id = mojom::kMainnetChainId;
+  duplicate_nft1->contract_address =
+      "0xed5af388653567af2f388e6224dc7c4b3241c544";
+  duplicate_nft1->token_id = "0xacf";  // "2767"
+  nft_identifiers.push_back(std::move(duplicate_nft1));
+
+  auto duplicate_nft2 = mojom::NftIdentifier::New();
+  duplicate_nft2->chain_id = mojom::kMainnetChainId;
+  duplicate_nft2->contract_address =
+      "0xed5af388653567af2f388e6224dc7c4b3241c544";
+  duplicate_nft2->token_id = "0xacf";  // "2767"
+  nft_identifiers.push_back(std::move(duplicate_nft2));
+
+  TestGetNftMetadatas(mojom::CoinType::ETH, std::move(nft_identifiers), {},
+                      l10n_util::GetStringUTF8(IDS_WALLET_INVALID_PARAMETERS));
+  nft_identifiers = std::vector<mojom::NftIdentifierPtr>();
+
   // If there are over 50 NFTs it returns invalid params.
   for (int i = 0; i < 51; i++) {
     auto nft_identifier = mojom::NftIdentifier::New();

diff --git a/components/brave_wallet/browser/simple_hash_client.cc b/components/brave_wallet/browser/simple_hash_client.cc
@@ -426,7 +426,7 @@ void SimpleHashClient::OnGetNftsForMetadatas(
   for (const auto& nft_identifier : nft_identifiers) {
     auto it = metadatas->find(nft_identifier);
     if (it != metadatas->end()) {
-      nft_metadatas.push_back(std::move(it->second));
+      nft_metadatas.push_back(it->second.Clone());
     }
   }
 

diff --git a/components/brave_wallet/browser/simple_hash_client_unittest.cc b/components/brave_wallet/browser/simple_hash_client_unittest.cc
@@ -1772,7 +1772,7 @@ TEST_F(SimpleHashClientUnitTest, GetNftMetadatas) {
               "value": "Red"
             },
             {
-              "trait_type": "Size",
+              "trait_type": "Size", 
               "value": "Small"
             }
           ]
@@ -1875,6 +1875,75 @@ TEST_F(SimpleHashClientUnitTest, GetNftMetadatas) {
   SetInterceptors(responses);
   TestGetNftMetadatas(mojom::CoinType::SOL, std::move(nft_identifiers),
                       expected_metadatas);
+
+  LOG(ERROR) << "BEFORE RELEVANT TESTS";
+  // Test case for duplicate NFT identifiers
+  nft_identifiers = std::vector<mojom::NftIdentifierPtr>();
+  expected_metadatas = std::vector<mojom::NftMetadataPtr>();
+
+  // Add two identical NFT identifiers
+  auto duplicate_nft_identifier1 = mojom::NftIdentifier::New();
+  duplicate_nft_identifier1->chain_id = mojom::kSolanaMainnet;
+  duplicate_nft_identifier1->contract_address =
+      "2iZBbRGnLVEEZH6JDsaNsTo66s2uxx7DTchVWKU8oisR";
+  duplicate_nft_identifier1->token_id = "";
+  nft_identifiers.push_back(std::move(duplicate_nft_identifier1));
+
+  auto duplicate_nft_identifier2 = mojom::NftIdentifier::New();
+  duplicate_nft_identifier2->chain_id = mojom::kSolanaMainnet;
+  duplicate_nft_identifier2->contract_address =
+      "2iZBbRGnLVEEZH6JDsaNsTo66s2uxx7DTchVWKU8oisR";
+  duplicate_nft_identifier2->token_id = "";
+  nft_identifiers.push_back(std::move(duplicate_nft_identifier2));
+
+  // Create JSON response for duplicate NFTs (response will contain only one
+  // entry)
+  std::string duplicate_json = R"({
+    "nfts": [
+      {
+        "nft_id": "solana.2iZBbRGnLVEEZH6JDsaNsTo66s2uxx7DTchVWKU8oisR",
+        "chain": "solana",
+        "contract_address": "2iZBbRGnLVEEZH6JDsaNsTo66s2uxx7DTchVWKU8oisR",
+        "token_id": null,
+        "name": "Common Water Warrior #19",
+        "description": "A true gladiator",
+        "image_url": "https://cdn.simplehash.com/assets/168e33bbf5276f717d8d190810ab93b4992ac8681054c1811f8248fe7636b54b.png",
+        "extra_metadata": {
+          "metadata_original_url": "https://nft.dragonwar.io/avatars/dragons/CWTWRDR_1.json"
+        }
+      }
+    ]
+  })";
+
+  // Set up expected metadata for the duplicate NFT
+  auto duplicate_metadata = mojom::NftMetadata::New();
+  duplicate_metadata->name = "Common Water Warrior #19";
+  duplicate_metadata->description = "A true gladiator";
+  duplicate_metadata->image =
+      "https://simplehash.wallet-cdn.brave.com/assets/"
+      "168e33bbf5276f717d8d190810ab93b4992ac8681054c1811f8248fe7636b54b.png";
+  duplicate_metadata->image_data = "";
+  duplicate_metadata->external_url = "";
+  duplicate_metadata->background_color = "";
+  duplicate_metadata->animation_url = "";
+  duplicate_metadata->youtube_url = "";
+
+  // Add the same metadata twice since we expect the API to return the same data
+  // for both requests
+  expected_metadatas.push_back(duplicate_metadata.Clone());
+  expected_metadatas.push_back(std::move(duplicate_metadata));
+
+  // Set up the response interceptor for the duplicate NFT request
+  responses.clear();
+  responses[GURL(
+      "https://simplehash.wallet.brave.com/api/v0/nfts/"
+      "assets?nft_ids=solana.2iZBbRGnLVEEZH6JDsaNsTo66s2uxx7DTchVWKU8oisR%"
+      "2Csolana.2iZBbRGnLVEEZH6JDsaNsTo66s2uxx7DTchVWKU8oisR")] =
+      duplicate_json;
+
+  SetInterceptors(responses);
+  TestGetNftMetadatas(mojom::CoinType::SOL, std::move(nft_identifiers),
+                      expected_metadatas);
 }
 
 TEST_F(SimpleHashClientUnitTest, GetNftBalances) {