Fix NFT visibility in BottomSheet fragment #20361

simoarpe · 2023-10-03T08:42:48Z

Resolves brave/brave-browser#31915
Resolves brave/brave-browser#33255

Details

Fixes an issue where NFTs listed in EditVisibleAssetsBottomSheetDialogFragment were always marked as visible.

Screen recording showing the right behavior:

fix-NFT-visibility.mov

⚠️ Notes for Reviewers

The approach used to mark an asset as visible or invisible is going to be rewritten soon.
The new way will leverage a WebUI based on Desktop implementation and will (probably) rely on the same persistency methods if compatible with Android.
Because we don't want to reinvent the wheel, the actual fix is just a simple check on the NFT data model returned from core API and is implemented by the commit 👉 357eaf4
After the transition to a WebUI the state will be handled on the client side and not from core.

Other Notable Changes

NetworkUtils#findNetwork has been improved with a new parameter as localhost networks now can belong to FIL, SOL or ETH and on debug builds was returning wrong values.
Improved filterTokens method: the filtering now uses Java streams, and it's (roughly) 50% faster as the previous logic was converting to/from array/list multiple times.
Added Javadoc to TokenUtils methods.

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
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 Brave Wallet and navigate to NFT section by tapping on the selector to the right.
If not present, add some NFTs by tapping on the plus icon.
Tap on "Edit visible NFTs" and deselect all NFTs.
Observe the full list is shown in the Hidden section.
Tap on "Edit visible NFTs" and deselect all NFTs.
Select all the available NFTs.
Observe the full list is shown in the NFTs section.
Observe the Hidden section is empty.

Add extra parameter to NetworkUtils#findNetwork method in order to distinguish localhost chain Ids that can belong to ETH, SOL, or FIL. Without the extra parameter symbol, the method was producing wrong network info pointing always to Solana chain Id on localhost also for Ethereum and Filecoin.

simoarpe · 2023-10-04T08:41:03Z

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

