NFT pinning improvements #17983

cypt4 · 2023-04-10T20:45:08Z

Update wallet NFT pinning state if setting changes
Resolves brave/brave-browser#29354

Don't show pinning status if autopin is disabled
Resolves brave/brave-browser#29553

Improve NFT pinning queue
Resolves brave/brave-browser#29551

Fixes pinning after wallet reset
brave/brave-browser#29207

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 wiki
- npm run lint, npm run presubmit wiki, 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:

Switch autopin setting on the settings page - check that feature status is changed on the wallet page ( Disabling Automatically pin NFTs settings doesn't update the IPFS banner on portfolio/nft page brave-browser#29354)
Check that pinning statuses are hidden when setting is off ( Clearing pinned NFT still shows NFT data is being pinned to your local IPFS node on NFT details screen brave-browser#29553)
Check case described in Adding NFT from the same collection doesn't show the pinning state until browser restart brave-browser#29551
Check case described in NFT pinning not started after wallet reset until browser is restarted brave-browser#29207

brave-builds · 2023-04-10T21:27:01Z

A Storybook has been deployed to preview UI for the latest push

muliswilliam

LGTM

supermassive · 2023-04-11T09:33:19Z

components/brave_wallet/browser/brave_wallet_auto_pin_service.cc

  pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
  pref_change_registrar_->Init(pref_service_);
  pref_change_registrar_->Add(
      kAutoPinEnabled,
      base::BindRepeating(&BraveWalletAutoPinService::OnAutoPinStatusChanged,
-                          weak_ptr_factory_.GetWeakPtr()));
+                          base::Unretained(this)));


Needs another weak_ptr_factory for invalidating in OnAutoPinStatusChanged

supermassive · 2023-04-11T09:33:49Z

components/brave_wallet/browser/brave_wallet_auto_pin_service.cc

+  auto intent =
+      std::make_unique<IntentData>(token, Operation::kAdd, absl::nullopt);
+  AddOrExecute(std::move(intent));


No need in temp variable here and below

supermassive · 2023-04-11T09:34:37Z

components/brave_wallet/browser/brave_wallet_auto_pin_service.cc

+    if (v->token == data->token && v->service == data->service &&
+        v->operation == data->operation) {
      return;
    }
  }
  if (current_ && current_->token == data->token &&
-      current_->service == data->service) {
+      current_->service == data->service &&
+      current_->operation == data->operation) {


Duplicated equality check algorithm

supermassive · 2023-04-11T09:34:46Z

components/brave_wallet/browser/brave_wallet_auto_pin_service.cc

+
+  if (data->operation == Operation::kAdd ||
+      data->operation == Operation::kValidate) {
+    if (tokens_.find(data->token) == tokens_.end()) {


base::Contains

supermassive · 2023-04-11T09:36:11Z

components/brave_wallet_ui/components/desktop/views/portfolio/portfolio-asset.tsx

    if (isNftAsset && selectedAsset && nftPinnable) {
      return getNftPinningStatus(selectedAsset)
    }
    return undefined
-  }, [nftPinnable, isNftAsset, selectedAsset, nftPinningStatus])
+  }, [nftPinnable, isNftAsset, selectedAsset, nftPinningStatus, isAutoPinEnabled])


nftPinningStatus is not referenced in useMemo body, need it here?

getNftPinningStatus is dependent on nftPinningStatus i guess

supermassive

LGTM with minor issues to fix

brave-builds · 2023-04-12T11:45:33Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-04-12T14:03:32Z

A Storybook has been deployed to preview UI for the latest push

Resolves brave/brave-browser#29551

Resolves brave/brave-browser#29553

Resolves brave/brave-browser#29354

Keeping known tokens in str form

brave-builds · 2023-04-13T19:33:16Z

A Storybook has been deployed to preview UI for the latest push

srirambv · 2023-04-17T08:51:56Z

Verification passed on

Brave	1.52.47 Chromium: 112.0.5615.121 (Official Build) nightly (64-bit)
Revision	`39cc4e45904ae9f1741c4fbaa866e629c96f2268-refs/branch-heads/5615_51@{#8}`
OS	Windows 11 Version 22H2 (Build 22621.1555)

