Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable NFT Pinning by default #17758

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Enable NFT Pinning by default #17758

merged 1 commit into from
Mar 29, 2023

Conversation

cypt4
Copy link
Collaborator

@cypt4 cypt4 commented Mar 24, 2023

Resolves brave/brave-browser#29017

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:

@cypt4
Copy link
Collaborator Author

cypt4 commented Mar 28, 2023

/build macos

@cypt4 cypt4 requested a review from yrliou March 28, 2023 20:03
@cypt4 cypt4 merged commit 087c362 into master Mar 29, 2023
@cypt4 cypt4 deleted the brave_29017 branch March 29, 2023 17:33
@github-actions github-actions bot added this to the 1.52.x - Nightly milestone Mar 29, 2023
brave-builds added a commit that referenced this pull request Mar 31, 2023
@srirambv
Copy link
Contributor

srirambv commented Apr 6, 2023

Verification passed on

Brave 1.52.17 Chromium: 112.0.5615.49 (Official Build) nightly (64-bit)
Revision bd2a7bcb881c11e8cfe3078709382934e3916914-refs/branch-heads/5615@{#936}
OS Windows 11 Version 22H2 (Build 22621.1485)

Verified test plan from brave-browser#29017

Enabling the feature - Passed
  • Verified brave://flags/#enable-nft-pinning is set to Default
  • Verified adding an NFT to visible asset list, automatically shows the NFT pinning banner
  • Verified clicking on Learn more on the banner loads interstitial page brave://wallet/crypto/local-ipfs-node
  • Verified clicking on Check which NFT's of mine can be pinned loads second interstitial page brave://wallet/crypto/inspect-nfts
  • Verified clicking on Keep my NFTs always online starts IPFS on localhost and start pinning the available NFT's
  • Verified when Keep my NFTs always online is selected, setting Automatically pin NFT's is enabled, default is disabled
Pinning - Passed
  • Verified able to pin NFTs
  • Verified when pinning is in process, shows message banner message about being pinned in local node
  • Verified when successfully pinned, show message about number of NFTs that are pinned
  • Additional verification notes for banner messages and pinning data recursively
Unpinning - Passed
  • Verified after removing an NFT and running curl http://127.0.0.1:<api port could be viewed at chrome://ipfs-internal>/api/v0/pin/ls?type=recursive, shows {"Keys":{}} message
    image
Pins Validation - Passed
  • Verified deleting brave_ifps folder and moving date ahead and restarting the browser restores brave_ipfs folder again
  • Verified resetting device date back to current date after restart starts to pin the NFT's again.
  • Running curl http://127.0.0.1:<api port could be viewed at chrome://ipfs-internal>/api/v0/pin/ls?type=recursive continues to show the pinned NFT details
Disable feature- Passed
  • Verified disabling Automatically pin NFTs setting and reloading the NFT tab shows the onboarding banner message
  • Verified manually deleting the NFT's from visible asset list also removes the onboarding banner message
Cleanup- Passed
  • Verified disabling Automatically pin NFTs setting, a new setting for Clear pinned NFTs is shown
  • Verified clicking that clears the NFTs pinned state and shows onboarding banner message
  • Encountered #29553 & #29554
Local node behaviour- Passed
  • Verified when user click on Keep my NFTs always online from interstitial page, IPFS local node is installed and started.
Error handling- Passed
  • Verified if NFT's content-type is not of the format image/*, NFT pinning fails. Verified as part of brave-browser#28775 (comment)
  • Verified due to network issues NFT pinning fails, it starts the pinning process again when browser is restarted

kjozwiak pushed a commit that referenced this pull request Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable NFT pinning flag
3 participants