Download Brave Wallet component on demand on Android #19086

AlexeyBarabash · 2023-06-28T16:10:39Z

This PR does 3 things:

Prevents automatic registration of Brave Wallet data files component for Android (wallet_data_files_installer.cc);
Performs on demand registration of the component after user presses Get started/Restore button during Wallet onboarding (SetupWalletFragment.onViewCreated)
Does display of the progress of the component download during onboarding screens (DownloadComponentProgressFragment.java)

The PR doesn't do anything with unregister component and cleanup downloaded/unpacked fiels.

Screenshots:

Description	Screenshot
`Secure your crypto` screen
`Backup your wallet` screen
`Your recovery phrase` screen
`Verify recovery phrase`
`Restore crypto account`

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:

I. Normal flow, unlimited fast internet connection

Install Brave
Open brave://components, ensure there is no Brave Wallet data files component
Open Wallet => Get Started => ... you must not see or see for a moment the Download wallet data % label
Open brave://components, ensure there is no Brave Wallet data files component is in list with Up-to-data status.

II. Create wallet flow on slow internet connection

Configure internet speed of the device to be 256 kbit/s, it can be done through WiFi router settings
Install Brave
Open Wallet => Get Started. Expected to have Download wallet data file on Secure your crypto / Backup your wallet / Your recovery phrase / Verify recovery phrase screens

III. Recover wallet flow on slow internet connection

Configure internet speed of the device to be 256 kbit/s, it can be done through WiFi router settings
Install Brave
Open Wallet => Restore. Expected to have Download wallet data file on Restore crypto account screen.

iefremov

chromium_src lgtm

.../browser/crypto_wallet/fragments/onboarding_fragments/DownloadComponentProgressFragment.java

...hromium/chrome/browser/crypto_wallet/fragments/onboarding_fragments/SetupWalletFragment.java

SergeyZhukovsky

++

mkarolin

strings++

simoarpe · 2023-07-28T14:27:42Z

android/java/org/chromium/chrome/browser/component_updater/BraveComponentUpdater.java

+     */
+    public interface ComponentUpdaterListener {
+        // Invoked when component update info has changed.
+        public void onComponentUpdateEvent(int event, String id);


Modifier 'public' is redundant for interface methods and can be safely removed.

Thanks for such a thorough review @simoarpe. PR will be cleaner with your notices.

Fixed at 54818d8#diff-b0ad67a3cc1f46d4b3163f2b99f39d57f940215f0c4a0a327a02bea2cf99f35cR72

simoarpe · 2023-07-28T14:28:22Z

android/java/org/chromium/chrome/browser/component_updater/BraveComponentUpdater.java

+    }
+
+    private final List<ComponentUpdaterListener> mComponentUpdaterListeners =
+            new CopyOnWriteArrayList<ComponentUpdaterListener>();


Explicit type argument ComponentUpdaterListener can be replaced with <>.

fixed at 54818d8#diff-b0ad67a3cc1f46d4b3163f2b99f39d57f940215f0c4a0a327a02bea2cf99f35cR76

simoarpe · 2023-07-28T14:32:39Z

android/java/org/chromium/chrome/browser/component_updater/BraveComponentUpdater.java