Verified the following issues

https://github.com/brave/brave-browser/issues/29354 - Passed

Verified disabling Automatically pin NFTs setting auto-updates the IPFS banner on Portfolio/NFT page

17983-1.mp4

https://github.com/brave/brave-browser/issues/29553 - Passed

Verified clearing pinned NFT doesn't show any pin status on the NFT

17983-2.mp4

https://github.com/brave/brave-browser/issues/29551 - Passed

Verified adding an NFT from the same collection automatically shows the pinning status and doesn't require browser restart

17983-3.mp4

https://github.com/brave/brave-browser/issues/29207 - Passed

Verified after resetting wallet newly added NFT in new wallet automatically starts pinning if the auto pin nft setting is enabled.

17983-4.mp4

Uplift of #17983 (squashed) to beta

cypt4 requested review from a team as code owners April 10, 2023 20:45

github-actions bot assigned cypt4 Apr 10, 2023

github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet feature/web3/wallet/core labels Apr 10, 2023

cypt4 force-pushed the brave_29493_1 branch from 4595d61 to 3cea9a8 Compare April 10, 2023 20:51

muliswilliam approved these changes Apr 11, 2023

View reviewed changes

supermassive reviewed Apr 11, 2023

View reviewed changes

supermassive approved these changes Apr 11, 2023

View reviewed changes

cypt4 added 7 commits April 14, 2023 01:13

Improve NFT pinning queue

cdf57f2

Resolves brave/brave-browser#29551

Don't show pinning status if autopin is disabled

4c7e214

Resolves brave/brave-browser#29553

Update wallet NFT pinning state if setting changes

80e1e2d

Resolves brave/brave-browser#29354

Additional queue improvements

b8ad14b

Review fix

bc678a7

Add tests

0fab09f

Keeping known tokens in str form

Reset IpfsBasePinService

75379ce

cypt4 force-pushed the brave_29493_1 branch from d169877 to 75379ce Compare April 13, 2023 18:14

linhkikuchi merged commit 0f56b39 into master Apr 14, 2023

linhkikuchi deleted the brave_29493_1 branch April 14, 2023 08:56

github-actions bot added this to the 1.52.x - Nightly milestone Apr 14, 2023

brave-builds mentioned this pull request Apr 14, 2023

NFT pinning improvements (uplift to 1.51.x) #18068

Merged

7 tasks

brave-builds added a commit that referenced this pull request Apr 14, 2023

Uplift of #17983 (squashed) to beta

258f70d

kjozwiak pushed a commit that referenced this pull request Apr 18, 2023

NFT pinning improvements (uplift to 1.51.x) (#18068)

48e3a6d

Uplift of #17983 (squashed) to beta

kjozwiak mentioned this pull request Apr 18, 2023

NFT pinning not started after wallet reset until browser is restarted brave/brave-browser#29207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NFT pinning improvements #17983

NFT pinning improvements #17983

cypt4 commented Apr 10, 2023 •

edited

Loading

brave-builds commented Apr 10, 2023

muliswilliam left a comment

supermassive Apr 11, 2023

cypt4 Apr 13, 2023

supermassive Apr 11, 2023

cypt4 Apr 13, 2023

supermassive Apr 11, 2023

cypt4 Apr 13, 2023

supermassive Apr 11, 2023

cypt4 Apr 13, 2023

supermassive Apr 11, 2023

cypt4 Apr 13, 2023

supermassive left a comment

brave-builds commented Apr 12, 2023

brave-builds commented Apr 12, 2023

brave-builds commented Apr 13, 2023

srirambv commented Apr 17, 2023

NFT pinning improvements #17983

NFT pinning improvements #17983

Conversation

cypt4 commented Apr 10, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Apr 10, 2023

muliswilliam 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

supermassive left a comment

Choose a reason for hiding this comment

brave-builds commented Apr 12, 2023

brave-builds commented Apr 12, 2023

brave-builds commented Apr 13, 2023

srirambv commented Apr 17, 2023

cypt4 commented Apr 10, 2023 •

edited

Loading