Import Brave Payments data from Muon #736

garrettr · 2018-10-25T18:42:25Z

Import Brave Payments data from Muon

Test plan

Happy path

Grab the code + build
Clear your brave-core profile directory (ex: ~/Library/Application Support/BraveSoftware/Brave-Browser-Development or %USERPROFILE%\AppData\Local\BraveSoftware\Brave-Browser-Development)
Decide if you'd like to use STAGING or PROD (for rewards services). STAGING might be better, since you can transfer some funds easily.
Have a Release channel Muon install which needs to have:
- wallet created (balance optional, but would be nice)
- preferences set (ex: contribution amount, media publishers enabled, etc)
- uses the environment you chose in step 3
Run brave-core from source (ex: npm start). If you chose a production profile for step 4, then you'll want to start like: npm run start -- --rewards=staging=false --vmodule=*rewards*=6 (local builds default to STAGING)
Once browser comes up, go to chrome://settings/importData
In the drop down, pick Brave
Verify a new option, Ledger shows up
When ready, choose ONLY ledger and click Import
Import will finish
- If a confirmation shows up, it worked 😄
- If it failed, the import cancels itself. Unfortunately, no UI... but you can confirm by checking the stdout console (where you did npm start)
Visit brave://rewards and verify each of the settings was imported:
- Auto-Contribute on/off status - this is set to the Brave Payments enabled/disabled status. Please note that If a user (in Muon) had Brave Payments disabled, import for payments will be skipped (settings and wallet info will not be processed).
- Allow contribution to non-verified sites
- Allow contribution to videos
- Monthly Payment - The actual contribution amount (see testing notes)
- Minimum visits for publisher relevancy
- Minimum page time before logging a visit
- Tips - recurring. these are sites which were Pinned in Muon (see testing notes)

When Muon Brave does not have rewards enabled

When user doesn't have rewards enabled in Muon Brave, the ledger import should basically do nothing. Ideally, it wouldn't show as an option in the import list.

Testing notes

There are some important considerations when verifying the preferences and amounts carried over 😄

