Rewards bitflyer android integration #11587

deeppandya · 2021-12-14T01:15:16Z

Resolves brave/brave-browser#18471

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, npm run lint, 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:

Add Balance related info

Add balance report

Add observer for tip update Add bifluer changes

Remove verify banner Updfate rewards modal pref

SergeyZhukovsky · 2021-12-14T14:22:12Z

components/brave_rewards/browser/rewards_protocol_handler.cc

@@ -69,7 +69,7 @@ void LoadRewardsURL(
  const std::string bitflyer_staging_host = bitflyer_staging_url.host();
  const std::string gemini_staging_host = gemini_oauth_staging_url.host();
  const char* kAllowedDomains[] = {
-      "bitflyer.com",                 // bitFlyer production
+      "dummy.com",                    // bitFlyer production


is it a real domain?

No, the bitFlyer production domain is stubbed out for now per request from @Miyayes.

What will we be changing this to before merging?

@zenparsing rightly pointed out that this won't work as-is, so we're reverting it and will rely on stubbing the production client id in our builds instead.

SergeyZhukovsky · 2021-12-14T14:25:39Z

@deeppandya I think we need to check it on Android 6, as the PR contains new vector images.

deeppandya · 2021-12-14T14:31:03Z

@deeppandya I think we need to check it on Android 6, as the PR contains new vector images.

@SergeyZhukovsky I have checked. If we use support components, it's fine.

SergeyZhukovsky

++

emerick · 2021-12-14T16:14:16Z

android/java/org/chromium/chrome/browser/BraveRewardsVerifyWalletActivity.java

+                            intent.putExtra(
+                                    BraveActivity.OPEN_URL, BraveWalletProvider.UPHOLD_ORIGIN_URL);


@deeppandya If this function is Uphold-specific, do we need a bitFlyer-specific function. If it's not Uphold-specific, then it seems like we need to be smarter here (and probably should rename the function, etc.)

emerick · 2021-12-14T16:15:05Z

android/java/org/chromium/chrome/browser/BraveWalletProvider.java

+                redirect_url = jsonObj.getString(REDIRECT_URL_KEY);
+            }
+        } catch (JSONException e) {
+        }


Should we log something here?

emerick · 2021-12-14T16:22:03Z

android/java/org/chromium/chrome/browser/rewards/BraveRewardsPanel.java

+        SharedPreferences.Editor editor = sharedPref.edit();
+        int rightDrawable = 0;
+        int leftDrawable = 0;
+        int text = 0;


Nit: Maybe textId is a better name here?

emerick · 2021-12-14T16:27:37Z

android/java/org/chromium/chrome/browser/app/BraveActivity.java

@@ -170,6 +170,8 @@
    public static final String ADD_FUNDS_URL = "chrome://rewards/#add-funds";
    public static final String REWARDS_SETTINGS_URL = "chrome://rewards/";
    public static final String BRAVE_REWARDS_SETTINGS_URL = "brave://rewards/";
+    public static final String BRAVE_REWARDS_SETTINGS_WALLET_PROVIDER_URL =


Nit: I would name this BRAVE_REWARDS_SETTINGS_WALLET_VERIFICATION_URL just to be a little more specific.

emerick · 2021-12-14T16:33:08Z

android/java/org/chromium/chrome/browser/rewards/BraveRewardsPanel.java

+                    view.getLocationOnScreen(location);
+                    int x = location[0];
+                    int y = location[1];
+                    loginPopupWindow.showAtLocation(view, Gravity.NO_GRAVITY, 0, y);


Just checking that we don't want to use x here?

emerick · 2021-12-14T16:35:02Z

android/java/org/chromium/chrome/browser/rewards/BraveRewardsPanel.java