+    /**
+     * Class describing the progress of component download.
+     */
+    public class CrxUpdateItem {


Inner class CrxUpdateItem may be static -> public static class CrxUpdateItem

thanks, fixed at 54818d8#diff-b0ad67a3cc1f46d4b3163f2b99f39d57f940215f0c4a0a327a02bea2cf99f35cR98

simoarpe · 2023-07-28T14:33:41Z

android/java/org/chromium/chrome/browser/component_updater/BraveComponentUpdater.java

+            crxUpdateItem.mId = result.getString("id");
+            crxUpdateItem.mDownloadedBytes = (long) result.getDouble("downloaded_bytes");
+            crxUpdateItem.mTotalBytes = (long) result.getDouble("total_bytes");
+            crxUpdateItem.mState = (int) result.getInt("state");


Casting to int is redundant and can be safely removed.

thanks, fixed at 54818d8#diff-b0ad67a3cc1f46d4b3163f2b99f39d57f940215f0c4a0a327a02bea2cf99f35cR145

simoarpe · 2023-07-28T14:36:16Z

android/java/org/chromium/chrome/browser/component_updater/BraveComponentUpdater.java

+            crxUpdateItem.mTotalBytes = (long) result.getDouble("total_bytes");
+            crxUpdateItem.mState = (int) result.getInt("state");
+        } catch (JSONException e) {
+            Log.e(TAG, "getUpdateState JSONException error " + e);


Let's not include the error in the error message. Better pass the entire stacktrace to the appropriate parameter:
Log.e(TAG, "getUpdateState JSONException error", e);

thanks, fixed at 54818d8#diff-b0ad67a3cc1f46d4b3163f2b99f39d57f940215f0c4a0a327a02bea2cf99f35cR147

simoarpe · 2023-07-28T14:36:38Z

android/java/org/chromium/chrome/browser/component_updater/BraveComponentUpdater.java

+        } catch (JSONException e) {
+            Log.e(TAG, "getUpdateState JSONException error " + e);
+        } catch (IllegalStateException e) {
+            Log.e(TAG, "getUpdateState IllegalStateException error " + e);


Same here: Log.e(TAG, "getUpdateState IllegalStateException error", e);

fixed at 54818d8#diff-b0ad67a3cc1f46d4b3163f2b99f39d57f940215f0c4a0a327a02bea2cf99f35cR149

simoarpe · 2023-07-28T14:40:07Z

.../browser/crypto_wallet/fragments/onboarding_fragments/DownloadComponentProgressFragment.java

+ * download. Used on all Brave Wallet Onboarding fragments.
+ */
+public class DownloadComponentProgressFragment extends Fragment {
+    private static final String TAG = "DCPF";


TAG is never used and can be safely removed.

fixed at 54818d8#diff-cc80fe4b95c53c61590521b7c292b734ca565052dda88afa2c5b252ae3889572L28

simoarpe · 2023-07-28T14:45:58Z

.../browser/crypto_wallet/fragments/onboarding_fragments/DownloadComponentProgressFragment.java

+                WalletDataFilesInstaller.getWalletDataFilesComponentId());
+        showHideProgressByItem(updateItem);
+
+        mComponentUpdaterListener = new BraveComponentUpdater.ComponentUpdaterListener() {


Can be replaced with lambda mComponentUpdaterListener = (event, id) -> {

👍 fixed at 54818d8#diff-cc80fe4b95c53c61590521b7c292b734ca565052dda88afa2c5b252ae3889572R64

simoarpe · 2023-07-28T14:49:08Z

.../browser/crypto_wallet/fragments/onboarding_fragments/DownloadComponentProgressFragment.java

+    private ElapsedRealtimeMillisTimer mGracePeriodNoDisplayTimer;
+    private static final long GRACE_NO_DISPLAY_MSEC = 1000;
+
+    public DownloadComponentProgressFragment() {}


Empty constructor can be removed.

yes, fixed at 54818d8#diff-cc80fe4b95c53c61590521b7c292b734ca565052dda88afa2c5b252ae3889572L36

simoarpe · 2023-07-28T14:52:29Z

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

+import org.chromium.base.annotations.CalledByNative;
+
+/**
+ * Class that is used by wallet_data_files_installer.cc to determine, do we need to download


Suggested change

* Class that is used by wallet_data_files_installer.cc to determine, do we need to download

* Class that is used by wallet_data_files_installer.cc to determine if we need to download

thanks, the proposed text sounds better.

fixed at 54818d8#diff-c7e4d99d837ddf5c13bf6fdc72cc6ae4b7b172941084c323bc6a424d0843a0b1R11

simoarpe

👍 Android changes look good!

browser/ui/android/strings/android_brave_strings.grd

android/java/org/chromium/chrome/browser/component_updater/BraveComponentUpdater.java

fixes brave/brave-browser#31824

…internet connection

AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label Jun 28, 2023

AlexeyBarabash self-assigned this Jun 28, 2023

AlexeyBarabash force-pushed the android_wallet_compo_on_demand branch 3 times, most recently from 5db0532 to 630023d Compare June 29, 2023 15:11

AlexeyBarabash force-pushed the android_wallet_compo_on_demand branch 4 times, most recently from fd29334 to 1f7de71 Compare July 17, 2023 13:50

AlexeyBarabash force-pushed the android_wallet_compo_on_demand branch 4 times, most recently from 236a13a to 6e0aaf4 Compare July 24, 2023 19:14

AlexeyBarabash changed the title ~~WIP: download Brave Wallet component on demand on Android~~ Download Brave Wallet component on demand on Android Jul 25, 2023

AlexeyBarabash added the feature/web3/wallet label Jul 25, 2023

AlexeyBarabash marked this pull request as ready for review July 25, 2023 19:32

AlexeyBarabash requested review from a team as code owners July 25, 2023 19:32

AlexeyBarabash removed the CI/skip Do not run CI builds (except noplatform) label Jul 25, 2023

AlexeyBarabash requested review from kdenhartog, Pavneet-Sing, SergeyZhukovsky, simoarpe and samartnik July 25, 2023 19:33

github-actions bot added the needs-security-review label Jul 25, 2023

github-actions bot assigned fmarier and thypon Jul 25, 2023

AlexeyBarabash requested a review from deeppandya July 26, 2023 08:53

iefremov approved these changes Jul 26, 2023

View reviewed changes

fmarier removed the needs-security-review label Jul 27, 2023

fmarier unassigned fmarier and thypon Jul 27, 2023

SergeyZhukovsky reviewed Jul 27, 2023

View reviewed changes

.../browser/crypto_wallet/fragments/onboarding_fragments/DownloadComponentProgressFragment.java Outdated Show resolved Hide resolved

SergeyZhukovsky reviewed Jul 27, 2023

View reviewed changes

...hromium/chrome/browser/crypto_wallet/fragments/onboarding_fragments/SetupWalletFragment.java Outdated Show resolved Hide resolved

AlexeyBarabash force-pushed the android_wallet_compo_on_demand branch from 27689e8 to e4b5941 Compare July 28, 2023 10:43

SergeyZhukovsky approved these changes Jul 28, 2023

View reviewed changes

mkarolin approved these changes Jul 28, 2023

View reviewed changes

simoarpe reviewed Jul 28, 2023

View reviewed changes

simoarpe approved these changes Jul 28, 2023

View reviewed changes

Pavneet-Sing approved these changes Jul 31, 2023

View reviewed changes

browser/ui/android/strings/android_brave_strings.grd Show resolved Hide resolved

android/java/org/chromium/chrome/browser/component_updater/BraveComponentUpdater.java Outdated Show resolved Hide resolved

AlexeyBarabash added 8 commits August 1, 2023 20:58

Download Brave Wallet component on demand on Android

2c7b54e

fixes brave/brave-browser#31824

Fixed presubmit warnings at BraveWalletActivity.java

91bd4f2

Added a period to not display the progress to avoid blinking on fast …

ef3ff42

…internet connection

Some pre-publish fixes of style/typos

b95f281

Fixed typo

04bcb0b

Download component immediately on onboarding, without waiting button

cdc5106

Fixed codereview notices

a903309

Fixed codereview notice - removed not required exception handler

2860c4f

AlexeyBarabash force-pushed the android_wallet_compo_on_demand branch from 112e789 to 2860c4f Compare August 1, 2023 19:02

AlexeyBarabash merged commit f179b8b into master Aug 1, 2023
6 checks passed

AlexeyBarabash deleted the android_wallet_compo_on_demand branch August 1, 2023 20:53

github-actions bot added this to the 1.58.x - Nightly milestone Aug 1, 2023

github-actions bot added the feature/web3/wallet/core label Aug 1, 2023

srirambv mentioned this pull request Sep 6, 2023

Download Brave Wallet data files component on demand on Android brave/brave-browser#31824

Closed

mkarolin mentioned this pull request Sep 12, 2023

Update l10n for 1.58.x (Chromium 117). brave/brave-browser#32814

Closed

mkarolin mentioned this pull request Oct 10, 2023

Update l10n for 1.59.x (Chromium 118). brave/brave-browser#33497

Closed

SergeyZhukovsky mentioned this pull request Oct 20, 2023

Do not parse JSON files in wallet component if wallet isn't created brave/brave-browser#33757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download Brave Wallet component on demand on Android #19086

Download Brave Wallet component on demand on Android #19086

AlexeyBarabash commented Jun 28, 2023 •

edited

Loading

iefremov left a comment

SergeyZhukovsky left a comment

mkarolin left a comment

simoarpe Jul 28, 2023

AlexeyBarabash Jul 28, 2023

simoarpe Jul 28, 2023

AlexeyBarabash Jul 28, 2023

simoarpe Jul 28, 2023

AlexeyBarabash Jul 28, 2023

simoarpe Jul 28, 2023

AlexeyBarabash Jul 28, 2023

simoarpe Jul 28, 2023

AlexeyBarabash Jul 28, 2023

simoarpe Jul 28, 2023

AlexeyBarabash Jul 28, 2023

simoarpe Jul 28, 2023

AlexeyBarabash Jul 28, 2023

simoarpe Jul 28, 2023

AlexeyBarabash Jul 28, 2023

simoarpe Jul 28, 2023

AlexeyBarabash Jul 28, 2023

simoarpe Jul 28, 2023

AlexeyBarabash Jul 28, 2023

simoarpe left a comment

	* Class that is used by wallet_data_files_installer.cc to determine, do we need to download
	* Class that is used by wallet_data_files_installer.cc to determine if we need to download

Download Brave Wallet component on demand on Android #19086

Download Brave Wallet component on demand on Android #19086

Conversation

AlexeyBarabash commented Jun 28, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

iefremov left a comment

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

mkarolin 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

simoarpe left a comment

Choose a reason for hiding this comment

AlexeyBarabash commented Jun 28, 2023 •

edited

Loading