From 7a2009fb5e020b0ffdacd32e4208e498127bdb50 Mon Sep 17 00:00:00 2001 From: oisupov Date: Fri, 24 Mar 2023 02:46:06 +0700 Subject: [PATCH 1/4] NFT pinning optimisation Resolves https://github.com/brave/brave-browser/issues/29230 --- .../browser/brave_wallet_pin_service.cc | 88 ++---- .../brave_wallet_pin_service_unittest.cc | 108 +++---- components/ipfs/pin/ipfs_local_pin_service.cc | 274 +++++++++++++----- components/ipfs/pin/ipfs_local_pin_service.h | 53 +++- .../pin/ipfs_local_pin_service_unittest.cc | 172 +++++++---- 5 files changed, 449 insertions(+), 246 deletions(-) diff --git a/components/brave_wallet/browser/brave_wallet_pin_service.cc b/components/brave_wallet/browser/brave_wallet_pin_service.cc index a6f3a0811364..93289a5a0fa6 100644 --- a/components/brave_wallet/browser/brave_wallet_pin_service.cc +++ b/components/brave_wallet/browser/brave_wallet_pin_service.cc @@ -30,7 +30,7 @@ const char kValidateTimestamp[] = "validate_timestamp"; const char kError[] = "error"; const char kErrorCode[] = "error_code"; const char kErrorMessage[] = "error_message"; -const char kAssetUrlListKey[] = "cids"; +const char kAssetUrlListKey[] = "urls"; namespace { // Solana NFTs don't have token_id @@ -106,28 +106,6 @@ absl::optional ExtractIpfsUrl(const std::string& url) { return source.value().spec(); } -absl::optional ExtractCID(const std::string& url) { - GURL gurl = GURL(ExtractIpfsUrl(url).value_or("")); - - if (!gurl.is_valid()) { - return absl::nullopt; - } - - std::vector result = base::SplitString( - gurl.path(), "/", base::WhitespaceHandling::KEEP_WHITESPACE, - base::SplitResult::SPLIT_WANT_NONEMPTY); - - if (result.size() == 0) { - return absl::nullopt; - } - - if (!ipfs::IsValidCID(result.at(0))) { - return absl::nullopt; - } - - return result.at(0); -} - } // namespace // static @@ -444,10 +422,10 @@ void BraveWalletPinService::Validate(mojom::BlockchainTokenPtr token, return; } - absl::optional> cids = + absl::optional> ipfs_urls = ResolvePinItems(service, token); - if (!cids) { + if (!ipfs_urls) { SetTokenStatus(service, std::move(token), mojom::TokenPinStatusCode::STATUS_PINNING_IN_PROGRESS, nullptr); @@ -466,7 +444,7 @@ void BraveWalletPinService::Validate(mojom::BlockchainTokenPtr token, if (!service) { local_pin_service_->ValidatePins( - path.value(), cids.value(), + path.value(), ipfs_urls.value(), base::BindOnce(&BraveWalletPinService::OnTokenValidated, weak_ptr_factory_.GetWeakPtr(), service, std::move(callback), std::move(token))); @@ -652,8 +630,8 @@ void BraveWalletPinService::ProcessTokenMetadata( const std::string& token_url, const std::string& result, AddPinCallback callback) { - auto metadata_cid = ExtractCID(token_url); - if (!metadata_cid) { + auto metadata_ipfs_url = ExtractIpfsUrl(token_url); + if (!metadata_ipfs_url) { auto pin_error = mojom::PinError::New( mojom::WalletPinServiceErrorCode::ERR_NON_IPFS_TOKEN_URL, "Metadata has non-ipfs url"); @@ -677,10 +655,10 @@ void BraveWalletPinService::ProcessTokenMetadata( } auto* image_url = parsed_result->FindStringKey("image"); - auto image_cid = - image_url != nullptr ? ExtractCID(*image_url) : absl::nullopt; + auto image_ipfs_url = + image_url != nullptr ? ExtractIpfsUrl(*image_url) : absl::nullopt; - if (!image_cid) { + if (!image_ipfs_url) { auto pin_error = mojom::PinError::New( mojom::WalletPinServiceErrorCode::ERR_NON_IPFS_TOKEN_URL, "Can't find proper image field"); @@ -690,32 +668,21 @@ void BraveWalletPinService::ProcessTokenMetadata( return; } - std::vector cids; - cids.push_back(metadata_cid.value()); - cids.push_back(image_cid.value()); - - auto ipfs_image_url = ExtractIpfsUrl(*image_url); - if (!ipfs_image_url) { - auto pin_error = mojom::PinError::New( - mojom::WalletPinServiceErrorCode::ERR_NON_IPFS_TOKEN_URL, - "Can't find proper image field"); - SetTokenStatus(service, token, - mojom::TokenPinStatusCode::STATUS_PINNING_FAILED, pin_error); - std::move(callback).Run(false, std::move(pin_error)); - return; - } + std::vector ipfs_urls; + ipfs_urls.push_back(metadata_ipfs_url.value()); + ipfs_urls.push_back(image_ipfs_url.value()); content_type_checker_->CheckContentTypeSupported( - ipfs_image_url.value(), + image_ipfs_url.value(), base::BindOnce(&BraveWalletPinService::OnContentTypeChecked, weak_ptr_factory_.GetWeakPtr(), service, token.Clone(), - std::move(cids), std::move(callback))); + std::move(ipfs_urls), std::move(callback))); } void BraveWalletPinService::OnContentTypeChecked( absl::optional service, mojom::BlockchainTokenPtr token, - std::vector cids, + std::vector ipfs_urls, AddPinCallback callback, absl::optional result) { if (!result.has_value()) { @@ -749,14 +716,14 @@ void BraveWalletPinService::OnContentTypeChecked( return; } - AddToken(service, token, cids); + AddToken(service, token, ipfs_urls); SetTokenStatus(service, token, mojom::TokenPinStatusCode::STATUS_PINNING_IN_PROGRESS, nullptr); if (!service) { local_pin_service_->AddPins( - path.value(), cids, + path.value(), ipfs_urls, base::BindOnce(&BraveWalletPinService::OnTokenPinned, weak_ptr_factory_.GetWeakPtr(), absl::nullopt, std::move(callback), std::move(token))); @@ -806,9 +773,10 @@ void BraveWalletPinService::OnTokenValidated( std::move(callback).Run(true, nullptr); } -bool BraveWalletPinService::AddToken(const absl::optional& service, - const mojom::BlockchainTokenPtr& token, - const std::vector& cids) { +bool BraveWalletPinService::AddToken( + const absl::optional& service, + const mojom::BlockchainTokenPtr& token, + const std::vector& ipfs_urls) { auto path = GetTokenPrefPath(service, token); if (!path) { return false; @@ -819,13 +787,13 @@ bool BraveWalletPinService::AddToken(const absl::optional& service, base::Value::Dict& update_dict = update.Get(); base::Value::Dict token_data; - base::Value::List cids_list; + base::Value::List ipfs_urls_list; - for (const auto& cid : cids) { - cids_list.Append(cid); + for (const auto& ipfs_url : ipfs_urls) { + ipfs_urls_list.Append(ipfs_url); } - token_data.Set(kAssetUrlListKey, std::move(cids_list)); + token_data.Set(kAssetUrlListKey, std::move(ipfs_urls_list)); token_data.Set( kAssetStatus, StatusToString(mojom::TokenPinStatusCode::STATUS_NOT_PINNED)); @@ -912,13 +880,13 @@ absl::optional> BraveWalletPinService::ResolvePinItems( return absl::nullopt; } - auto* cids = token_data_as_dict->FindList(kAssetUrlListKey); - if (!cids) { + auto* ipfs_urls = token_data_as_dict->FindList(kAssetUrlListKey); + if (!ipfs_urls) { return absl::nullopt; } std::vector result; - for (const base::Value& item : *cids) { + for (const base::Value& item : *ipfs_urls) { result.push_back(item.GetString()); } diff --git a/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc b/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc index 0db747880c0a..b15ce301851e 100644 --- a/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc +++ b/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc @@ -262,8 +262,8 @@ TEST_F(BraveWalletPinServiceTest, AddSolPin) { [](const std::string& prefix, const std::vector& cids, ipfs::AddPinCallback callback) { EXPECT_EQ(kSolMonkey1Path, prefix); - EXPECT_EQ("bafy1", cids.at(0)); - EXPECT_EQ("bafy2", cids.at(1)); + EXPECT_EQ("ipfs://bafy1", cids.at(0)); + EXPECT_EQ("ipfs://bafy2/1.png", cids.at(1)); std::move(callback).Run(true); })); @@ -288,14 +288,14 @@ TEST_F(BraveWalletPinServiceTest, AddSolPin) { .FindDictByDottedPath(kSolMonkey1Path); base::Value::List expected_cids; - expected_cids.Append("bafy1"); - expected_cids.Append("bafy2"); + expected_cids.Append("ipfs://bafy1"); + expected_cids.Append("ipfs://bafy2/1.png"); EXPECT_EQ(BraveWalletPinService::StatusToString( mojom::TokenPinStatusCode::STATUS_PINNED), *(token_record->FindString("status"))); EXPECT_EQ(nullptr, token_record->FindDict("error")); - EXPECT_EQ(expected_cids, *(token_record->FindList("cids"))); + EXPECT_EQ(expected_cids, *(token_record->FindList("urls"))); EXPECT_EQ(base::Time::FromTimeT(123u), base::ValueToTime(token_record->Find("validate_timestamp"))); } @@ -368,9 +368,10 @@ TEST_F(BraveWalletPinServiceTest, AddPin) { [](const std::string& prefix, const std::vector& cids, ipfs::AddPinCallback callback) { EXPECT_EQ(kMonkey1Path, prefix); - EXPECT_EQ("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq", - cids.at(0)); - EXPECT_EQ("bafy1", cids.at(1)); + EXPECT_EQ( + "ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq/2413", + cids.at(0)); + EXPECT_EQ("ipfs://bafy1", cids.at(1)); std::move(callback).Run(true); })); @@ -395,14 +396,15 @@ TEST_F(BraveWalletPinServiceTest, AddPin) { .FindDictByDottedPath(kMonkey1Path); base::Value::List expected_cids; - expected_cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - expected_cids.Append("bafy1"); + expected_cids.Append( + "ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq/2413"); + expected_cids.Append("ipfs://bafy1"); EXPECT_EQ(BraveWalletPinService::StatusToString( mojom::TokenPinStatusCode::STATUS_PINNED), *(token_record->FindString("status"))); EXPECT_EQ(nullptr, token_record->FindDict("error")); - EXPECT_EQ(expected_cids, *(token_record->FindList("cids"))); + EXPECT_EQ(expected_cids, *(token_record->FindList("urls"))); EXPECT_EQ(base::Time::FromTimeT(123u), base::ValueToTime(token_record->Find("validate_timestamp"))); } @@ -477,9 +479,10 @@ TEST_F(BraveWalletPinServiceTest, AddPin_GatewayUrl) { [](const std::string& prefix, const std::vector& cids, ipfs::AddPinCallback callback) { EXPECT_EQ(kMonkey5Path, prefix); - EXPECT_EQ("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq", - cids.at(0)); - EXPECT_EQ("bafy3", cids.at(1)); + EXPECT_EQ( + "ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq/2888", + cids.at(0)); + EXPECT_EQ("ipfs://bafy3", cids.at(1)); std::move(callback).Run(true); })); @@ -504,14 +507,15 @@ TEST_F(BraveWalletPinServiceTest, AddPin_GatewayUrl) { .FindDictByDottedPath(kMonkey5Path); base::Value::List expected_cids; - expected_cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - expected_cids.Append("bafy3"); + expected_cids.Append( + "ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq/2888"); + expected_cids.Append("ipfs://bafy3"); EXPECT_EQ(BraveWalletPinService::StatusToString( mojom::TokenPinStatusCode::STATUS_PINNED), *(token_record->FindString("status"))); EXPECT_EQ(nullptr, token_record->FindDict("error")); - EXPECT_EQ(expected_cids, *(token_record->FindList("cids"))); + EXPECT_EQ(expected_cids, *(token_record->FindList("urls"))); EXPECT_EQ(base::Time::FromTimeT(123u), base::ValueToTime(token_record->Find("validate_timestamp"))); } @@ -585,9 +589,10 @@ TEST_F(BraveWalletPinServiceTest, AddPin_ContentVerification) { [](const std::string& prefix, const std::vector& cids, ipfs::AddPinCallback callback) { EXPECT_EQ(kMonkey1Path, prefix); - EXPECT_EQ("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq", - cids.at(0)); - EXPECT_EQ("bafy1", cids.at(1)); + EXPECT_EQ( + "ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq/2413", + cids.at(0)); + EXPECT_EQ("ipfs://bafy1", cids.at(1)); std::move(callback).Run(true); })); @@ -612,14 +617,15 @@ TEST_F(BraveWalletPinServiceTest, AddPin_ContentVerification) { .FindDictByDottedPath(kMonkey1Path); base::Value::List expected_cids; - expected_cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - expected_cids.Append("bafy1"); + expected_cids.Append( + "ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq/2413"); + expected_cids.Append("ipfs://bafy1"); EXPECT_EQ(BraveWalletPinService::StatusToString( mojom::TokenPinStatusCode::STATUS_PINNED), *(token_record->FindString("status"))); EXPECT_EQ(nullptr, token_record->FindDict("error")); - EXPECT_EQ(expected_cids, *(token_record->FindList("cids"))); + EXPECT_EQ(expected_cids, *(token_record->FindList("urls"))); EXPECT_EQ(base::Time::FromTimeT(123u), base::ValueToTime(token_record->Find("validate_timestamp"))); } @@ -649,9 +655,9 @@ TEST_F(BraveWalletPinServiceTest, AddPin_ContentVerification) { [](const std::string& prefix, const std::vector& cids, ipfs::AddPinCallback callback) { EXPECT_EQ(kMonkey1Path, prefix); - EXPECT_EQ("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq", + EXPECT_EQ("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq", cids.at(0)); - EXPECT_EQ("bafy2", cids.at(1)); + EXPECT_EQ("ipfs://bafy2", cids.at(1)); std::move(callback).Run(true); })); @@ -702,9 +708,9 @@ TEST_F(BraveWalletPinServiceTest, AddPin_ContentVerification) { [](const std::string& prefix, const std::vector& cids, ipfs::AddPinCallback callback) { EXPECT_EQ(kMonkey3Path, prefix); - EXPECT_EQ("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq", + EXPECT_EQ("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq", cids.at(0)); - EXPECT_EQ("bafy3", cids.at(1)); + EXPECT_EQ("ipfs://bafy3", cids.at(1)); std::move(callback).Run(true); })); @@ -775,8 +781,8 @@ TEST_F(BraveWalletPinServiceTest, RemoveSolPin) { item.Set("status", "pinned"); item.Set("validation_timestamp", "123"); base::Value::List cids; - cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - cids.Append("Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); + cids.Append("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); + cids.Append("ipfs://Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); update_dict.SetByDottedPath(kSolMonkey1Path, std::move(item)); } @@ -842,8 +848,8 @@ TEST_F(BraveWalletPinServiceTest, RemovePin) { item.Set("status", "pinned"); item.Set("validation_timestamp", "123"); base::Value::List cids; - cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - cids.Append("bafy1"); + cids.Append("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); + cids.Append("ipfs://bafy1"); update_dict.SetByDottedPath(kMonkey1Path, std::move(item)); } @@ -909,9 +915,9 @@ TEST_F(BraveWalletPinServiceTest, ValidatePin) { item.Set("status", "pinned"); item.Set("validate_timestamp", base::TimeToValue(base::Time::Now())); base::Value::List cids; - cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - cids.Append("Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); - item.Set("cids", std::move(cids)); + cids.Append("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); + cids.Append("ipfs://Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); + item.Set("urls", std::move(cids)); update_dict.SetByDottedPath(kMonkey1Path, std::move(item)); } @@ -926,9 +932,9 @@ TEST_F(BraveWalletPinServiceTest, ValidatePin) { .WillByDefault(::testing::Invoke( [](const std::string& prefix, const std::vector& cids, ipfs::ValidatePinsCallback callback) { - EXPECT_EQ("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq", + EXPECT_EQ("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq", cids.at(0)); - EXPECT_EQ("Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg", + EXPECT_EQ("ipfs://Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg", cids.at(1)); EXPECT_EQ(kMonkey1Path, prefix); std::move(callback).Run(true); @@ -1020,9 +1026,9 @@ TEST_F(BraveWalletPinServiceTest, GetTokenStatus) { item.Set("validate_timestamp", base::TimeToValue(base::Time::FromTimeT(123u))); base::Value::List cids; - cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - cids.Append("Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); - item.Set("cids", std::move(cids)); + cids.Append("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); + cids.Append("ipfs://Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); + item.Set("urls", std::move(cids)); update_dict.SetByDottedPath(kSolMonkey1Path, std::move(item)); } @@ -1036,9 +1042,9 @@ TEST_F(BraveWalletPinServiceTest, GetTokenStatus) { item.Set("validate_timestamp", base::TimeToValue(base::Time::FromTimeT(123u))); base::Value::List cids; - cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - cids.Append("Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); - item.Set("cids", std::move(cids)); + cids.Append("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); + cids.Append("ipfs://Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); + item.Set("urls", std::move(cids)); update_dict.SetByDottedPath(kMonkey1Path, std::move(item)); } @@ -1113,9 +1119,9 @@ TEST_F(BraveWalletPinServiceTest, GetLastValidateTime) { item.Set("validate_timestamp", base::TimeToValue(base::Time::FromTimeT(123u))); base::Value::List cids; - cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - cids.Append("Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); - item.Set("cids", std::move(cids)); + cids.Append("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); + cids.Append("ipfs://Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); + item.Set("urls", std::move(cids)); update_dict.SetByDottedPath(kMonkey1Path, std::move(item)); } @@ -1277,9 +1283,9 @@ TEST_F(BraveWalletPinServiceTest, Reset) { item.Set("validate_timestamp", base::TimeToValue(base::Time::FromTimeT(123u))); base::Value::List cids; - cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - cids.Append("Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); - item.Set("cids", std::move(cids)); + cids.Append("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); + cids.Append("ipfs://Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); + item.Set("urls", std::move(cids)); update_dict.SetByDottedPath(kMonkey1Path, std::move(item)); } @@ -1306,9 +1312,9 @@ TEST_F(BraveWalletPinServiceTest, Reset_Failed) { item.Set("validate_timestamp", base::TimeToValue(base::Time::FromTimeT(123u))); base::Value::List cids; - cids.Append("QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); - cids.Append("Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); - item.Set("cids", std::move(cids)); + cids.Append("ipfs://QmeSjSinHpPnmXmspMjwiXyN6zS4E9zccariGR3jxcaWtq"); + cids.Append("ipfs://Qmcyc7tm9sZB9JnvLgejPTwdzjjNjDMiRWCUvaZAfp6cUg"); + item.Set("urls", std::move(cids)); update_dict.SetByDottedPath(kMonkey1Path, std::move(item)); } diff --git a/components/ipfs/pin/ipfs_local_pin_service.cc b/components/ipfs/pin/ipfs_local_pin_service.cc index 981e5fd900be..de2219a15c7f 100644 --- a/components/ipfs/pin/ipfs_local_pin_service.cc +++ b/components/ipfs/pin/ipfs_local_pin_service.cc @@ -11,61 +11,161 @@ #include #include "base/containers/contains.h" +#include "base/functional/callback.h" +#include "base/strings/strcat.h" #include "base/strings/string_split.h" +#include "base/strings/string_util.h" #include "brave/components/ipfs/pref_names.h" #include "components/prefs/scoped_user_pref_update.h" namespace ipfs { +bool PinData::operator==(const PinData& d) const { + return this->cid == d.cid && this->pinning_mode == d.pinning_mode; +} +bool PinData::operator==(const PinData& d) { + return this->cid == d.cid && this->pinning_mode == d.pinning_mode; +} + namespace { const char kRecursiveMode[] = "recursive"; +const char kDirectMode[] = "direct"; + +std::string GetPrefNameFromPinningMode(PinningMode mode) { + switch (mode) { + case DIRECT: + return kDirectMode; + case RECURSIVE: + return kRecursiveMode; + } + NOTREACHED(); + return kRecursiveMode; +} + } // namespace +// Splits ipfs:// url to a list of PinData items +absl::optional> IpfsLocalPinService::ExtractPinData( + const std::string& ipfs_url) { + auto gurl = GURL(ipfs_url); + if (!gurl.SchemeIs(ipfs::kIPFSScheme)) { + return absl::nullopt; + } + // This will just remove ipfs:// scheme + auto path = gurl.path(); + std::vector splitted = + base::SplitString(path, "/", base::WhitespaceHandling::KEEP_WHITESPACE, + base::SplitResult::SPLIT_WANT_NONEMPTY); + if (splitted.empty()) { + return absl::nullopt; + } + std::vector result; + size_t counter = 0; + std::string builder = "/ipfs"; + for (const auto& part : splitted) { + builder += "/"; + builder += part; + bool recursive = ++counter == splitted.size(); + result.push_back(PinData{ + builder, recursive ? PinningMode::RECURSIVE : PinningMode::DIRECT}); + } + return result; +} + +absl::optional> +IpfsLocalPinService::ExtractMergedPinData(std::vector ipfs_urls) { + std::vector result; + for (const auto& ipfs_url : ipfs_urls) { + auto pins_data_for_url = ExtractPinData(ipfs_url); + if (!pins_data_for_url) { + return absl::nullopt; + } + for (const auto& item : pins_data_for_url.value()) { + if (std::find(result.begin(), result.end(), item) == result.end()) { + result.push_back(item); + } + } + } + return result; +} + AddLocalPinJob::AddLocalPinJob(PrefService* prefs_service, IpfsService* ipfs_service, const std::string& key, - const std::vector& cids, + const std::vector& pins_data, AddPinCallback callback) : prefs_service_(prefs_service), ipfs_service_(ipfs_service), key_(key), - cids_(cids), + pins_data_(pins_data), callback_(std::move(callback)) {} AddLocalPinJob::~AddLocalPinJob() = default; void AddLocalPinJob::Start() { - ipfs_service_->AddPin(cids_, true, - base::BindOnce(&AddLocalPinJob::OnAddPinResult, - weak_ptr_factory_.GetWeakPtr())); + auto callback = base::BarrierCallback( + 2, base::BindOnce(&AddLocalPinJob::OnAddPinResult, + weak_ptr_factory_.GetWeakPtr())); + + std::vector recursive_cids; + std::vector direct_cids; + + for (const auto& pin_data : pins_data_) { + if (pin_data.pinning_mode == PinningMode::RECURSIVE) { + recursive_cids.push_back(pin_data.cid); + } else { + direct_cids.push_back(pin_data.cid); + } + } + + ipfs_service_->AddPin( + recursive_cids, true, + base::BindOnce(&AddLocalPinJob::Accumulate, + weak_ptr_factory_.GetWeakPtr(), PinningMode::RECURSIVE, + recursive_cids, callback)); + ipfs_service_->AddPin( + direct_cids, false, + base::BindOnce(&AddLocalPinJob::Accumulate, + weak_ptr_factory_.GetWeakPtr(), PinningMode::DIRECT, + direct_cids, callback)); +} + +void AddLocalPinJob::Accumulate( + PinningMode pinning_mode, + std::vector cids, + base::OnceCallback callback, + absl::optional result) { + if (!result || result->pins.size() != cids.size()) { + pinning_failed_ = true; + } + + std::move(callback).Run(std::pair>( + pinning_mode, result)); } -void AddLocalPinJob::OnAddPinResult(absl::optional result) { +void AddLocalPinJob::OnAddPinResult(std::vector result) { if (is_canceled_) { std::move(callback_).Run(false); return; } - if (!result) { + if (pinning_failed_) { std::move(callback_).Run(false); return; } - for (const auto& cid : cids_) { - if (!base::Contains(result->pins, cid)) { - std::move(callback_).Run(false); - return; - } - } - { ScopedDictPrefUpdate update(prefs_service_, kIPFSPinnedCids); base::Value::Dict& update_dict = update.Get(); - for (const auto& cid : cids_) { - base::Value::List* list = update_dict.EnsureList(cid); - list->EraseValue(base::Value(key_)); - list->Append(base::Value(key_)); + for (const auto& mode : result) { + auto* mode_dict = + update_dict.EnsureDict(GetPrefNameFromPinningMode(mode.first)); + for (const auto& cid : mode.second.value().pins) { + base::Value::List* list = mode_dict->EnsureList(cid); + list->EraseValue(base::Value(key_)); + list->Append(base::Value(key_)); + } } } std::move(callback_).Run(true); @@ -83,20 +183,29 @@ RemoveLocalPinJob::~RemoveLocalPinJob() = default; void RemoveLocalPinJob::Start() { { ScopedDictPrefUpdate update(prefs_service_, kIPFSPinnedCids); - base::Value::Dict& update_dict = update.Get(); + base::Value::Dict& pinning_modes_dict = update.Get(); + // Iterate over pinning modes + for (const auto pair : pinning_modes_dict) { + auto* cids_dict = pair.second.GetIfDict(); + if (!cids_dict) { + NOTREACHED() << "Corrupted prefs structure."; + continue; + } - std::vector remove_list; - for (auto pair : update_dict) { - base::Value::List* list = pair.second.GetIfList(); - if (list) { - list->EraseValue(base::Value(key_)); - if (list->empty()) { - remove_list.push_back(pair.first); + std::vector remove_list; + // Iterate over CIDs + for (const auto cid : *cids_dict) { + base::Value::List* list = cid.second.GetIfList(); + if (list) { + list->EraseValue(base::Value(key_)); + if (list->empty()) { + remove_list.push_back(cid.first); + } } } - } - for (const auto& cid : remove_list) { - update_dict.Remove(cid); + for (const auto& cid : remove_list) { + cids_dict->Remove(cid); + } } } std::move(callback_).Run(true); @@ -105,18 +214,23 @@ void RemoveLocalPinJob::Start() { VerifyLocalPinJob::VerifyLocalPinJob(PrefService* prefs_service, IpfsService* ipfs_service, const std::string& key, - const std::vector& cids, + const std::vector& pins_data, ValidatePinsCallback callback) : prefs_service_(prefs_service), ipfs_service_(ipfs_service), key_(key), - cids_(cids), + pins_data_(pins_data), callback_(std::move(callback)) {} VerifyLocalPinJob::~VerifyLocalPinJob() = default; void VerifyLocalPinJob::Start() { - ipfs_service_->GetPins(absl::nullopt, kRecursiveMode, true, + std::vector cids; + for (const auto& pin_data : pins_data_) { + cids.push_back(pin_data.cid); + } + + ipfs_service_->GetPins(cids, "all", true, base::BindOnce(&VerifyLocalPinJob::OnGetPinsResult, weak_ptr_factory_.GetWeakPtr())); } @@ -131,28 +245,8 @@ void VerifyLocalPinJob::OnGetPinsResult(absl::optional result) { std::move(callback_).Run(absl::nullopt); return; } - ScopedDictPrefUpdate update(prefs_service_, kIPFSPinnedCids); - base::Value::Dict& update_dict = update.Get(); - - bool verification_passed = true; - for (const auto& cid : cids_) { - base::Value::List* list = update_dict.FindList(cid); - if (!list) { - verification_passed = false; - } else { - if (result->find(cid) != result->end()) { - list->EraseValue(base::Value(key_)); - list->Append(base::Value(key_)); - } else { - verification_passed = false; - list->EraseValue(base::Value(key_)); - } - if (list->empty()) { - update_dict.Remove(cid); - } - } - } - std::move(callback_).Run(verification_passed); + + std::move(callback_).Run(result->size() == pins_data_.size()); } GcJob::GcJob(PrefService* prefs_service, @@ -165,27 +259,56 @@ GcJob::GcJob(PrefService* prefs_service, GcJob::~GcJob() = default; void GcJob::Start() { - ipfs_service_->GetPins( - absl::nullopt, kRecursiveMode, true, + auto callback = base::BarrierCallback( + 2, base::BindOnce(&GcJob::OnGetPinsResult, weak_ptr_factory_.GetWeakPtr())); + ipfs_service_->GetPins( + absl::nullopt, GetPrefNameFromPinningMode(PinningMode::RECURSIVE), true, + base::BindOnce(&GcJob::Accumulate, weak_ptr_factory_.GetWeakPtr(), + PinningMode::RECURSIVE, callback)); + ipfs_service_->GetPins( + absl::nullopt, GetPrefNameFromPinningMode(PinningMode::DIRECT), true, + base::BindOnce(&GcJob::Accumulate, weak_ptr_factory_.GetWeakPtr(), + PinningMode::DIRECT, callback)); +} + +void GcJob::Accumulate(PinningMode pinning_mode, + base::OnceCallback callback, + absl::optional result) { + if (!result) { + gc_job_failed_ = true; + } + + std::move(callback).Run(std::pair>( + pinning_mode, result)); } -void GcJob::OnGetPinsResult(absl::optional result) { +void GcJob::OnGetPinsResult(std::vector result) { if (is_canceled_) { std::move(callback_).Run(false); return; } - if (!result) { + if (gc_job_failed_) { std::move(callback_).Run(false); return; } + std::vector cids_to_delete; - const base::Value::Dict& dict = prefs_service_->GetDict(kIPFSPinnedCids); - for (const auto& pair : result.value()) { - const base::Value::List* list = dict.FindList(pair.first); - if (!list || list->empty()) { - cids_to_delete.push_back(pair.first); + const base::Value::Dict& pinning_modes_dict = + prefs_service_->GetDict(kIPFSPinnedCids); + // Check both recursive and direct mode dictionaries. If there is no CID in + // both, then unpin. + for (const auto& modes : result) { + for (const auto& cid : modes.second.value()) { + if (!pinning_modes_dict.FindListByDottedPath( + base::StrCat({GetPrefNameFromPinningMode(PinningMode::RECURSIVE), + ".", cid.first})) && + !pinning_modes_dict.FindListByDottedPath( + base::StrCat({GetPrefNameFromPinningMode(PinningMode::DIRECT), + ".", cid.first}))) { + cids_to_delete.push_back(cid.first); + } } } @@ -263,10 +386,16 @@ void IpfsLocalPinService::SetIpfsBasePinServiceForTesting( IpfsLocalPinService::~IpfsLocalPinService() = default; void IpfsLocalPinService::AddPins(const std::string& key, - const std::vector& cids, + const std::vector& ipfs_urls, AddPinCallback callback) { + auto pins_data = ExtractMergedPinData(ipfs_urls); + if (!pins_data) { + NOTREACHED(); + std::move(callback).Run(false); + return; + } ipfs_base_pin_service_->AddJob(std::make_unique( - prefs_service_, ipfs_service_, key, cids, + prefs_service_, ipfs_service_, key, pins_data.value(), base::BindOnce(&IpfsLocalPinService::OnAddJobFinished, weak_ptr_factory_.GetWeakPtr(), std::move(callback)))); } @@ -279,11 +408,18 @@ void IpfsLocalPinService::RemovePins(const std::string& key, weak_ptr_factory_.GetWeakPtr(), std::move(callback)))); } -void IpfsLocalPinService::ValidatePins(const std::string& key, - const std::vector& cids, - ValidatePinsCallback callback) { +void IpfsLocalPinService::ValidatePins( + const std::string& key, + const std::vector& ipfs_urls, + ValidatePinsCallback callback) { + auto pins_data = ExtractMergedPinData(ipfs_urls); + if (!pins_data) { + NOTREACHED(); + std::move(callback).Run(absl::nullopt); + return; + } ipfs_base_pin_service_->AddJob(std::make_unique( - prefs_service_, ipfs_service_, key, cids, + prefs_service_, ipfs_service_, key, pins_data.value(), base::BindOnce(&IpfsLocalPinService::OnValidateJobFinished, weak_ptr_factory_.GetWeakPtr(), std::move(callback)))); } diff --git a/components/ipfs/pin/ipfs_local_pin_service.h b/components/ipfs/pin/ipfs_local_pin_service.h index 429d7aace302..0696b22335d5 100644 --- a/components/ipfs/pin/ipfs_local_pin_service.h +++ b/components/ipfs/pin/ipfs_local_pin_service.h @@ -8,8 +8,11 @@ #include #include +#include #include +#include "base/barrier_callback.h" +#include "base/functional/callback.h" #include "brave/components/ipfs/ipfs_service.h" #include "brave/components/ipfs/pin/ipfs_base_pin_service.h" #include "third_party/abseil-cpp/absl/types/optional.h" @@ -18,6 +21,19 @@ using ipfs::IpfsService; namespace ipfs { +enum PinningMode { DIRECT = 0, RECURSIVE = 1 }; + +struct PinData { + std::string cid; + PinningMode pinning_mode; + + bool operator==(const PinData&) const; + bool operator==(const PinData&); +}; + +absl::optional> ExtractPinData( + const std::string& ipfs_url); + using AddPinCallback = base::OnceCallback; using RemovePinCallback = base::OnceCallback; using ValidatePinsCallback = base::OnceCallback)>; @@ -45,20 +61,29 @@ class AddLocalPinJob : public IpfsBaseJob { AddLocalPinJob(PrefService* prefs_service, IpfsService* ipfs_service, const std::string& key, - const std::vector& cids, + const std::vector& pins_data, AddPinCallback callback); ~AddLocalPinJob() override; void Start() override; private: - void OnAddPinResult(absl::optional result); + using AccumulatedResult = + std::pair>; + void Accumulate(PinningMode pinning_mode, + std::vector cids, + base::OnceCallback callback, + absl::optional result); + void OnAddPinResult(std::vector result); raw_ptr prefs_service_; raw_ptr ipfs_service_; std::string key_; - std::vector cids_; + std::vector pins_data_; AddPinCallback callback_; + + bool pinning_failed_ = false; + base::WeakPtrFactory weak_ptr_factory_{this}; }; @@ -85,7 +110,7 @@ class VerifyLocalPinJob : public IpfsBaseJob { VerifyLocalPinJob(PrefService* prefs_service, IpfsService* ipfs_service, const std::string& key, - const std::vector& cids, + const std::vector& PinData, ValidatePinsCallback callback); ~VerifyLocalPinJob() override; @@ -97,7 +122,7 @@ class VerifyLocalPinJob : public IpfsBaseJob { raw_ptr prefs_service_; raw_ptr ipfs_service_; std::string key_; - std::vector cids_; + std::vector pins_data_; ValidatePinsCallback callback_; base::WeakPtrFactory weak_ptr_factory_{this}; }; @@ -113,12 +138,19 @@ class GcJob : public IpfsBaseJob { void Start() override; private: - void OnGetPinsResult(absl::optional result); + using AccumulatedResult = + std::pair>; + void Accumulate(PinningMode pinning_mode, + base::OnceCallback callback, + absl::optional result); + void OnGetPinsResult(std::vector result); void OnPinsRemovedResult(absl::optional result); raw_ptr prefs_service_; raw_ptr ipfs_service_; GcCallback callback_; + bool gc_job_failed_ = false; + base::WeakPtrFactory weak_ptr_factory_{this}; }; @@ -133,18 +165,23 @@ class IpfsLocalPinService : public KeyedService { // Pins provided cids and stores related record in the prefs. virtual void AddPins(const std::string& key, - const std::vector& cids, + const std::vector& ipfs_urls, AddPinCallback callback); // Unpins all cids related to the key. virtual void RemovePins(const std::string& key, RemovePinCallback callback); // Checks that all cids related to the key are pinned. virtual void ValidatePins(const std::string& key, - const std::vector& cids, + const std::vector& ipfs_urls, ValidatePinsCallback callback); void ScheduleGcTask(); void SetIpfsBasePinServiceForTesting(std::unique_ptr); + static absl::optional> ExtractPinData( + const std::string& ipfs_url); + static absl::optional> ExtractMergedPinData( + std::vector ipfs_urls); + private: void OnLsPinCliResult(base::OnceCallback callback, absl::optional result); diff --git a/components/ipfs/pin/ipfs_local_pin_service_unittest.cc b/components/ipfs/pin/ipfs_local_pin_service_unittest.cc index b5c6308e780b..c8d31d56093a 100644 --- a/components/ipfs/pin/ipfs_local_pin_service_unittest.cc +++ b/components/ipfs/pin/ipfs_local_pin_service_unittest.cc @@ -93,20 +93,29 @@ TEST_F(IpfsLocalPinServiceTest, AddLocalPinJobTest) { [](const std::vector& cids, bool recursive, IpfsService::AddPinCallback callback) { AddPinResult result; - result.pins = cids; + if (recursive) { + result.pins = {"Qjson", "Qimage"}; + } else { + result.pins = {"Qma", "Qmb", "QmaD1", "QmbD1"}; + } std::move(callback).Run(result); })); absl::optional success; - service()->AddPins("a", {"Qma", "Qmb", "Qmc"}, - base::BindLambdaForTesting( - [&success](bool result) { success = result; })); + service()->AddPins( + "a", {"ipfs://Qma/metadata/1.json", "ipfs://Qmb/images/1.jpg"}, + base::BindLambdaForTesting( + [&success](bool result) { success = result; })); - std::string expected = R"({ + std::string expected = R"({"recursive": { + "Qjson" : ["a"], + "Qimage" : ["a"] + }, "direct": { "Qma" : ["a"], "Qmb" : ["a"], - "Qmc" : ["a"] - })"; + "QmaD1" : ["a"], + "QmbD1" : ["a"] + }})"; absl::optional expected_value = base::JSONReader::Read( expected, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); @@ -120,21 +129,32 @@ TEST_F(IpfsLocalPinServiceTest, AddLocalPinJobTest) { [](const std::vector& cids, bool recursive, IpfsService::AddPinCallback callback) { AddPinResult result; - result.pins = cids; + if (recursive) { + result.pins = {"Qma", "Qmb", "Qmc", "Qmd", "Qimage"}; + } std::move(callback).Run(result); })); absl::optional success; - service()->AddPins("b", {"Qma", "Qmb", "Qmc", "Qmd"}, + service()->AddPins("b", + {"ipfs://Qma", "ipfs://Qmb", "ipfs://Qmc", "ipfs://Qmd", + "ipfs://Qimage"}, base::BindLambdaForTesting( [&success](bool result) { success = result; })); - std::string expected = R"({ - "Qma" : ["a", "b"], - "Qmb" : ["a", "b"], - "Qmc" : ["a", "b"], - "Qmd" : ["b"] - })"; + std::string expected = R"({"recursive": { + "Qjson" : ["a"], + "Qimage" : ["a", "b"], + "Qma": ["b"], + "Qmb": ["b"], + "Qmc": ["b"], + "Qmd": ["b"] + }, "direct": { + "Qma" : ["a"], + "Qmb" : ["a"], + "QmaD1" : ["a"], + "QmbD1" : ["a"] + }})"; absl::optional expected_value = base::JSONReader::Read( expected, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); @@ -142,27 +162,39 @@ TEST_F(IpfsLocalPinServiceTest, AddLocalPinJobTest) { EXPECT_TRUE(success.value()); } + // Not all CIDS are pinned - pinning for "c" fails { ON_CALL(*GetIpfsService(), AddPin(_, _, _)) .WillByDefault(::testing::Invoke( [](const std::vector& cids, bool recursive, IpfsService::AddPinCallback callback) { AddPinResult result; - result.pins = {"Qma", "Qmb", "Qmc"}; + if (recursive) { + result.pins = {"Qma", "Qmb", "Qmc"}; + } std::move(callback).Run(result); })); absl::optional success; - service()->AddPins("c", {"Qma", "Qmb", "Qmc", "Qmd", "Qme"}, - base::BindLambdaForTesting( - [&success](bool result) { success = result; })); - - std::string expected = R"({ - "Qma" : ["a", "b"], - "Qmb" : ["a", "b"], - "Qmc" : ["a", "b"], - "Qmd" : ["b"] - })"; + service()->AddPins( + "c", + {"ipfs://Qma", "ipfs://Qmb", "ipfs://Qmc", "ipfs://Qmd", "ipfs://Qme"}, + base::BindLambdaForTesting( + [&success](bool result) { success = result; })); + + std::string expected = R"({"recursive": { + "Qjson" : ["a"], + "Qimage" : ["a", "b"], + "Qma": ["b"], + "Qmb": ["b"], + "Qmc": ["b"], + "Qmd": ["b"] + }, "direct": { + "Qma" : ["a"], + "Qmb" : ["a"], + "QmaD1" : ["a"], + "QmbD1" : ["a"] + }})"; absl::optional expected_value = base::JSONReader::Read( expected, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); @@ -170,6 +202,7 @@ TEST_F(IpfsLocalPinServiceTest, AddLocalPinJobTest) { EXPECT_FALSE(success.value()); } + // Nothing is pinned for "c" { ON_CALL(*GetIpfsService(), AddPin(_, _, _)) .WillByDefault(::testing::Invoke( @@ -179,16 +212,25 @@ TEST_F(IpfsLocalPinServiceTest, AddLocalPinJobTest) { })); absl::optional success; - service()->AddPins("c", {"Qma", "Qmb", "Qmc", "Qmd", "Qme"}, - base::BindLambdaForTesting( - [&success](bool result) { success = result; })); - - std::string expected = R"({ - "Qma" : ["a", "b"], - "Qmb" : ["a", "b"], - "Qmc" : ["a", "b"], - "Qmd" : ["b"] - })"; + service()->AddPins( + "c", + {"ipfs://Qma", "ipfs://Qmb", "ipfs://Qmc", "ipfs://Qmd", "ipfs://Qme"}, + base::BindLambdaForTesting( + [&success](bool result) { success = result; })); + + std::string expected = R"({"recursive": { + "Qjson" : ["a"], + "Qimage" : ["a", "b"], + "Qma": ["b"], + "Qmb": ["b"], + "Qmc": ["b"], + "Qmd": ["b"] + }, "direct": { + "Qma" : ["a"], + "Qmb" : ["a"], + "QmaD1" : ["a"], + "QmbD1" : ["a"] + }})"; absl::optional expected_value = base::JSONReader::Read( expected, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); @@ -199,12 +241,12 @@ TEST_F(IpfsLocalPinServiceTest, AddLocalPinJobTest) { TEST_F(IpfsLocalPinServiceTest, RemoveLocalPinJobTest) { { - std::string base = R"({ + std::string base = R"({"recursive": { "Qma" : ["a", "b"], "Qmb" : ["a", "b"], "Qmc" : ["a", "b"], "Qmd" : ["b"] - })"; + }, "direct": {}})"; absl::optional base_value = base::JSONReader::Read( base, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); @@ -226,7 +268,8 @@ TEST_F(IpfsLocalPinServiceTest, RemoveLocalPinJobTest) { absl::optional expected_value = base::JSONReader::Read( expected, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); - EXPECT_EQ(expected_value.value(), GetPrefs()->GetDict(kIPFSPinnedCids)); + EXPECT_EQ(expected_value.value(), + *(GetPrefs()->GetDict(kIPFSPinnedCids).FindDict("recursive"))); EXPECT_TRUE(success.value()); } @@ -245,7 +288,8 @@ TEST_F(IpfsLocalPinServiceTest, RemoveLocalPinJobTest) { absl::optional expected_value = base::JSONReader::Read( expected, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); - EXPECT_EQ(expected_value.value(), GetPrefs()->GetDict(kIPFSPinnedCids)); + EXPECT_EQ(expected_value.value(), + *(GetPrefs()->GetDict(kIPFSPinnedCids).FindDict("recursive"))); EXPECT_TRUE(success.value()); } @@ -260,19 +304,20 @@ TEST_F(IpfsLocalPinServiceTest, RemoveLocalPinJobTest) { absl::optional expected_value = base::JSONReader::Read( expected, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); - EXPECT_EQ(expected_value.value(), GetPrefs()->GetDict(kIPFSPinnedCids)); + EXPECT_EQ(expected_value.value(), + *(GetPrefs()->GetDict(kIPFSPinnedCids).FindDict("recursive"))); EXPECT_TRUE(success.value()); } } TEST_F(IpfsLocalPinServiceTest, VerifyLocalPinJobTest) { { - std::string base = R"({ + std::string base = R"({"recursive": { "Qma" : ["a", "b"], "Qmb" : ["a", "b"], "Qmc" : ["a", "b"], "Qmd" : ["b"] - })"; + }, "direct": {}})"; absl::optional base_value = base::JSONReader::Read( base, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); @@ -292,7 +337,7 @@ TEST_F(IpfsLocalPinServiceTest, VerifyLocalPinJobTest) { absl::optional success; service()->ValidatePins( - "a", {"Qma", "Qmb", "Qmc"}, + "a", {"ipfs://Qma", "ipfs://Qmb", "ipfs://Qmc"}, base::BindLambdaForTesting([&success](absl::optional result) { success = result.value(); })); @@ -315,7 +360,7 @@ TEST_F(IpfsLocalPinServiceTest, VerifyLocalPinJobTest) { absl::optional success; service()->ValidatePins( - "a", {"Qma", "Qmb", "Qmc"}, + "a", {"ipfs://Qma", "ipfs://Qmb", "ipfs://Qmc"}, base::BindLambdaForTesting([&success](absl::optional result) { success = result.value(); })); @@ -335,7 +380,7 @@ TEST_F(IpfsLocalPinServiceTest, VerifyLocalPinJobTest) { absl::optional success = false; service()->ValidatePins( - "b", {"Qma", "Qmb", "Qmc", "Qmd"}, + "b", {"ipfs://Qma", "ipfs://Qmb", "ipfs://Qmc", "ipfs://Qmd"}, base::BindLambdaForTesting([&success](absl::optional result) { success = result.value(); })); @@ -348,7 +393,10 @@ TEST_F(IpfsLocalPinServiceTest, VerifyLocalPinJobTest) { { absl::optional success; VerifyLocalPinJob job( - GetPrefs(), GetIpfsService(), "b", {"Qma", "Qmb", "Qmc", "Qmd"}, + GetPrefs(), GetIpfsService(), "b", + IpfsLocalPinService::ExtractMergedPinData( + {"ipfs://Qma", "ipfs://Qmb", "ipfs://Qmc", "ipfs://Qmd"}) + .value(), base::BindLambdaForTesting( [&success](absl::optional result) { success = result; })); @@ -368,12 +416,14 @@ TEST_F(IpfsLocalPinServiceTest, VerifyLocalPinJobTest) { TEST_F(IpfsLocalPinServiceTest, GcJobTest) { { - std::string base = R"({ + std::string base = R"({"recursive":{ "Qma" : ["a", "b"], - "Qmb" : ["a", "b"], + "Qmc" : ["a", "b"], "Qmd" : ["b"] - })"; + }, "direct":{ + "Qmb" : ["a", "b"] + }})"; absl::optional base_value = base::JSONReader::Read( base, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); @@ -420,7 +470,12 @@ TEST_F(IpfsLocalPinServiceTest, GcJobTest) { callback) { EXPECT_FALSE(cid.has_value()); EXPECT_TRUE(quiet); - GetPinsResult result = {{"Qm1", "Recursive"}, {"Qm2", "Recursive"}}; + GetPinsResult result; + if (type == "recursive") { + result = {{"Qm1", "Recursive"}, {"Qm2", "Recursive"}}; + } else { + result = {}; + } std::move(callback).Run(result); })); EXPECT_CALL(*GetIpfsService(), RemovePin(_, _)).Times(1); @@ -465,12 +520,12 @@ TEST_F(IpfsLocalPinServiceTest, GcJobTest) { TEST_F(IpfsLocalPinServiceTest, ResetTest) { { - std::string base = R"({ + std::string base = R"({"recursive":{ "Qma" : ["a", "b"], "Qmb" : ["a", "b"], "Qmc" : ["a", "b"], "Qmd" : ["b"] - })"; + }, "direct":{}})"; absl::optional base_value = base::JSONReader::Read( base, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); @@ -499,12 +554,12 @@ TEST_F(IpfsLocalPinServiceTest, ResetTest) { TEST_F(IpfsLocalPinServiceTest, ResetTest_UnpinFailed) { { - std::string base = R"({ + std::string base = R"({"recursive": { "Qma" : ["a", "b"], "Qmb" : ["a", "b"], "Qmc" : ["a", "b"], "Qmd" : ["b"] - })"; + }, "direct":{}})"; absl::optional base_value = base::JSONReader::Read( base, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); @@ -527,18 +582,19 @@ TEST_F(IpfsLocalPinServiceTest, ResetTest_UnpinFailed) { absl::optional reset_result; service()->Reset(base::BindLambdaForTesting( [&reset_result](bool result) { reset_result = result; })); - EXPECT_EQ(4u, GetPrefs()->GetDict(kIPFSPinnedCids).size()); + EXPECT_EQ(4u, + GetPrefs()->GetDict(kIPFSPinnedCids).FindDict("recursive")->size()); ASSERT_FALSE(reset_result.value()); } TEST_F(IpfsLocalPinServiceTest, ResetTest_NoPinnedItems) { { - std::string base = R"({ + std::string base = R"({"recursive":{ "Qma" : ["a", "b"], "Qmb" : ["a", "b"], "Qmc" : ["a", "b"], "Qmd" : ["b"] - })"; + }, "direct":{}})"; absl::optional base_value = base::JSONReader::Read( base, base::JSON_PARSE_CHROMIUM_EXTENSIONS | base::JSONParserOptions::JSON_PARSE_RFC); From a8aae875180f3e9d024967568ccd72cfb6dba534 Mon Sep 17 00:00:00 2001 From: oisupov Date: Fri, 24 Mar 2023 17:46:14 +0700 Subject: [PATCH 2/4] Review fix --- components/ipfs/ipfs_service.cc | 14 +++- components/ipfs/ipfs_service.h | 4 +- components/ipfs/pin/ipfs_local_pin_service.cc | 70 ++++++++----------- components/ipfs/pin/ipfs_local_pin_service.h | 55 ++++++++------- .../pin/ipfs_local_pin_service_unittest.cc | 6 +- components/ipfs/pin/ipfs_pin_rpc_types.h | 1 + 6 files changed, 80 insertions(+), 70 deletions(-) diff --git a/components/ipfs/ipfs_service.cc b/components/ipfs/ipfs_service.cc index e2c883344d61..033a3fe1ce7b 100644 --- a/components/ipfs/ipfs_service.cc +++ b/components/ipfs/ipfs_service.cc @@ -406,8 +406,8 @@ void IpfsService::AddPin(const std::vector& cids, iter->get()->Request( "POST", gurl, std::string(), std::string(), false, - base::BindOnce(&IpfsService::OnPinAddResult, base::Unretained(this), iter, - std::move(callback)), + base::BindOnce(&IpfsService::OnPinAddResult, base::Unretained(this), + cids.size(), recursive, iter, std::move(callback)), GetHeaders(gurl)); } @@ -1075,6 +1075,8 @@ void IpfsService::OnGetPinsResult( } void IpfsService::OnPinAddResult( + size_t cids_count_in_request, + bool recursive, APIRequestList::iterator iter, AddPinCallback callback, api_request_helper::APIRequestResult response) { @@ -1095,6 +1097,14 @@ void IpfsService::OnPinAddResult( return; } + if (parse_result->pins.size() != cids_count_in_request) { + VLOG(1) << "Not all CIDs were added"; + std::move(callback).Run(absl::nullopt); + return; + } + + parse_result->recursive = recursive; + std::move(callback).Run(std::move(parse_result)); } diff --git a/components/ipfs/ipfs_service.h b/components/ipfs/ipfs_service.h index b567d99ca8a9..59b3c78a7f74 100644 --- a/components/ipfs/ipfs_service.h +++ b/components/ipfs/ipfs_service.h @@ -222,7 +222,9 @@ class IpfsService : public KeyedService, void OnGetPinsResult(APIRequestList::iterator iter, GetPinsCallback callback, api_request_helper::APIRequestResult response); - void OnPinAddResult(APIRequestList::iterator iter, + void OnPinAddResult(size_t cids_count_in_request, + bool recursive, + APIRequestList::iterator iter, AddPinCallback callback, api_request_helper::APIRequestResult response); void OnPinRemoveResult(APIRequestList::iterator iter, diff --git a/components/ipfs/pin/ipfs_local_pin_service.cc b/components/ipfs/pin/ipfs_local_pin_service.cc index de2219a15c7f..9d46ec1abe66 100644 --- a/components/ipfs/pin/ipfs_local_pin_service.cc +++ b/components/ipfs/pin/ipfs_local_pin_service.cc @@ -23,9 +23,6 @@ namespace ipfs { bool PinData::operator==(const PinData& d) const { return this->cid == d.cid && this->pinning_mode == d.pinning_mode; } -bool PinData::operator==(const PinData& d) { - return this->cid == d.cid && this->pinning_mode == d.pinning_mode; -} namespace { const char kRecursiveMode[] = "recursive"; @@ -53,27 +50,26 @@ absl::optional> IpfsLocalPinService::ExtractPinData( } // This will just remove ipfs:// scheme auto path = gurl.path(); - std::vector splitted = + std::vector slit = base::SplitString(path, "/", base::WhitespaceHandling::KEEP_WHITESPACE, base::SplitResult::SPLIT_WANT_NONEMPTY); - if (splitted.empty()) { + if (slit.empty()) { return absl::nullopt; } std::vector result; - size_t counter = 0; std::string builder = "/ipfs"; - for (const auto& part : splitted) { + for (const auto& part : slit) { builder += "/"; builder += part; - bool recursive = ++counter == splitted.size(); - result.push_back(PinData{ - builder, recursive ? PinningMode::RECURSIVE : PinningMode::DIRECT}); + result.push_back(PinData{builder, PinningMode::DIRECT}); } + result[result.size() - 1].pinning_mode = PinningMode::RECURSIVE; return result; } absl::optional> -IpfsLocalPinService::ExtractMergedPinData(std::vector ipfs_urls) { +IpfsLocalPinService::ExtractMergedPinData( + const std::vector& ipfs_urls) { std::vector result; for (const auto& ipfs_url : ipfs_urls) { auto pins_data_for_url = ExtractPinData(ipfs_url); @@ -81,7 +77,7 @@ IpfsLocalPinService::ExtractMergedPinData(std::vector ipfs_urls) { return absl::nullopt; } for (const auto& item : pins_data_for_url.value()) { - if (std::find(result.begin(), result.end(), item) == result.end()) { + if (!base::Contains(result, item)) { result.push_back(item); } } @@ -103,7 +99,7 @@ AddLocalPinJob::AddLocalPinJob(PrefService* prefs_service, AddLocalPinJob::~AddLocalPinJob() = default; void AddLocalPinJob::Start() { - auto callback = base::BarrierCallback( + auto callback = base::BarrierCallback>( 2, base::BindOnce(&AddLocalPinJob::OnAddPinResult, weak_ptr_factory_.GetWeakPtr())); @@ -121,29 +117,25 @@ void AddLocalPinJob::Start() { ipfs_service_->AddPin( recursive_cids, true, base::BindOnce(&AddLocalPinJob::Accumulate, - weak_ptr_factory_.GetWeakPtr(), PinningMode::RECURSIVE, - recursive_cids, callback)); + weak_ptr_factory_.GetWeakPtr(), callback)); ipfs_service_->AddPin( direct_cids, false, base::BindOnce(&AddLocalPinJob::Accumulate, - weak_ptr_factory_.GetWeakPtr(), PinningMode::DIRECT, - direct_cids, callback)); + weak_ptr_factory_.GetWeakPtr(), callback)); } void AddLocalPinJob::Accumulate( - PinningMode pinning_mode, - std::vector cids, - base::OnceCallback callback, + base::OnceCallback)> callback, absl::optional result) { - if (!result || result->pins.size() != cids.size()) { + if (!result) { pinning_failed_ = true; } - std::move(callback).Run(std::pair>( - pinning_mode, result)); + std::move(callback).Run(std::move(result)); } -void AddLocalPinJob::OnAddPinResult(std::vector result) { +void AddLocalPinJob::OnAddPinResult( + std::vector> result) { if (is_canceled_) { std::move(callback_).Run(false); return; @@ -158,10 +150,11 @@ void AddLocalPinJob::OnAddPinResult(std::vector result) { ScopedDictPrefUpdate update(prefs_service_, kIPFSPinnedCids); base::Value::Dict& update_dict = update.Get(); - for (const auto& mode : result) { - auto* mode_dict = - update_dict.EnsureDict(GetPrefNameFromPinningMode(mode.first)); - for (const auto& cid : mode.second.value().pins) { + for (const auto& add_pin_result : result) { + auto* mode_dict = update_dict.EnsureDict(GetPrefNameFromPinningMode( + add_pin_result->recursive ? PinningMode::RECURSIVE + : PinningMode::DIRECT)); + for (const auto& cid : add_pin_result->pins) { base::Value::List* list = mode_dict->EnsureList(cid); list->EraseValue(base::Value(key_)); list->Append(base::Value(key_)); @@ -259,31 +252,30 @@ GcJob::GcJob(PrefService* prefs_service, GcJob::~GcJob() = default; void GcJob::Start() { - auto callback = base::BarrierCallback( + auto callback = base::BarrierCallback>( 2, base::BindOnce(&GcJob::OnGetPinsResult, weak_ptr_factory_.GetWeakPtr())); ipfs_service_->GetPins( absl::nullopt, GetPrefNameFromPinningMode(PinningMode::RECURSIVE), true, base::BindOnce(&GcJob::Accumulate, weak_ptr_factory_.GetWeakPtr(), - PinningMode::RECURSIVE, callback)); + callback)); ipfs_service_->GetPins( absl::nullopt, GetPrefNameFromPinningMode(PinningMode::DIRECT), true, base::BindOnce(&GcJob::Accumulate, weak_ptr_factory_.GetWeakPtr(), - PinningMode::DIRECT, callback)); + callback)); } -void GcJob::Accumulate(PinningMode pinning_mode, - base::OnceCallback callback, - absl::optional result) { +void GcJob::Accumulate( + base::OnceCallback)> callback, + absl::optional result) { if (!result) { gc_job_failed_ = true; } - std::move(callback).Run(std::pair>( - pinning_mode, result)); + std::move(callback).Run(std::move(result)); } -void GcJob::OnGetPinsResult(std::vector result) { +void GcJob::OnGetPinsResult(std::vector> result) { if (is_canceled_) { std::move(callback_).Run(false); return; @@ -299,8 +291,8 @@ void GcJob::OnGetPinsResult(std::vector result) { prefs_service_->GetDict(kIPFSPinnedCids); // Check both recursive and direct mode dictionaries. If there is no CID in // both, then unpin. - for (const auto& modes : result) { - for (const auto& cid : modes.second.value()) { + for (const auto& it : result) { + for (const auto& cid : it.value()) { if (!pinning_modes_dict.FindListByDottedPath( base::StrCat({GetPrefNameFromPinningMode(PinningMode::RECURSIVE), ".", cid.first})) && diff --git a/components/ipfs/pin/ipfs_local_pin_service.h b/components/ipfs/pin/ipfs_local_pin_service.h index 0696b22335d5..737f451556ac 100644 --- a/components/ipfs/pin/ipfs_local_pin_service.h +++ b/components/ipfs/pin/ipfs_local_pin_service.h @@ -28,7 +28,6 @@ struct PinData { PinningMode pinning_mode; bool operator==(const PinData&) const; - bool operator==(const PinData&); }; absl::optional> ExtractPinData( @@ -42,18 +41,25 @@ using GcCallback = base::OnceCallback; /** * Pins provided cids and writes record to kIPFSPinnedCids: * { - * // List of all pinned CIDs - * "Qme1": [ - * // List of tokens that contain this CID - * "nft.local.60.0x1.0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d.0x1" - * "nft.local.60.0x1.0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d.0x2" - * ], - * "Qme2": [ - * "nft.local.60.0x1.0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d.0x1" - * ], - * "Qme3": [ - * "nft.local.60.0x1.0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d.0x2" - * ] + * // CIDs which were pinned recursively + * "recursive": { + * // List of all pinned CIDs + * "Qme1": [ + * // List of tokens that contain this CID + * "nft.local.60.0x1.0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d.0x1" + * "nft.local.60.0x1.0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d.0x2" + * ], + * "Qme2": [ + * "nft.local.60.0x1.0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d.0x1" + * ], + * "Qme3": [ + * "nft.local.60.0x1.0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d.0x2" + * ] + * }, + * // CIDs which were pinned using direct mode + * "direct": { + * ... + * } * } */ class AddLocalPinJob : public IpfsBaseJob { @@ -68,13 +74,10 @@ class AddLocalPinJob : public IpfsBaseJob { void Start() override; private: - using AccumulatedResult = - std::pair>; - void Accumulate(PinningMode pinning_mode, - std::vector cids, - base::OnceCallback callback, - absl::optional result); - void OnAddPinResult(std::vector result); + void Accumulate( + base::OnceCallback)> callback, + absl::optional result); + void OnAddPinResult(std::vector> result); raw_ptr prefs_service_; raw_ptr ipfs_service_; @@ -138,12 +141,10 @@ class GcJob : public IpfsBaseJob { void Start() override; private: - using AccumulatedResult = - std::pair>; - void Accumulate(PinningMode pinning_mode, - base::OnceCallback callback, - absl::optional result); - void OnGetPinsResult(std::vector result); + void Accumulate( + base::OnceCallback)> callback, + absl::optional result); + void OnGetPinsResult(std::vector> result); void OnPinsRemovedResult(absl::optional result); raw_ptr prefs_service_; @@ -180,7 +181,7 @@ class IpfsLocalPinService : public KeyedService { static absl::optional> ExtractPinData( const std::string& ipfs_url); static absl::optional> ExtractMergedPinData( - std::vector ipfs_urls); + const std::vector& ipfs_urls); private: void OnLsPinCliResult(base::OnceCallback callback, diff --git a/components/ipfs/pin/ipfs_local_pin_service_unittest.cc b/components/ipfs/pin/ipfs_local_pin_service_unittest.cc index c8d31d56093a..bfbaadc5157f 100644 --- a/components/ipfs/pin/ipfs_local_pin_service_unittest.cc +++ b/components/ipfs/pin/ipfs_local_pin_service_unittest.cc @@ -98,6 +98,7 @@ TEST_F(IpfsLocalPinServiceTest, AddLocalPinJobTest) { } else { result.pins = {"Qma", "Qmb", "QmaD1", "QmbD1"}; } + result.recursive = recursive; std::move(callback).Run(result); })); @@ -132,6 +133,7 @@ TEST_F(IpfsLocalPinServiceTest, AddLocalPinJobTest) { if (recursive) { result.pins = {"Qma", "Qmb", "Qmc", "Qmd", "Qimage"}; } + result.recursive = recursive; std::move(callback).Run(result); })); @@ -170,8 +172,10 @@ TEST_F(IpfsLocalPinServiceTest, AddLocalPinJobTest) { IpfsService::AddPinCallback callback) { AddPinResult result; if (recursive) { - result.pins = {"Qma", "Qmb", "Qmc"}; + std::move(callback).Run(absl::nullopt); + return; } + result.recursive = recursive; std::move(callback).Run(result); })); diff --git a/components/ipfs/pin/ipfs_pin_rpc_types.h b/components/ipfs/pin/ipfs_pin_rpc_types.h index ba0b9c732886..a5c6a7e8c113 100644 --- a/components/ipfs/pin/ipfs_pin_rpc_types.h +++ b/components/ipfs/pin/ipfs_pin_rpc_types.h @@ -17,6 +17,7 @@ struct AddPinResult { ~AddPinResult(); AddPinResult(const AddPinResult&); AddPinResult& operator=(const AddPinResult&); + bool recursive = true; std::vector pins; int progress = 0; }; From 31d6a3f4b37478bdced52290b078f07983babc00 Mon Sep 17 00:00:00 2001 From: oisupov Date: Tue, 28 Mar 2023 00:48:32 +0700 Subject: [PATCH 3/4] Ensure ipfs daemon is initialized --- .../brave_wallet_pin_service_unittest.cc | 1 - components/ipfs/ipfs_service.h | 2 +- components/ipfs/pin/ipfs_base_pin_service.cc | 46 ++++++++++++++++--- components/ipfs/pin/ipfs_base_pin_service.h | 8 ++-- .../pin/ipfs_base_pin_service_unittest.cc | 44 +++++++++++++++--- 5 files changed, 84 insertions(+), 17 deletions(-) diff --git a/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc b/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc index b15ce301851e..1f2866b49608 100644 --- a/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc +++ b/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc @@ -179,7 +179,6 @@ class MockIpfsService : public IpfsService { ~MockIpfsService() override = default; MOCK_METHOD1(AddObserver, void(ipfs::IpfsServiceObserver* observer)); - MOCK_METHOD0(IsDaemonLaunched, bool()); }; class MockContentTypeChecker : public ContentTypeChecker { diff --git a/components/ipfs/ipfs_service.h b/components/ipfs/ipfs_service.h index 59b3c78a7f74..449e26778fcc 100644 --- a/components/ipfs/ipfs_service.h +++ b/components/ipfs/ipfs_service.h @@ -106,7 +106,7 @@ class IpfsService : public KeyedService, void AddObserver(IpfsServiceObserver* observer); void RemoveObserver(IpfsServiceObserver* observer); - bool IsDaemonLaunched() const; + virtual bool IsDaemonLaunched() const; static void RegisterProfilePrefs(PrefRegistrySimple* registry); bool IsIPFSExecutableAvailable() const; void RegisterIpfsClientUpdater(); diff --git a/components/ipfs/pin/ipfs_base_pin_service.cc b/components/ipfs/pin/ipfs_base_pin_service.cc index 8584a74bd825..0f6b610f48bd 100644 --- a/components/ipfs/pin/ipfs_base_pin_service.cc +++ b/components/ipfs/pin/ipfs_base_pin_service.cc @@ -35,12 +35,18 @@ void IpfsBasePinService::OnIpfsShutdown() { } } -void IpfsBasePinService::OnGetConnectedPeers( +void IpfsBasePinService::OnGetConnectedPeersResult( + size_t attempt, bool success, const std::vector& peers) { - if (success && !daemon_ready_) { + if (daemon_ready_) { + return; + } + if (success) { daemon_ready_ = true; DoNextJob(); + } else { + PostGetConnectedPeers(attempt++); } } @@ -83,13 +89,41 @@ void IpfsBasePinService::MaybeStartDaemon() { return; } - ipfs_service_->StartDaemonAndLaunch(base::BindOnce( - &IpfsBasePinService::OnDaemonStarted, weak_ptr_factory_.GetWeakPtr())); + ipfs_service_->StartDaemonAndLaunch( + base::BindOnce(&IpfsBasePinService::PostGetConnectedPeers, + weak_ptr_factory_.GetWeakPtr(), 1)); } -void IpfsBasePinService::OnDaemonStarted() { +void IpfsBasePinService::PostGetConnectedPeers(size_t attempt) { + if (daemon_ready_) { + return; + } + + if (!ipfs_service_->IsDaemonLaunched()) { + return; + } + + if (jobs_.empty()) { + return; + } + + if (attempt > 5) { + return; + } + // Ensure that daemon service is fully initialized - ipfs_service_->GetConnectedPeers(base::NullCallback(), 2); + base::SequencedTaskRunner::GetCurrentDefault()->PostDelayedTask( + FROM_HERE, + base::BindOnce(&IpfsBasePinService::GetConnectedPeers, + weak_ptr_factory_.GetWeakPtr(), attempt), + base::Seconds(30 * attempt)); +} + +void IpfsBasePinService::GetConnectedPeers(size_t attempt) { + ipfs_service_->GetConnectedPeers( + base::BindOnce(&IpfsBasePinService::OnGetConnectedPeersResult, + weak_ptr_factory_.GetWeakPtr(), attempt), + absl::nullopt); } } // namespace ipfs diff --git a/components/ipfs/pin/ipfs_base_pin_service.h b/components/ipfs/pin/ipfs_base_pin_service.h index f223f78d8a1e..492758c30fc9 100644 --- a/components/ipfs/pin/ipfs_base_pin_service.h +++ b/components/ipfs/pin/ipfs_base_pin_service.h @@ -37,10 +37,11 @@ class IpfsBasePinService : public IpfsServiceObserver { virtual void AddJob(std::unique_ptr job); void OnJobDone(bool result); + void OnGetConnectedPeersResult(size_t attempt, + bool succes, + const std::vector& peers); void OnIpfsShutdown() override; - void OnGetConnectedPeers(bool succes, - const std::vector& peers) override; protected: // For testing @@ -52,8 +53,9 @@ class IpfsBasePinService : public IpfsServiceObserver { bool IsDaemonReady(); void MaybeStartDaemon(); - void OnDaemonStarted(); void DoNextJob(); + void PostGetConnectedPeers(size_t attempt); + void GetConnectedPeers(size_t attempt); bool daemon_ready_ = false; raw_ptr ipfs_service_; diff --git a/components/ipfs/pin/ipfs_base_pin_service_unittest.cc b/components/ipfs/pin/ipfs_base_pin_service_unittest.cc index 21930b11f595..e594aefe2e22 100644 --- a/components/ipfs/pin/ipfs_base_pin_service_unittest.cc +++ b/components/ipfs/pin/ipfs_base_pin_service_unittest.cc @@ -9,6 +9,7 @@ #include #include "base/test/bind.h" +#include "base/time/time_override.h" #include "brave/components/ipfs/ipfs_service.h" #include "brave/components/ipfs/pref_names.h" #include "components/prefs/testing_pref_service.h" @@ -16,6 +17,7 @@ #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" +using base::subtle::ScopedTimeClockOverrides; using testing::_; namespace ipfs { @@ -28,6 +30,7 @@ class MockIpfsService : public IpfsService { ~MockIpfsService() override = default; + MOCK_METHOD(bool, IsDaemonLaunched, (), (const, override)); MOCK_METHOD1(StartDaemonAndLaunch, void(base::OnceCallback)); MOCK_METHOD2(GetConnectedPeers, void(IpfsService::GetConnectedPeersCallback, @@ -41,6 +44,8 @@ class IpfsBasePinServiceTest : public testing::Test { IpfsBasePinServiceTest() = default; void SetUp() override { + task_environment_ = std::make_unique( + base::test::TaskEnvironment::TimeSource::MOCK_TIME); ipfs_base_pin_service_ = std::make_unique(GetIpfsService()); } @@ -51,10 +56,12 @@ class IpfsBasePinServiceTest : public testing::Test { IpfsBasePinService* service() { return ipfs_base_pin_service_.get(); } + content::BrowserTaskEnvironment* env() { return task_environment_.get(); } + private: std::unique_ptr ipfs_base_pin_service_; testing::NiceMock ipfs_service_; - content::BrowserTaskEnvironment task_environment_; + std::unique_ptr task_environment_; }; class MockJob : public IpfsBaseJob { @@ -74,7 +81,7 @@ class MockJob : public IpfsBaseJob { }; TEST_F(IpfsBasePinServiceTest, TasksExecuted) { - service()->OnGetConnectedPeers(true, {}); + service()->OnGetConnectedPeersResult(1, true, {}); absl::optional method_called; std::unique_ptr first_job = std::make_unique( base::BindLambdaForTesting([&method_called]() { method_called = true; })); @@ -93,7 +100,7 @@ TEST_F(IpfsBasePinServiceTest, TasksExecuted) { } TEST_F(IpfsBasePinServiceTest, OnIpfsShutdown) { - service()->OnGetConnectedPeers(true, {}); + service()->OnGetConnectedPeersResult(1, true, {}); EXPECT_TRUE(service()->daemon_ready_); std::unique_ptr first_job = @@ -116,7 +123,7 @@ TEST_F(IpfsBasePinServiceTest, OnIpfsShutdown) { } TEST_F(IpfsBasePinServiceTest, OnGetConnectedPeers) { - service()->OnGetConnectedPeers(true, {}); + service()->OnGetConnectedPeersResult(1, true, {}); EXPECT_TRUE(service()->daemon_ready_); std::unique_ptr first_job = @@ -126,11 +133,11 @@ TEST_F(IpfsBasePinServiceTest, OnGetConnectedPeers) { service()->AddJob(std::move(first_job)); - service()->OnGetConnectedPeers(true, {}); + service()->OnGetConnectedPeersResult(1, true, {}); service()->AddJob(std::move(second_job)); - service()->OnGetConnectedPeers(true, {}); + service()->OnGetConnectedPeersResult(1, true, {}); EXPECT_EQ(1u, service()->jobs_.size()); EXPECT_TRUE(service()->current_job_); @@ -146,4 +153,29 @@ TEST_F(IpfsBasePinServiceTest, OnGetConnectedPeers) { EXPECT_FALSE(service()->current_job_); } +TEST_F(IpfsBasePinServiceTest, OnGetConnectedPeers_Retry) { + ON_CALL(*GetIpfsService(), StartDaemonAndLaunch(_)) + .WillByDefault( + ::testing::Invoke([](base::OnceCallback callback) { + std::move(callback).Run(); + })); + ON_CALL(*GetIpfsService(), IsDaemonLaunched()) + .WillByDefault(::testing::Return(true)); + + std::unique_ptr first_job = + std::make_unique(base::DoNothing()); + + service()->AddJob(std::move(first_job)); + EXPECT_CALL(*GetIpfsService(), GetConnectedPeers(_, _)).Times(1); + + env()->AdvanceClock(base::Minutes(2)); + env()->RunUntilIdle(); + + EXPECT_CALL(*GetIpfsService(), GetConnectedPeers(_, _)).Times(1); + service()->OnGetConnectedPeersResult(1, false, {}); + + env()->AdvanceClock(base::Minutes(2)); + env()->RunUntilIdle(); +} + } // namespace ipfs From 6562e596db1da8f5d391c74983575cb506ca7dd5 Mon Sep 17 00:00:00 2001 From: oisupov Date: Tue, 28 Mar 2023 22:41:18 +0700 Subject: [PATCH 4/4] Fix validation --- .../browser/brave_wallet_auto_pin_service.cc | 14 +++++---- .../browser/brave_wallet_auto_pin_service.h | 2 +- .../brave_wallet_auto_pin_service_unittest.cc | 3 +- .../browser/brave_wallet_pin_service.cc | 30 +++++++++---------- .../brave_wallet_pin_service_unittest.cc | 21 +++++++------ .../brave_wallet/common/brave_wallet.mojom | 14 +++++++-- components/ipfs/pin/ipfs_local_pin_service.cc | 3 +- .../pin/ipfs_local_pin_service_unittest.cc | 2 +- 8 files changed, 52 insertions(+), 37 deletions(-) diff --git a/components/brave_wallet/browser/brave_wallet_auto_pin_service.cc b/components/brave_wallet/browser/brave_wallet_auto_pin_service.cc index 5fc1be589fad..6f9b128ba647 100644 --- a/components/brave_wallet/browser/brave_wallet_auto_pin_service.cc +++ b/components/brave_wallet/browser/brave_wallet_auto_pin_service.cc @@ -281,13 +281,15 @@ void BraveWalletAutoPinService::OnTaskFinished(bool result, } void BraveWalletAutoPinService::OnValidateTaskFinished( - bool result, - mojom::PinErrorPtr error) { - if (!result) { - AddOrExecute(std::make_unique(current_->token, Operation::kAdd, - current_->service)); + mojom::TokenValidationResult result) { + if (result == mojom::TokenValidationResult::kValidationError) { + PostRetry(std::move(current_)); + } + auto current = std::move(current_); + if (result == mojom::TokenValidationResult::kValidationFailed) { + AddOrExecute(std::make_unique(current->token, Operation::kAdd, + current->service)); } - current_.reset(); CheckQueue(); } diff --git a/components/brave_wallet/browser/brave_wallet_auto_pin_service.h b/components/brave_wallet/browser/brave_wallet_auto_pin_service.h index ab3152455c91..205e948da468 100644 --- a/components/brave_wallet/browser/brave_wallet_auto_pin_service.h +++ b/components/brave_wallet/browser/brave_wallet_auto_pin_service.h @@ -96,7 +96,7 @@ class BraveWalletAutoPinService void UnpinToken(const std::unique_ptr& data); void OnTaskFinished(bool result, mojom::PinErrorPtr error); - void OnValidateTaskFinished(bool result, mojom::PinErrorPtr error); + void OnValidateTaskFinished(mojom::TokenValidationResult result); mojo::Receiver token_observer_{this}; diff --git a/components/brave_wallet/browser/brave_wallet_auto_pin_service_unittest.cc b/components/brave_wallet/browser/brave_wallet_auto_pin_service_unittest.cc index 39fdadb2372e..d8e1c6b0ae56 100644 --- a/components/brave_wallet/browser/brave_wallet_auto_pin_service_unittest.cc +++ b/components/brave_wallet/browser/brave_wallet_auto_pin_service_unittest.cc @@ -270,7 +270,8 @@ TEST_F(BraveWalletAutoPinServiceTest, ValidateOldTokens_WhenRestore) { [](BlockchainTokenPtr token, const absl::optional& service, BraveWalletPinService::ValidateCallback callback) { - std::move(callback).Run(true, nullptr); + std::move(callback).Run( + mojom::TokenValidationResult::kValidationPassed); })); EXPECT_CALL(*GetBraveWalletPinService(), diff --git a/components/brave_wallet/browser/brave_wallet_pin_service.cc b/components/brave_wallet/browser/brave_wallet_pin_service.cc index 93289a5a0fa6..df56a00c296c 100644 --- a/components/brave_wallet/browser/brave_wallet_pin_service.cc +++ b/components/brave_wallet/browser/brave_wallet_pin_service.cc @@ -412,13 +412,20 @@ absl::optional BraveWalletPinService::ServiceFromPrefPath( void BraveWalletPinService::Validate(mojom::BlockchainTokenPtr token, const absl::optional& service, ValidateCallback callback) { + auto path = GetTokenPrefPath(absl::nullopt, token); + if (!path) { + std::move(callback).Run(mojom::TokenValidationResult::kValidationIgnored); + return; + } + mojom::TokenPinStatusPtr status = GetTokenStatus(service, token); if (!status) { - std::move(callback).Run(false, nullptr); + std::move(callback).Run(mojom::TokenValidationResult::kValidationIgnored); return; } + if (status->code != mojom::TokenPinStatusCode::STATUS_PINNED) { - std::move(callback).Run(false, nullptr); + std::move(callback).Run(mojom::TokenValidationResult::kValidationIgnored); return; } @@ -429,16 +436,7 @@ void BraveWalletPinService::Validate(mojom::BlockchainTokenPtr token, SetTokenStatus(service, std::move(token), mojom::TokenPinStatusCode::STATUS_PINNING_IN_PROGRESS, nullptr); - std::move(callback).Run(true, nullptr); - return; - } - - auto path = GetTokenPrefPath(absl::nullopt, token); - if (!path) { - std::move(callback).Run( - false, - mojom::PinError::New(mojom::WalletPinServiceErrorCode::ERR_WRONG_TOKEN, - "Wrong token data")); + std::move(callback).Run(mojom::TokenValidationResult::kValidationFailed); return; } @@ -450,7 +448,7 @@ void BraveWalletPinService::Validate(mojom::BlockchainTokenPtr token, std::move(callback), std::move(token))); } else { // Remote pinning not implemented yet - std::move(callback).Run(false, nullptr); + std::move(callback).Run(mojom::TokenValidationResult::kValidationIgnored); } } @@ -756,7 +754,7 @@ void BraveWalletPinService::OnTokenValidated( mojom::BlockchainTokenPtr token, absl::optional result) { if (!result.has_value()) { - std::move(callback).Run(false, nullptr); + std::move(callback).Run(mojom::TokenValidationResult::kValidationError); return; } @@ -764,13 +762,13 @@ void BraveWalletPinService::OnTokenValidated( SetTokenStatus(service, token, mojom::TokenPinStatusCode::STATUS_PINNING_IN_PROGRESS, nullptr); + std::move(callback).Run(mojom::TokenValidationResult::kValidationFailed); } else { // Also updates verification timestamp SetTokenStatus(service, token, mojom::TokenPinStatusCode::STATUS_PINNED, nullptr); + std::move(callback).Run(mojom::TokenValidationResult::kValidationPassed); } - - std::move(callback).Run(true, nullptr); } bool BraveWalletPinService::AddToken( diff --git a/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc b/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc index 1f2866b49608..b67da480d13d 100644 --- a/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc +++ b/components/brave_wallet/browser/brave_wallet_pin_service_unittest.cc @@ -939,14 +939,15 @@ TEST_F(BraveWalletPinServiceTest, ValidatePin) { std::move(callback).Run(true); })); - absl::optional validate_status; + absl::optional validate_status; service()->Validate( std::move(token), absl::nullopt, base::BindLambdaForTesting( - [&validate_status](bool status, mojom::PinErrorPtr error) { + [&validate_status](mojom::TokenValidationResult status) { validate_status = status; })); - EXPECT_TRUE(validate_status.value()); + EXPECT_EQ(mojom::TokenValidationResult::kValidationPassed, + validate_status.value()); const base::Value::Dict* token_record = GetPrefs() @@ -967,15 +968,16 @@ TEST_F(BraveWalletPinServiceTest, ValidatePin) { std::move(callback).Run(absl::nullopt); })); - absl::optional validate_status; + absl::optional validate_status; service()->Validate( std::move(token), absl::nullopt, base::BindLambdaForTesting( - [&validate_status](bool status, mojom::PinErrorPtr error) { + [&validate_status](mojom::TokenValidationResult status) { validate_status = status; })); - EXPECT_FALSE(validate_status.value()); + EXPECT_EQ(mojom::TokenValidationResult::kValidationError, + validate_status.value()); const base::Value::Dict* token_record = GetPrefs() @@ -996,15 +998,16 @@ TEST_F(BraveWalletPinServiceTest, ValidatePin) { std::move(callback).Run(false); })); - absl::optional validate_status; + absl::optional validate_status; service()->Validate( std::move(token), absl::nullopt, base::BindLambdaForTesting( - [&validate_status](bool status, mojom::PinErrorPtr error) { + [&validate_status](mojom::TokenValidationResult status) { validate_status = status; })); - EXPECT_TRUE(validate_status.value()); + EXPECT_EQ(mojom::TokenValidationResult::kValidationFailed, + validate_status.value()); const base::Value::Dict* token_record = GetPrefs() diff --git a/components/brave_wallet/common/brave_wallet.mojom b/components/brave_wallet/common/brave_wallet.mojom index e41cbcdebf97..aacf0c951d56 100644 --- a/components/brave_wallet/common/brave_wallet.mojom +++ b/components/brave_wallet/common/brave_wallet.mojom @@ -265,6 +265,17 @@ interface BraveWalletPinServiceObserver { OnLocalNodeStatusChanged(bool running); }; +enum TokenValidationResult { + // All is ok + kValidationPassed, + // Token should be repinned + kValidationFailed, + // Should retry validation later + kValidationError, + // Token is unsupported + kValidationIgnored +}; + // Low-level interface for token pinning. // String service argument is used to select on which pinning // service operation should be performed. @@ -283,8 +294,7 @@ interface WalletPinService { GetTokenStatus(BlockchainToken token) => (TokenPinOverview? status, PinError? error); // Checks whether token is pinned correctly. - Validate(BlockchainToken token, string? service) => (bool result, - PinError? error); + Validate(BlockchainToken token, string? service) => (TokenValidationResult result); // Returns whether IPFS localnode is currently running. IsLocalNodeRunning() => (bool result); diff --git a/components/ipfs/pin/ipfs_local_pin_service.cc b/components/ipfs/pin/ipfs_local_pin_service.cc index 9d46ec1abe66..d32617a6acd7 100644 --- a/components/ipfs/pin/ipfs_local_pin_service.cc +++ b/components/ipfs/pin/ipfs_local_pin_service.cc @@ -235,10 +235,11 @@ void VerifyLocalPinJob::OnGetPinsResult(absl::optional result) { } if (!result) { - std::move(callback_).Run(absl::nullopt); + std::move(callback_).Run(false); return; } + // TODO(cypt4): Check exact pinning modes for each cid. std::move(callback_).Run(result->size() == pins_data_.size()); } diff --git a/components/ipfs/pin/ipfs_local_pin_service_unittest.cc b/components/ipfs/pin/ipfs_local_pin_service_unittest.cc index bfbaadc5157f..3a2b870dc7a1 100644 --- a/components/ipfs/pin/ipfs_local_pin_service_unittest.cc +++ b/components/ipfs/pin/ipfs_local_pin_service_unittest.cc @@ -414,7 +414,7 @@ TEST_F(IpfsLocalPinServiceTest, VerifyLocalPinJobTest) { job.Start(); - EXPECT_FALSE(success.has_value()); + EXPECT_FALSE(success.value()); } }