-                        } else if (mType == WalletCoinAdapter.AdapterType.SEND_ASSETS_LIST) {
-                            assert mSelectedNetwork != null;
-                            TokenUtils.getUserAssetsFiltered(mBraveWalletService, mSelectedNetwork,
-                                    mSelectedNetwork.coin, TokenUtils.TokenType.ALL, tokens -> {
-                                        _mAssetsResult.postValue(new AssetsResult(
-                                                Arrays.asList(tokens), Collections.emptyList()));
-                                    });
-                        } else if (mType == WalletCoinAdapter.AdapterType.BUY_ASSETS_LIST) {
-                            TokenUtils.getBuyTokensFiltered(mBlockchainRegistry, mSelectedNetwork,
-                                    TokenUtils.TokenType.ALL, tokens -> {
-                                        _mAssetsResult.postValue(new AssetsResult(
-                                                Arrays.asList(tokens), Collections.emptyList()));
-                                    });


SEND_ASSETS_LIST and BUY_ASSETS_LIST can be safely removed as they were never accessed: these parts now rely on WebUIs.

SergeyZhukovsky

++

Pavneet-Sing · 2023-10-05T11:46:49Z

The approach used to mark an asset as visible or invisible is going to be rewritten soon.
The new way will leverage a WebUI based on Desktop implementation and will (probably) rely on the same persistency methods if compatible with Android.
Because we don't want to reinvent the wheel, the actual fix is just a simple check on the NFT data model returned from core API and is implemented by the commit 👉 357eaf4
After the transition to a WebUI the state will be handled on the client side and not from core.

We don't need to maintain anything or reinvent(as it is a bug). Token state is definitely handled on the core side, not on client side. This seems like a regression for assets/tokens, but NFT visibility is completely depends on isVisible flag so this is the accurate and only required check to determine NFT visibility.

About tokens visibility issue, It is working as expected for non-native tokens. The issue with native token visibility is that the native token always seems to be added by Stream.concat(Stream.of(nativeAsset), tokenStream); and that's why it is always visible.

assets.webm

Anyway, We will come back to these native changes/issues later if needed after completing webUI issues as priority.

...omium/chrome/browser/crypto_wallet/fragments/EditVisibleAssetsBottomSheetDialogFragment.java

srirambv · 2023-10-06T10:02:39Z

Verification passed on Oppo Reno 5 with Android 13 running 1.61.3 x64 Nightly build

Verified steps from PR
Verified unchecking NFTs moves them into hidden tab
Verified checking NFTs moves them back into collected tab

20361.mp4

* Uplift of #20361 (squashed) to beta * Fix NPE when accessing null network object --------- Co-authored-by: Simone Arpe <simon.arpe@gmail.com>

* Uplift of #20361 (squashed) to release * Fix NPE when accessing null network object --------- Co-authored-by: Simone Arpe <simon.arpe@gmail.com>

simoarpe added 12 commits September 25, 2023 12:39

Perform minor refactoring

abc3c24

Apply code formatting

f799044

Improve error handling

87a623a

Remove old unused code

b859022

Refactor isSameToken method

13d027c

Introduce stream for faster filtering and avoid multiple convertions

ac3aae2

Refactor TokenUtils add Javadoc and remove unused parameters

639370d

Remove old refuse code

3a07807

Apply code formatting

761bbd5

Add Javadoc to TokenUtils methods

31455c2

Apply code formatting

3239bf8

simoarpe added CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x64 Do not run CI builds for Windows x64 unused-CI/skip-linux-x64 Do not run CI builds for Linux x64 labels Oct 3, 2023

simoarpe self-assigned this Oct 3, 2023

github-actions bot added the feature/web3/wallet label Oct 3, 2023

simoarpe added 3 commits October 3, 2023 11:30

Improve logic to find network asset and prevent NPE

fdbd6e5

Apply code formatting

fa112ab

Fix NFT visibility

357eaf4

simoarpe commented Oct 4, 2023

View reviewed changes

simoarpe requested review from Pavneet-Sing and SergeyZhukovsky October 4, 2023 08:43

SergeyZhukovsky approved these changes Oct 4, 2023

View reviewed changes

Pavneet-Sing approved these changes Oct 5, 2023

View reviewed changes

...omium/chrome/browser/crypto_wallet/fragments/EditVisibleAssetsBottomSheetDialogFragment.java Show resolved Hide resolved

simoarpe merged commit fa85d6a into master Oct 5, 2023
6 checks passed

simoarpe deleted the simone/fix-assets-visibility branch October 5, 2023 12:42

github-actions bot added this to the 1.61.x - Nightly milestone Oct 5, 2023

simoarpe mentioned this pull request Oct 5, 2023

(Wallet) Fix NPE when updating local network in network selector model class brave/brave-browser#33440

Closed

simoarpe restored the simone/fix-assets-visibility branch October 6, 2023 08:08

brave-builds mentioned this pull request Oct 6, 2023

Fix NFT visibility in BottomSheet fragment (uplift to 1.60.x) #20427

Merged

7 tasks

brave-builds added a commit that referenced this pull request Oct 6, 2023

Uplift of #20361 (squashed) to beta

8e3ede8

brave-builds added a commit that referenced this pull request Oct 6, 2023

Uplift of #20361 (squashed) to release

76d22da

brave-builds mentioned this pull request Oct 6, 2023

Fix NFT visibility in BottomSheet fragment (uplift to 1.59.x) #20428

Merged

7 tasks

kjozwiak pushed a commit that referenced this pull request Oct 6, 2023

Fix NFT visibility in BottomSheet fragment (uplift to 1.60.x) (#20427)

9d4de0e

* Uplift of #20361 (squashed) to beta * Fix NPE when accessing null network object --------- Co-authored-by: Simone Arpe <simon.arpe@gmail.com>

srirambv mentioned this pull request Oct 9, 2023

Hide/unhide NFT doesn't work as expected brave/brave-browser#33255

Closed

bsclifton deleted the simone/fix-assets-visibility branch November 10, 2023 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NFT visibility in BottomSheet fragment #20361

Fix NFT visibility in BottomSheet fragment #20361

simoarpe commented Oct 3, 2023 •

edited by kjozwiak

Loading

simoarpe Oct 4, 2023

SergeyZhukovsky left a comment

Pavneet-Sing commented Oct 5, 2023 •

edited

Loading

srirambv commented Oct 6, 2023

Fix NFT visibility in BottomSheet fragment #20361

Fix NFT visibility in BottomSheet fragment #20361

Conversation

simoarpe commented Oct 3, 2023 • edited by kjozwiak Loading

Details

⚠️ Notes for Reviewers

Other Notable Changes

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

simoarpe Oct 4, 2023

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

Pavneet-Sing commented Oct 5, 2023 • edited Loading

srirambv commented Oct 6, 2023

simoarpe commented Oct 3, 2023 •

edited by kjozwiak

Loading

Pavneet-Sing commented Oct 5, 2023 •

edited

Loading