When tips are imported, it's converted from a percentage to a physical amount in BAT. So for example, if user had monthly contribution amount of 20 BAT and they had a pinned site for 50%, the import would add this as a recurring tip for 10 BAT
The contribution amount for Auto-Contribute is decreased by the amount of recurring tips. For example, if user has monthly contribution of 20 BAT and they end up with 5 BAT in recurring tips, the auto-contribute amount would then be 15 BAT.
If (after subtracting recurring tips) the amount is less than 10 BAT (see https://github.com/brave/brave-core/pull/736/files#diff-fc0938edc13ab88906460783120256edR235), auto-contribute is disabled. For example, if user has monthly contribution of 15 BAT and their pinned sites account for 50% of that, they would only be left with 7.5 BAT. This is less than the minimum amount (10 BAT), so auto-contribute is then disabled. (see Muon Upgrade: Pinning to Recurring Tips workflow brave-browser#1910 (comment) for original explanation by @davidtemkin)

Original description

Looking for feedback on this WIP PR for brave/brave-browser#1215. This PR currently handles the plumbing for a new importer data type—currently called "ledger", though I expect we may want to change that to "Brave Payments" throughout—and has an example of importing data from Muon's user data directory in the utility process and passing it over IPC to the browser process.

The most pertinent remaining questions are:

What specific data needs to be imported from Brave Payments?
Once that data is imported and passed to BraveProfileWriter::ImportLedger, how do we integrate it into Brave Rewards so it works with the new code and provides the desired user experience?

Since these are broad and open questions, the best place for feedback is in the following design docs:

Brave Rewards, "Upgrading from Muon to Brave Core". This is the high-level design doc, focused on UX.
[Design Doc] Implement wallet import/migration for existing users #1215. This is the low-level design doc, focused on implementation details.

patches/chrome-common-importer-importer_bridge.h.patch

bridiver · 2018-10-25T19:03:39Z

patches/chrome-common-importer-importer_data_types.h.patch

 --- a/chrome/common/importer/importer_data_types.h
 +++ b/chrome/common/importer/importer_data_types.h
-@@ -31,7 +31,8 @@ enum ImportItem {
+@@ -31,7 +31,9 @@ enum ImportItem {
   SEARCH_ENGINES     = 1 << 4,
   HOME_PAGE          = 1 << 5,
   AUTOFILL_FORM_DATA = 1 << 6,
 -  ALL                = (1 << 7) - 1  // All the bits should be 1, hence the -1.
 +  STATS              = 1 << 7,


I think we can replace this with just a generic BRAVE import type?

bridiver · 2018-10-25T19:06:13Z

patches/chrome-common-importer-profile_import.mojom.patch

@@ -10,23 +10,27 @@ index 625b5ddec268292acf11f3c060c07c336ca1317f..1738d9d68a93ec1940688caa3352d55f
 import "url/mojom/url.mojom";

 const string kProfileImportServiceName = "profile_import";
-@@ -31,6 +32,9 @@ struct FaviconUsageDataList;
+@@ -31,6 +32,12 @@ struct FaviconUsageDataList;
 [Native]
 struct ImporterIE7PasswordInfo;

 +[Native]


same as above, I think we can combine all Brave-specific data together

@bridiver I think we should keep them separate to allow for user choice when importing from Brave/Muon. For example, what if a user wants to import their stats but not their wallet, or vice versa?

but is this necessary for that to happen? Can't we still group it together even if it isn't all there for any given import?

BraveExternalProcessImporterHost::StartImportSettings still gets the full list of selected items if we keep those split out, so maybe we keep the individual items, but group the import into a single method?

and actually I thought this was a silent automatic import?

and actually I thought this was a silent automatic import?

We need to support both the silent automatic import case and the manual import case (i.e. former browser-laptop user downloads Brave-Core from website and installs it, then decides they want to import their old browser-laptop data). The importer code is identical between the use cases. The specifics of the silent automatic import case are handled in a separate PR (#729); among other things, it assumes the user wants to import all supported importer data types.

bridiver · 2018-10-25T19:07:44Z

patches/chrome-common-importer-profile_import.typemap.patch


 mojom = "//chrome/common/importer/profile_import.mojom"
 public_headers = [
+  "//brave/common/importer/brave_ledger.h",


and again here, even if we couldn't combine the data, we should certainly combine the header files so we only have to patch once. We should always be looking for places where we can create extension points that allow us to make additional changes without additional patches

patches/chrome-common-importer-profile_import_process_param_traits_macros.h.patch

patches/chrome-common-pref_names.cc.patch

patches/chrome-common-pref_names.h.patch

patches/third_party-blink-renderer-core-layout-intersection_geometry.cc.patch

bridiver · 2018-10-25T19:09:54Z

internally we can split out stats, ledger, etc.., but from an upstream importer perspective I think we should combine it all together

bridiver · 2018-10-25T19:11:44Z

patches/chrome-browser-resources-settings-people_page-import_data_dialog.html.patch

@@ -16,6 +16,12 @@ index fdd6f9d2265fe069d159ceed6e1e7ec561a2915e..e0dee6f2af7b92dd2b7f88fefbfda92f
 +              pref="{{prefs.import_dialog_stats}}"
 +              label="$i18n{importStats}">
 +          </settings-checkbox>
+          <settings-checkbox


I think there might be a way to do a template include here that could wrap all of this and move the actual source to brave-core. Not 100% sure though

in either case for this and the one below, do we need separate checkboxes for all of these? If we do that's fine, just seems like it could be consolidated. Also won't this show up for non-Brave imports? That might be a little confusing?

in either case for this and the one below, do we need separate checkboxes for all of these?

I think we do want separate checkboxes, again so users can decide what specific types of data they want to import from Brave. e.g. I can imagine plenty of users might want to keep their stats but their wallet hasn't been working and they'd like to start over fresh there.

Also won't this show up for non-Brave imports?

No, see how services_supported is set for each importer in importer_list.cc.

patches/chrome-browser-ui-webui-settings-md_settings_ui.cc.patch

bridiver · 2018-10-25T19:31:55Z

patches/chrome-browser-importer-external_process_importer_client.h.patch

@@ -12,16 +12,18 @@ index 864a6951115dda5ed74963f18b35692960397d50..ba056b13fbae07f151286f531e1a8d86
 struct ImportedBookmarkEntry;
 class InProcessImporterBridge;
 +struct BraveStats;


I'm not sure that this patch is necessary either because we can cast as mentioned in the other case, but might still be necessary to match up with the mojom file

if we do need to keep these changes, we could combine the patch with a chromium_src override. The override could insert the struct and I think we could define a macro in the header override that would insert all the methods with a single one-line patch

bsclifton · 2018-11-05T21:49:25Z

Will this import Brave Payments information... even if the person disabled Payments? (ex: maybe they turned it off because they were upset)?

A use-case for when the person had it disabled (but HAS a wallet with tokens in there) might be that they didn't like auto-contribute. We have the opportunity to import the wallet... but turn ONLY auto-contribute off

bsclifton · 2018-11-05T21:53:02Z

A heads up on a possible edge-case scenario (we'd have to install an old version to verify)... but the first rev of Payments was done using Bitcoin. This also used the ledger-state.json... and is detectable (I think we removed the detection logic though)

If the wallet address is a Bitcoin address, we should skip importing the payments (and it might be possible to help those folks recover funds manually)

utility/importer/brave_importer.cc

garrettr · 2018-11-06T04:58:37Z

Will this import Brave Payments information... even if the person disabled Payments?

I am currently importing whatever payments data I can find, regardless of whether payments was enabled in browser-laptop

browser/importer/brave_profile_writer.cc

bsclifton · 2018-11-15T07:40:52Z

There are a few TODOs to sort out, but this is basically ready for review
cc: @garrettr @bridiver @NejcZdovc

Please note: the wallet restore (when compiled locally) will use the staging environment. If you'd like to restore a prod wallet, you need to add the proper command line flag

…s values from BraveImporter to BraveProfileWriter

- Implements pinned logic captured in brave/brave-browser#1910 - Tip amount in BAT is calculated based on % of monthly contribution. If < 1 BAT, pin is ignored. - Any amounts spent towards tipping are deducted from monthly budget - If monthly budget is below lowest value (15 BAT), auto-contribute is disabled - Log error (and result code) when RestoreWallet is not successful - Set contribution amount (again) after RestoreWallet, since we found this is adjusted - Created new `kBravePaymentsPinnedItemCount` profile preference (present after import) for use with brave/brave-browser#1910 - Remove unused wallet_seed code

- If wallet exists, import will be cancelled - In other failure scenarios, import will be cancelled - Observer is now added (in writer) after validation (to prevent dcheck failure on cancel)

- Create wallet BEFORE restore is done - Call SetUserChangedContribution() before changing contribution amount - Contribution amount is only set AFTER the restore - Handle popular use-case where contribution amount isn't set (ex: Muon would default them; only choosing it would set the value) - Update lines to be 80 chars (or less) per review feedback

- Remove chrome-common-importer-importer_bridge.h.patch - Replace patch with chromium_src override - Replace patches with chromium_src overrides - Remove workaround for DCHECK that appears fixed - Remove patch, register importer prefs in brave_profile_prefs.cc instead - Remove forward declarations in ExternalProcessImporterClient patch

If a Rewards wallet exists prior to import, ledger_state is now backed up before attempting to recover

- Revert "Remove chrome-common-importer-importer_bridge.h.patch" This reverts commit e6706b1. - Use chromium_src to minimize patches - Remove unnecessary include - Fix header include position

- removed un-used `clobber_wallet` param - added validation for previously unvalidated fields - pulled common cancel logic into common method (ProfileWriter) - any parsing attempts are considered fatal; returns immediately - properly handle reference to this when calling PostTaskAndReplyWithResult - add dcheck for bridge_ptr_ - put `importer/brave_ledger.*` under !is_android block - update CHECK to be DCHECK for ProfileWriter destructor - use regular profile (ex: don't call GetOriginalProfile) in ProfileWriter - remove signature from PostTaskAndReplyWithResult call

- Update browser import name from "Brave" to "Brave (old)" - Updated import type name from "Ledger" to "Brave Payments" - Removed debug log statement - Removed explicit call to `SetUserChangedContribution` (call instead made inside RewardsServiceImpl)

bsclifton · 2018-11-28T16:05:23Z

Great catch @LaurenWags - I captured brave/brave-browser#2265 for the issue you saw and have already started to work on a fix. The values ARE being set properly, however the UI is not respecting them

tmancey

👍 from me

bsclifton · 2018-11-28T18:00:09Z

master ca6fa95
0.58.x a0082fc
0.57.x 055cfbe

Import Brave Payments data from Muon

garrettr added the pr/work-in-progress label Oct 25, 2018