Core part for local nft pinning #16073

cypt4 · 2022-11-23T16:43:08Z

Resolves brave/brave-browser#19283

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

This is core part, so there is actually nothing to be tested

github-actions · 2022-11-29T13:25:13Z

⚠️ PR head is an unsigned commit
commit: fa5f781
reason: unsigned
Please follow the handbook to configure commit signing
cc: @cypt4

yrliou

Some early feedback on the first commit, haven't checked the change in IpfsService.

yrliou · 2022-12-07T19:37:17Z

components/ipfs/ipfs_json_parser.h

+  static bool GetAddPinsResultFromJSON(const std::string& json,
+                                       ipfs::AddPinResult* add_pin_result);
+  static bool GetGetPinsResultFromJSON(const std::string& json,
+                                       ipfs::GetPinsResult* result);
+  static bool GetRemovePinsResultFromJSON(


I think returning absl::optionalipfs::*PinResult is preferred, let's start with these new functions.

yrliou · 2022-12-08T00:31:56Z

components/ipfs/ipfs_json_parser.cc

+    return false;
+  }
+
+  const base::Value* pins_arr = records_v->FindKey("Pins");


const base::Dict* dict = records_v->GetIfDict();
Then we can use dict->FindList and dict->FindInt below.
By using dict->FindList, we don't need to check type explicitly as FindList will return nullptr if it's not list type.

yrliou · 2022-12-08T00:33:00Z

components/ipfs/ipfs_json_parser.cc

+    return false;
+  }
+
+  auto progress = records_v->FindIntKey("Progress");


I believe this one is deprecated and should use Value::Dict::FindList() instead.

yrliou · 2022-12-08T00:46:54Z

components/ipfs/ipfs_json_parser.cc

+    return false;
+  }
+
+  const base::Value* pins_arr = records_v->FindKey("Pins");


yrliou · 2022-12-08T00:55:11Z

components/ipfs/ipfs_json_parser.cc

+    return false;
+  }
+
+  const base::Value* keys = records_v->FindKey("Keys");


const base::Dict* dict = records_v->GetIfDict(); then use dict->FindDict directly.

yrliou · 2022-12-08T00:58:19Z

components/ipfs/ipfs_json_parser.cc

+      VLOG(1) << "Missing Type for " << it.first;
+      return false;
+    }
+    const std::string* type = it.second.FindStringKey("Type");


FindStringKey is deprecated.

components/ipfs/pin/ipfs_pin_rpc_types.h

components/brave_wallet/browser/json_rpc_service.cc

supermassive · 2022-12-12T03:43:19Z

components/ipfs/pin/ipfs_base_pin_service.cc

+IpfsBasePinService::IpfsBasePinService() {}
+
+IpfsBasePinService::~IpfsBasePinService() {}
+


= default for both

@cypt4 Just a friendly reminder, ^ is not resolved yet.

supermassive · 2022-12-12T03:46:20Z

components/ipfs/pin/ipfs_local_pin_service.h

+ private:
+  void OnAddPinResult(bool status, absl::optional<AddPinResult> result);
+
+  PrefService* prefs_service_;


raw_ptr for pointer fields in this file

supermassive · 2022-12-12T03:47:35Z

components/ipfs/pin/ipfs_pin_rpc_types.h

+  ~AddPinResult();
+  AddPinResult(const AddPinResult&);
+  std::vector<std::string> pins;
+  int progress;


int progress = 0;

supermassive · 2022-12-12T03:55:45Z

components/ipfs/pin/ipfs_local_pin_service.cc

