(Wallet) Implement NFT asset details screen #16671

simoarpe · 2023-01-13T18:20:34Z

Resolves brave/brave-browser#27276

Implements new NFT asset detail screen supporting GIF, BMP, PNG and JPG images.

demo.mov

Security Review

Requested security review: https://github.com/brave/security/issues/1168.

Light Theme	Dark Theme

Related Issues

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:

Open the wallet section
If not already present, add a new NFT selecting "Edit visible assets" > "Add custom assets"
(E.g 0x59468516a8259058bad1ca5f8f4bff190d30e066, id 183 - Invisible Friends)
Tap on the NFT item
Observe the new NFT detail screen loads correctly image, title and description.

android/java/org/chromium/chrome/browser/app/domain/PortfolioModel.java

android/java/AndroidManifest.xml

android/java/org/chromium/chrome/browser/crypto_wallet/activities/NftDetailActivity.java

android/java/res/layout/activity_nft_detail.xml

SergeyZhukovsky

check lint, but ++ otherwise

diracdeltas · 2023-01-14T01:49:39Z

Is there a security review for this? Particularly SVG support. CC @kdenhartog

simoarpe · 2023-01-16T10:56:35Z

@diracdeltas, requested a security review: https://github.com/brave/security/issues/1168.

android/java/org/chromium/chrome/browser/crypto_wallet/activities/NftDetailActivity.java

browser/ui/android/strings/android_brave_strings.grd

android/java/org/chromium/chrome/browser/crypto_wallet/util/Utils.java

android/java/org/chromium/chrome/browser/crypto_wallet/fragments/PortfolioFragment.java

android/java/org/chromium/chrome/browser/crypto_wallet/util/AndroidUtils.java

android/java/org/chromium/chrome/browser/crypto_wallet/util/Utils.java

android/java/res/layout/activity_nft_detail.xml

Pavneet-Sing · 2023-01-17T10:15:45Z

Is there a security review for this? Particularly SVG support. CC @kdenhartog

We haven't added SVG support with any new lib yet so no security review is needed for now.
It will/should be opened with the implementation of

(Wallet) Add SVG support to NFT images

cc: @diracdeltas @simoarpe

android/java/org/chromium/chrome/browser/crypto_wallet/fragments/PortfolioFragment.java

kdenhartog

One non blocker from me and I see you've addressed the concern about arbitrary URL file types by limiting this to Network and Data URLs only. LGTM

kdenhartog · 2023-01-20T01:12:56Z

android/java/org/chromium/chrome/browser/crypto_wallet/activities/NftDetailActivity.java

+                    String networkName = defaultNetwork.chainName;
+                    mNetworkNameView.setText(networkName);
+                    String blockExplorerUrl = defaultNetwork.blockExplorerUrls.length > 0
+                            ? defaultNetwork.blockExplorerUrls[0]


Non blocking: is there a reason this needs to be an array if we're just grabbing the first index here always?

It's a Mojom generated NetworkInfo class to get response data of network as an object from backend so it cannot be changed as it's being used across platforms and since there can be 0,1 or 1+ values so array is the ideal type for this kind of data. Changing this would require changes across platforms, backend and there could be a use case where more than the first url is being used.

That's a very good question!
The blockExplorerUrls is an array because some tokens may have multiple blockchain explorer endpoints, this could be useful in case one endpoint is down. Of course, we should add the required logic on top where we check if an endpoint is down and process the next one if present. (It could be an improvement to implement in the future).

Another constraint to consider is about our current configuration as well: NetworkInfo.java (defaultNetwork) is a Java class generated by the mojo binding generator. So the original type is a struct defined in brave_wallet.mojom here.
Not savvy enough to know how much room we have to modify these mojom files. I'm sure @SergeyZhukovsky could tell us more.

kdenhartog