+            }
+        } else if (errorCode == BraveRewardsNativeWorker.LEDGER_ERROR) { // No Internet connection
+            String args[] = {};
+            Log.e(TAG, "LEDGER_ERROR");


Maybe we could say something like Failed to fetch rewards parameters from server (or something like that) here?

zenparsing · 2021-12-15T14:57:10Z

components/brave_rewards/browser/rewards_protocol_handler.cc

@@ -69,7 +69,7 @@ void LoadRewardsURL(
  const std::string bitflyer_staging_host = bitflyer_staging_url.host();
  const std::string gemini_staging_host = gemini_oauth_staging_url.host();
  const char* kAllowedDomains[] = {
-      "bitflyer.com",                 // bitFlyer production
+      "dummy.com",                    // bitFlyer production


What will we be changing this to before merging?

zenparsing · 2021-12-15T16:35:03Z

components/brave_rewards/browser/rewards_service.h

@@ -312,6 +312,8 @@ class RewardsService : public KeyedService {

  virtual void GetExternalWallet(GetExternalWalletCallback callback) = 0;

+  virtual std::string GetExternalWalletType() const = 0;


Code that relies on this method may need to be rewritten when we improve how we handle external wallets internally.

Currently, SetExternalWalletType is used to set a pref, which mostly serves to control which wallet type will be returned from GetExternalWallet. This was a hack to work around the fact that the front-end design expects to be able to retrieve a populated mojom::ExternalWallet structure, even when the user is not connected to any wallet provider.

Ideally, this pref would not be necessary: the engine would know which wallet is currently connected (if any), and the front-end would not expect to get "login" URLs from the ExternalWallet structure. In that case we'd remove the SetExternalWalletType method.

Code that relies on GetExternalWalletType (a sync method), may have to be rewritten in the future, and I would prefer that we not have more implementations using the SetExternalWalletType hack going forward.

@zenparsing mentioned in our meeting that this is good for now, but let's not use GetExternalWalletType beyond this going forward.

This reverts commit 4ec8301.

deeppandya added this to the 1.35.x - Nightly milestone Dec 14, 2021

deeppandya requested review from emerick, zenparsing and SergeyZhukovsky December 14, 2021 01:15

deeppandya requested a review from a team as a code owner December 14, 2021 01:15

deeppandya self-assigned this Dec 14, 2021

emerick and others added 14 commits December 13, 2021 20:15

Initial work on bitFlyer support on Android

73ddb97

Stub in non-fuctional production bitFlyer domain for now

4ec8301

Add rewards panel 2.0 UI

1ac3080

Add publisher info

2200d0a

Add Balance related info

Add monthly contribution

2f0bc1e

Update Layout for Tip/Summary toggle

f958810

Add balance report

Add notification layout

8ac8822

Resolve reabse issue

d9bfb2f

Add fetch grants

2496186

Add rewards onboarding

aab02eb

Add observers for claim update

9dda1a1

Add observer for tip update Add bifluer changes

Add shadow for all popups

1d990ad

Remove verify banner Updfate rewards modal pref

Refactor colors and strings

9282b0f

Refactor rewards panel

e989f67

deeppandya force-pushed the rewards-bitflyer-android-integration branch from 0fb2072 to e989f67 Compare December 14, 2021 01:16

SergeyZhukovsky reviewed Dec 14, 2021

View reviewed changes

SergeyZhukovsky approved these changes Dec 14, 2021

View reviewed changes

emerick reviewed Dec 14, 2021

View reviewed changes

Hide pending tips on Android

86154c1

zenparsing approved these changes Dec 15, 2021

View reviewed changes

zenparsing reviewed Dec 15, 2021

View reviewed changes

Revert "Stub in non-fuctional production bitFlyer domain for now"

988e874

This reverts commit 4ec8301.

deeppandya added 2 commits December 15, 2021 17:42

Address PR comments

fddfef4

Resolve CI error

7b2d712

deeppandya merged commit b048108 into master Dec 16, 2021

deeppandya deleted the rewards-bitflyer-android-integration branch December 16, 2021 14:55

emerick restored the rewards-bitflyer-android-integration branch February 4, 2022 22:37

emerick deleted the rewards-bitflyer-android-integration branch March 28, 2022 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewards bitflyer android integration #11587

Rewards bitflyer android integration #11587

deeppandya commented Dec 14, 2021

SergeyZhukovsky Dec 14, 2021

emerick Dec 14, 2021

zenparsing Dec 15, 2021

emerick Dec 15, 2021

SergeyZhukovsky commented Dec 14, 2021

deeppandya commented Dec 14, 2021

SergeyZhukovsky left a comment

emerick Dec 14, 2021

emerick Dec 14, 2021

emerick Dec 14, 2021

emerick Dec 14, 2021

emerick Dec 14, 2021

emerick Dec 14, 2021

zenparsing Dec 15, 2021

zenparsing Dec 15, 2021

emerick Dec 15, 2021

		intent.putExtra(
		BraveActivity.OPEN_URL, BraveWalletProvider.UPHOLD_ORIGIN_URL);

		@@ -312,6 +312,8 @@ class RewardsService : public KeyedService {

		virtual void GetExternalWallet(GetExternalWalletCallback callback) = 0;

		virtual std::string GetExternalWalletType() const = 0;

Rewards bitflyer android integration #11587

Rewards bitflyer android integration #11587

Conversation

deeppandya commented Dec 14, 2021

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

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

SergeyZhukovsky commented Dec 14, 2021

deeppandya commented Dec 14, 2021

SergeyZhukovsky 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