+      base::Value::Dict& update_dict = update->GetDict();
+
+      for (const auto& cid : cids_) {
+        base::Value::List* list = update_dict.FindList(cid);


supermassive · 2022-12-12T03:56:28Z

components/ipfs/pin/ipfs_local_pin_service.cc

+                                    absl::optional<AddPinResult> result) {
+  if (status && result) {
+    for (const auto& cid : cids_) {
+      if (std::find(result->pins.begin(), result->pins.end(), cid) ==


base::Contains

supermassive · 2022-12-12T04:00:23Z

components/ipfs/ipfs_service.cc

+
+  bool success = response.Is2XXResponseCode();
+
+  if (!success) {


just if (!response.Is2XXResponseCode())
same for methods below

supermassive · 2022-12-12T04:06:39Z

components/ipfs/ipfs_service.cc

+  for (const auto& cid : cids) {
+    gurl = net::AppendQueryParameter(gurl, kArgQueryParam, cid);
+  }
+  LOG(ERROR) << "XXZZZ " << gurl.spec();


Y is missing

components/ipfs/ipfs_service.cc

supermassive · 2022-12-12T04:17:16Z

components/ipfs/ipfs_service.cc

+      "POST", gurl, std::string(), std::string(), false,
+      base::BindOnce(&IpfsService::OnPinRemoveResult, base::Unretained(this),
+                     iter, std::move(callback)),
+      {{net::HttpRequestHeaders::kOrigin,


{{net::HttpRequestHeaders::kOrigin, url::Origin::Create(gurl).Serialize()}}

worth an anonymous function to stop copypasting.

supermassive · 2022-12-12T04:23:51Z

components/ipfs/ipfs_service.cc

+
+  ipfs::GetPinsResult result;
+  bool parse_result =
+      IPFSJSONParser::GetGetPinsResultFromJSON(response.body(), &result);


Please use response.value_body() (returns base::Value) after rebasing. #15933 just merged.

same for methods below

supermassive · 2022-12-12T05:01:49Z

components/ipfs/ipfs_service.h

+  using AddPinCallback =
+      base::OnceCallback<void(bool, absl::optional<AddPinResult>)>;
+  using RemovePinCallback =
+      base::OnceCallback<void(bool, absl::optional<RemovePinResult>)>;
+  using GetPinsCallback =
+      base::OnceCallback<void(bool, absl::optional<GetPinsResult>)>;


For these three callbacks let's remove bool argument and consider empty optional as failure

supermassive · 2022-12-12T05:06:36Z

components/ipfs/pin/ipfs_local_pin_service.cc

+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&IpfsLocalPinService::AddGcTask, base::Unretained(this)),
+      base::Minutes(1));


I think there is no guarantee that after a minute delay from start IpfsLocalPinService is always alive. We might get browser shutdown crash here.

supermassive · 2022-12-12T05:09:04Z

components/ipfs/pin/ipfs_local_pin_service.cc

+  ipfs_base_pin_service_->AddJob(std::make_unique<AddLocalPinJob>(
+      prefs_service_, ipfs_service_, prefix, cids,
+      base::BindOnce(&IpfsLocalPinService::OnAddJobFinished,
+                     base::Unretained(this), std::move(callback))));
+}


It is quite hard to track how lifetimes of callback and this correlate. Better pass it by weak ptr and be safe.

components/ipfs/ipfs_service.h

supermassive · 2022-12-12T05:24:53Z

components/ipfs/pin/ipfs_base_pin_service.cc

+}
+
+void IpfsBasePinService::OnDaemonStarted() {
+  ipfs_service_->GetConnectedPeers(base::NullCallback(), 2);


Why NullCallback? What is the purpose of GetConnectedPeers call?

To ensure that daemon is fully loaded

Please add a comment then.

components/ipfs/pin/ipfs_base_pin_service.cc

supermassive · 2022-12-12T05:39:58Z

components/brave_wallet/browser/brave_wallet_pin_service.h

+  PrefService* prefs_;
+
+  // JsonRpcService is used to fetch token metadata
+  JsonRpcService* json_rpc_service_;
+  ipfs::IpfsLocalPinService* local_pin_service_;


supermassive · 2022-12-12T06:02:32Z

components/brave_wallet/browser/brave_wallet_auto_pin_service.cc

+
+void BraveWalletAutoPinService::OnTokenRemoved(BlockchainTokenPtr token) {
+  const auto iter =
+      std::remove_if(queue_.begin(), queue_.end(),


base::EraseIf from cxx20_erase_deque.h

supermassive · 2022-12-12T06:03:37Z

components/brave_wallet/browser/brave_wallet_auto_pin_service.cc

+IntentData::IntentData(const BlockchainTokenPtr& token,
+                       Operation operation,
+                       absl::optional<std::string> service)
+    : token(token.Clone()), operation(operation), service(service) {}


service(std::move(service))

supermassive · 2022-12-12T06:04:34Z

components/brave_wallet/browser/brave_wallet_auto_pin_service.cc

+
+void BraveWalletAutoPinService::Restore() {
+  brave_wallet_service_->GetUserAssets(
+      mojom::kMainnetChainId, mojom::CoinType::ETH,


Is it intentional to get assets for mainnet always?

I've added new GetAllUserAssets

components/brave_wallet/browser/brave_wallet_auto_pin_service.cc

supermassive · 2022-12-12T07:27:11Z

components/brave_wallet/browser/brave_wallet_auto_pin_service.h

+
+namespace brave_wallet {
+
+enum Operation { ADD = 0, DELETE = 1, VALIDATE = 2 };


Per style guide please use kAdd, kDelete and kValidate

supermassive · 2022-12-12T07:30:02Z

components/brave_wallet/browser/brave_wallet_auto_pin_service.cc

+  if (!result) {
+    AddOrExecute(std::make_unique<IntentData>(current_->token, Operation::ADD,
+                                              current_->service));
+  }


Should that be if (result) ?

We re-add token if it is corrupted here

supermassive · 2022-12-12T07:33:41Z

components/brave_wallet/browser/brave_wallet_pin_service.h

+  virtual mojom::TokenPinStatusPtr GetTokenStatus(
+      absl::optional<std::string> service,
+      const mojom::BlockchainTokenPtr& token);
+  virtual absl::optional<base::Time> GetLastValidateTime(
+      absl::optional<std::string> service,
+      const mojom::BlockchainTokenPtr& token);
+  virtual std::set<std::string> GetTokens(
+      const absl::optional<std::string>& service);


const absl::optionalstd::string& service

supermassive · 2022-12-12T07:36:56Z

components/brave_wallet/browser/brave_wallet_pin_service.cc

+  if (!service) {
+    local_pin_service_->ValidatePins(
+        GetPath(absl::nullopt, token), cids.value(),
+        base::BindOnce(&BraveWalletPinService::OnTokenValidated,
+                       base::Unretained(this), service, std::move(callback),
+                       std::move(token)));
+  }
+}


Is this ok we don't do anything if there as service string? callback would not be resolved then.

supermassive · 2022-12-12T07:39:30Z

components/brave_wallet/browser/brave_wallet_pin_service.cc

+  if (!service) {
+    local_pin_service_->AddPins(
+        GetPath(service, token), cids,
+        base::BindOnce(&BraveWalletPinService::OnTokenPinned,
+                       base::Unretained(this), absl::nullopt,
+                       std::move(callback), std::move(token)));
+  }
+}


Also losing callback if there is a service.

supermassive · 2022-12-12T07:43:23Z

components/brave_wallet/browser/brave_wallet_pin_service.cc

+  cids.push_back(ExtractCID(token_url).value());
+  auto* image = parsed_result->FindStringKey("image");
+  if (image) {
+    cids.push_back(ExtractCID(*image).value());
+  }


Is it safe to call .value() here? It is unclear where validation happens.

supermassive · 2022-12-12T07:54:32Z

components/brave_wallet/browser/brave_wallet_pin_service.cc

+  return base::StrCat({kNftPart, ".", service.value_or(kLocalService), ".",
+                       base::NumberToString(static_cast<int>(token->coin)), ".",
+                       token->chain_id, ".", token->contract_address, ".",
+                       token->token_id});


(same for BraveWalletPinService::GetTokens)
Is it safe to make pref path this way? What if any part of it has a dot?
Do we really need keep such hierarchy in prefs? Maybe just an array of {service, coin, chain_id, etc...} ?

It is needed some times to resolve tokens by group like service where they are pinned.
I've added protection in GetPath method

@cypt4 Note that token_id might be an empty string for other NFTs that's not ERC721, do we need to have this detailed hierarchy? Are all service, coin, chain_id, contract_address, token_id needs to be in the key path or we only need a few of them like service for grouping?

supermassive · 2022-12-12T08:03:19Z

components/brave_wallet/browser/brave_wallet_pin_service.cc

+    return mojom::TokenPinStatusCode::STATUS_PINNED;
+  } else if (status == "pinning_in_progress") {
+    return mojom::TokenPinStatusCode::STATUS_PINNING_IN_PROGRESS;
+  } else if (status == "unpining_in_progress") {


Everywhere in the file pining->pinning

supermassive · 2022-12-22T03:29:32Z

components/ipfs/pin/ipfs_local_pin_service.cc

+    DictionaryPrefUpdate update(prefs_service_, kIPFSPinnedCids);
+    base::Value::Dict& update_dict = update->GetDict();
+
+    bool verification_pased = true;


yrliou · 2023-01-23T17:26:26Z

app/brave_settings_strings.grdp

+  <message name="IDS_SETTINGS_IPFS_NFT_AUTO_PINNING_ENABLED_LABEL" desc="The text label for NFT auto-pinning to IPFS feature.">
+    Automatically pin NFTs to the local IPFS node
+  </message>
+  <message name="IDS_SETTINGS_IPFS_NFT_AUTO_PINNING_ENABLED_DESC" desc="The description for IPFS companion switch in settings">


Content of desc seems wrong here?

browser/resources/settings/brave_ipfs_page/brave_ipfs_page.html

components/brave_wallet/browser/brave_wallet_auto_pin_service.cc

components/brave_wallet/browser/brave_wallet_pin_service.cc

yrliou · 2023-01-24T03:56:41Z

components/brave_wallet/browser/brave_wallet_pin_service.cc

+  return base::StrCat({kNftPart, ".", service.value_or(kLocalService), ".",
+                       base::NumberToString(static_cast<int>(token->coin)), ".",
+                       token->chain_id, ".", token->contract_address, ".",
+                       token->token_id});


@cypt4 Note that token_id might be an empty string for other NFTs that's not ERC721, do we need to have this detailed hierarchy? Are all service, coin, chain_id, contract_address, token_id needs to be in the key path or we only need a few of them like service for grouping?

components/brave_wallet/common/brave_wallet.mojom

Resolves brave/brave-browser#19283

stoletheminerals

Just to clarify some IPFS basics. When pinning files that are already present on IPFS and has a CID we don't actually fetch them in the browser process, but it happens in the Kubo daemon? Should we check that the files we pin are images and not something else? If we do so can you point me to where in the code it happens?

stoletheminerals · 2023-02-09T23:30:10Z

components/brave_wallet/browser/brave_wallet_pin_service.cc

+    const std::string& path) {
+  std::vector<std::string> parts =
+      base::SplitString(path, ".", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+  if (parts.size() != 6) {


Why do we store data like this instead of using something more common (e.g. JSON)? Is it specific to chromium?

Yes we store a lot of things in the PrefsService, like saved accounts, assets, NFTs. Using raw JSON means we should have some sort of transactions for write operations. PrefsService already has such mechanism.

stoletheminerals · 2023-02-09T23:50:36Z

components/brave_wallet/browser/brave_wallet_pin_service.cc

+                       weak_ptr_factory_.GetWeakPtr(), service,
+                       std::move(callback), std::move(token)));
+  } else {
+    // Remote pinning not implemented yet


Do we locally pin files that are already remotely pinned (by Pinata for example)?

Yes, NFTs from OpenSea or other NFT platforms are most likely are pinned in some way. But they could be unpinned in any moment.

stoletheminerals · 2023-02-10T05:24:06Z

components/brave_wallet/browser/brave_wallet_pin_service.cc

+  if (image) {
+    auto image_cid = ExtractCID(*image);
+    if (image_cid) {
+      cids.push_back(image_cid.value());


Do we check if the file we pin is an image?

bridiver · 2024-07-17T21:10:26Z

browser/profiles/brave_profile_manager.cc

 #if BUILDFLAG(ENABLE_IPFS)
+  brave_wallet::BraveWalletAutoPinServiceFactory::GetServiceForContext(profile);


Why are these being called here in the first place? You should be using BrowserContextKeyedServiceFactory:: ServiceIsCreatedWithContext

bridiver · 2024-07-17T21:12:54Z

browser/profiles/brave_profile_manager.cc

@@ -44,6 +44,7 @@
 #endif

 #if BUILDFLAG(ENABLE_IPFS)


why is a brave wallet service guarded by enabled_ipfs, they are not guarded by enable_ipfs in the BUILD.gn

header buildflags and gn buildflags should always match regardless of whether the code is only use when some other buildflag is enabled

cypt4 force-pushed the brave_19283_2 branch 3 times, most recently from af506e2 to fa5f781 Compare November 27, 2022 12:34

cypt4 marked this pull request as ready for review November 29, 2022 09:58

cypt4 requested review from a team as code owners November 29, 2022 09:58

muliswilliam approved these changes Nov 29, 2022

View reviewed changes

yrliou assigned cypt4 Nov 30, 2022

yrliou self-requested a review November 30, 2022 19:22

cypt4 force-pushed the brave_19283_2 branch 3 times, most recently from ba27385 to 3886be5 Compare December 2, 2022 22:20

supermassive self-requested a review December 8, 2022 03:59

yrliou reviewed Dec 8, 2022

View reviewed changes

supermassive reviewed Dec 12, 2022

View reviewed changes

components/brave_wallet/browser/json_rpc_service.cc Show resolved Hide resolved

supermassive reviewed Dec 12, 2022

View reviewed changes

cypt4 force-pushed the brave_19283_2 branch from 3886be5 to 9720ce6 Compare December 19, 2022 13:10

cypt4 requested review from yrliou and supermassive December 20, 2022 21:01

supermassive reviewed Dec 22, 2022

View reviewed changes

cypt4 force-pushed the brave_19283_2 branch 2 times, most recently from 8fdd79a to 077c9eb Compare December 22, 2022 12:17

yrliou reviewed Jan 24, 2023

View reviewed changes

cypt4 mentioned this pull request Jan 24, 2023

NFT Pinning for Solana tokens brave/brave-browser#28000

Closed

cypt4 added 14 commits January 25, 2023 01:56

Add pin-related RPC calls to IpfsService

eb47509

Resolves brave/brave-browser#19283

Add IpfsLocalPinService

5c4bb37

Add BraveWalletPinService

82b5ada

Add BraveWalletAutoPinService

426bacc

Add bindings to WebUI

478a82b

Fix build

e1e7517

Fix build

46940d2

Build&review fix

2397d3e

Review fix

a79e00c

Review fix

8c436f2

Add method to resolve local node status

f0ce50a

Fix builds

737e96c

Review fix

0a34eed

Review fix

411a98b

cypt4 force-pushed the brave_19283_2 branch from b1bf679 to 1efc302 Compare January 24, 2023 22:15

Review fix

be3bd52

cypt4 requested a review from yrliou January 24, 2023 22:29

cypt4 force-pushed the brave_19283_2 branch from 1efc302 to be3bd52 Compare January 24, 2023 22:29

yrliou approved these changes Jan 25, 2023

View reviewed changes

cypt4 merged commit dac3157 into master Jan 25, 2023

cypt4 deleted the brave_19283_2 branch January 25, 2023 14:35

github-actions bot added this to the 1.49.x - Nightly milestone Jan 25, 2023

kylehickinson mentioned this pull request Jan 25, 2023

Move IPFS pinning related code behind local node build flag #16866

Closed

25 tasks

cypt4 mentioned this pull request Jan 26, 2023

Revert "Core part for local nft pinning" #16871

Closed

yrliou mentioned this pull request Feb 3, 2023

NFT Pinning for ERC1155 NFTs brave/brave-browser#28267

Closed

stoletheminerals reviewed Feb 10, 2023

View reviewed changes

bridiver reviewed Jul 17, 2024

View reviewed changes

		IpfsBasePinService::IpfsBasePinService() {}

		IpfsBasePinService::~IpfsBasePinService() {}


		namespace brave_wallet {

		enum Operation { ADD = 0, DELETE = 1, VALIDATE = 2 };

		#if BUILDFLAG(ENABLE_IPFS)
		brave_wallet::BraveWalletAutoPinServiceFactory::GetServiceForContext(profile);

Core part for local nft pinning #16073

Core part for local nft pinning #16073

Conversation

cypt4 commented Nov 23, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

github-actions bot commented Nov 29, 2022

yrliou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoletheminerals left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypt4 commented Nov 23, 2022 •

edited

Loading

bridiver Jul 17, 2024 •

edited

Loading