Spotted one blocker to make sure we're disabling support for http. Also one other non blocker for resolving IPFS content which we could do via a gateway if we don't have support for nodes in Android (which I'm pretty sure is the case)

kdenhartog · 2023-01-20T01:24:55Z

android/java/org/chromium/chrome/browser/crypto_wallet/util/ImageLoader.java

@@ -51,6 +45,6 @@ public static boolean isSvg(String url) {
    }

    private static boolean isValidImgUrl(String url) {
-        return !TextUtils.isEmpty(url);
+        return URLUtil.isDataUrl(url) || URLUtil.isNetworkUrl(url);


Blocker: Just double checked brave-core and noticed that we support only HTTPS here, so going to block to make sure we're not supporting http URLs here.

In case http, we should

Not load the URL

Try replacing it with https before loading?

Okay, I'm going to replace URLUtil.isNetworkUrl(url) with URLUtil.isHttpsUrl(url). Doing so, only HTTPS will be allowed.

Addressed in this commit 👉 66efe7e (#16671)

android/java/org/chromium/chrome/browser/crypto_wallet/util/ImageLoader.java

simoarpe · 2023-01-20T11:40:38Z

@kdenhartog security feedback addressed. Re-requesting your review, thanks.

kdenhartog

Looks good to me. All security concerns have been addressed

Improve performance and (more importantly) fix serialization as inner classes hold a reference to their outer class when non-static.

Makes sure the URL is a data URL that starts with `data:`, or a network URL that starts with `http` or `https`.

Implements new NFT asset detail screen supporting GIF, BMP, PNG and JPG images. (cherry picked from commit 15fd2dc)

srirambv · 2023-01-31T09:13:09Z

Verification passed on Oppo Reno 5 with Android 13 running 1.49.75 x64 Nighty build

Verified NFT asset details view is shown when clicked on the NFT
Verified clicking on the Token ID loads the NFT's page on blockexplorer
Verified able to view both Solana NFT and Ethereum NFT

16671.mp4

* (Wallet) Implement NFT asset details screen (#16671) Implements new NFT asset detail screen supporting GIF, BMP, PNG and JPG images. (cherry picked from commit 15fd2dc) * Add missing import

simoarpe added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Jan 13, 2023

simoarpe requested review from Pavneet-Sing and SergeyZhukovsky January 13, 2023 18:20

simoarpe self-assigned this Jan 13, 2023

simoarpe commented Jan 13, 2023

View reviewed changes

android/java/org/chromium/chrome/browser/app/domain/PortfolioModel.java Show resolved Hide resolved

SergeyZhukovsky reviewed Jan 13, 2023

View reviewed changes

android/java/AndroidManifest.xml Outdated Show resolved Hide resolved

SergeyZhukovsky reviewed Jan 13, 2023

View reviewed changes

android/java/org/chromium/chrome/browser/crypto_wallet/activities/NftDetailActivity.java Outdated Show resolved Hide resolved

SergeyZhukovsky reviewed Jan 13, 2023

View reviewed changes

android/java/org/chromium/chrome/browser/crypto_wallet/activities/NftDetailActivity.java Outdated Show resolved Hide resolved

SergeyZhukovsky reviewed Jan 13, 2023

View reviewed changes

android/java/res/layout/activity_nft_detail.xml Outdated Show resolved Hide resolved

SergeyZhukovsky approved these changes Jan 13, 2023

View reviewed changes

Pavneet-Sing reviewed Jan 17, 2023

View reviewed changes

simoarpe requested a review from Pavneet-Sing January 17, 2023 16:04

Pavneet-Sing reviewed Jan 17, 2023

View reviewed changes

android/java/org/chromium/chrome/browser/crypto_wallet/fragments/PortfolioFragment.java Outdated Show resolved Hide resolved

Pavneet-Sing approved these changes Jan 17, 2023

View reviewed changes

simoarpe force-pushed the simone/nft-detail-screen branch from 71f4d1c to 1b4c20c Compare January 17, 2023 16:41

kdenhartog approved these changes Jan 20, 2023

View reviewed changes

kdenhartog requested changes Jan 20, 2023

View reviewed changes

simoarpe requested a review from kdenhartog January 20, 2023 11:39

kdenhartog approved these changes Jan 23, 2023

View reviewed changes

simoarpe added 3 commits January 24, 2023 10:58

Make inner classes static

9b95377

Improve performance and (more importantly) fix serialization as inner classes hold a reference to their outer class when non-static.

Make ImageLoader helper method more flexible

27e4fef

Add new strings required for NFT Detail screen

0790df7

simoarpe added 12 commits January 24, 2023 10:59

New helper method to calculate screen height

e248e2c

New NFT Detail activity layout

1d56e2d

Add new NftDetailActivity

99e500b

Open NFT Detail activity when NFT is tapped

f09a576

Address review feedback

f6c69d2

Update NftDetailActivity.java

10b2b70

Update NftDetailActivity.java

6b75bae

Apply formatting

aca0eab

Address review feedback

0903fac

Observe NFT data models

5c789e6

Improve URL check

81e7681

Makes sure the URL is a data URL that starts with `data:`, or a network URL that starts with `http` or `https`.

Address security review feedback

b97dc59

simoarpe force-pushed the simone/nft-detail-screen branch from 66efe7e to b97dc59 Compare January 24, 2023 10:01

simoarpe merged commit 15fd2dc into master Jan 24, 2023

simoarpe deleted the simone/nft-detail-screen branch January 24, 2023 12:37

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

simoarpe restored the simone/nft-detail-screen branch January 24, 2023 15:10

simoarpe mentioned this pull request Jan 25, 2023

Implement NFT asset details screen (1.48.x) #16853

Merged

7 tasks

simoarpe added a commit that referenced this pull request Jan 25, 2023

(Wallet) Implement NFT asset details screen (#16671)

9f34c92

Implements new NFT asset detail screen supporting GIF, BMP, PNG and JPG images. (cherry picked from commit 15fd2dc)

simoarpe deleted the simone/nft-detail-screen branch January 31, 2023 11:43

This was referenced Feb 6, 2023

NFT asset details screen brave/brave-browser#27276

Closed

Show NFT section on portfolio brave/brave-browser#27777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Wallet) Implement NFT asset details screen #16671

(Wallet) Implement NFT asset details screen #16671

simoarpe commented Jan 13, 2023 •

edited by srirambv

Loading

SergeyZhukovsky left a comment

diracdeltas commented Jan 14, 2023

simoarpe commented Jan 16, 2023

Pavneet-Sing commented Jan 17, 2023 •

edited

Loading

kdenhartog left a comment

kdenhartog Jan 20, 2023

Pavneet-Sing Jan 20, 2023

simoarpe Jan 20, 2023

kdenhartog left a comment

kdenhartog Jan 20, 2023

Pavneet-Sing Jan 20, 2023

simoarpe Jan 20, 2023

simoarpe Jan 20, 2023

kdenhartog Jan 23, 2023

simoarpe commented Jan 20, 2023

kdenhartog left a comment

srirambv commented Jan 31, 2023

(Wallet) Implement NFT asset details screen #16671

(Wallet) Implement NFT asset details screen #16671

Conversation

simoarpe commented Jan 13, 2023 • edited by srirambv Loading

Resolves brave/brave-browser#27276

Security Review

Related Issues

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

diracdeltas commented Jan 14, 2023

simoarpe commented Jan 16, 2023

Pavneet-Sing commented Jan 17, 2023 • edited Loading

kdenhartog 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

kdenhartog 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

simoarpe commented Jan 20, 2023

kdenhartog left a comment

Choose a reason for hiding this comment

srirambv commented Jan 31, 2023

simoarpe commented Jan 13, 2023 •

edited by srirambv

Loading

Pavneet-Sing commented Jan 17, 2023 •

edited

Loading