Japan installed Rewards now allows for BAT only in Add Funds dialog #2470

jasonrsadler · 2019-05-21T15:34:46Z

Fixes brave/brave-browser#4476

Brave-UI: brave/brave-ui#470

Submitter Checklist:

Test Plan:

Regression case:

Start Brave with clean profile.
Enable Rewards and navigate to Rewards page.
Click Add Funds and ensure that all funding currencies are available

Test case:

Start Brave with clean profile using -- --rewards=current-country=JP
Enable Rewards and navigate to Rewards page.
Click Add Funds and ensure that on BAT currency is available

Test case 2:

Start Brave with clean profile using -- --rewards=current-country=US (or replace US with any value other than 'JP')
Enable Rewards and navigate to Rewards page.
Click Add Funds and ensure that all funding currencies are available

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

NejcZdovc · 2019-05-22T06:39:36Z

components/brave_rewards/browser/rewards_service_impl.cc

@@ -1528,8 +1531,35 @@ void RewardsServiceImpl::GetAddresses(const GetAddressesCallback& callback) {
    return;
  }

-  bat_ledger_->GetAddresses(base::BindOnce(&RewardsServiceImpl::OnGetAddresses,
-        AsWeakPtr(), callback));
+  bat_ledger_->GetAddressesLimitedCountries(


This logic needs to be moved to ledger code.

So what should happen is that we send country code to ledger and ledger return addresses. Rewards service doesn't need to know about limitations. We shouldn't have business logic in here.

I agree but I wasn't sure if I should call country_codes::CountryCharsToCountryID in ledger.

So instead, I'll move to ledger and make the call to country_codes::CountryCharsToCountryID through LedgerClient

what I would do is just pass country into GetAddresses function so that ledger can know what is your current country and then ledger can take it from there

this way ledger don't need to do another call out

What's happening here is the call to country_codes::GetCountryIdFromPrefs and country_codes::CountryCharsToCountryID take the characters and packs them into an integer (US is something like 20154 JP is something like 19345) and the current country ID (5 digit number) is what is stored in prefs. So to get the comparison, we have to do another call out to get the ID to compare.

we could send in country ID and instead of storing JP in ledger we store ID (which is ISO 3166-1 country code)

No, the ISO 3166-1 code is a 3 digit code.

The code that Cr is storing in prefs is a code that is converted and packed based off of the alpha-2 code received. The result is a 5 digit code.

https://chromium.googlesource.com/chromium/src/+/refs/heads/master/components/country_codes/country_codes.h#35

This is Cr specific and I wouldn't consider "standardized"

Logic is moved to Ledger. There is a callout to LedgerClient to get the Cr specific country code from the alpha-2 chars.

NejcZdovc

we need to move logic in ledger

NejcZdovc · 2019-05-22T06:41:28Z

browser/ui/webui/brave_rewards_ui.cc

-    data.SetString("ETH", addresses.at("ETH"));
-    data.SetString("LTC", addresses.at("LTC"));
-
+    if (addresses.count("BTC") == 1) {


could we just iterate through addresses map, so that we don't have if's

But we have hardcoded strings and wouldn't necessarily know which iteration gets set with which string.

for (auto& address : addresses) { data.SetString(address.first, address.second); }

I was thinking something like this

Ok. That works but FYI SetString is deprecated.

using:

base::Value data(base::Value::Type::DICTIONARY); for (auto& address : addresses) { data.SetKey(address.first, base::Value(address.second)) }

yeah I just copied what we have in the code now :) great

Comments addressed

NejcZdovc · 2019-05-27T10:23:22Z

components/brave_rewards/browser/rewards_service_impl.cc

@@ -1527,9 +1530,17 @@ void RewardsServiceImpl::GetAddresses(const GetAddressesCallback& callback) {
  if (!Connected()) {
    return;
  }
+  int32_t current_country(


nit: could you please do int32_t current_country = country_codes::GetCountryIDFromPrefs(profile_->GetPrefs()) as it's a lot easier to read

NejcZdovc · 2019-05-27T10:25:23Z

components/brave_rewards/browser/rewards_service_impl.cc

@@ -2624,6 +2635,13 @@ void RewardsServiceImpl::HandleFlags(const std::string& options) {

      SetShortRetries(short_retries);
    }
+
+    if (name == "current-country") {


nice one 👍

NejcZdovc · 2019-05-27T10:30:48Z

vendor/bat-native-ledger/include/bat/ledger/ledger_client.h

@@ -222,6 +224,10 @@ class LEDGER_EXPORT LedgerClient {

  virtual void GetExcludedPublishersNumberDB(
      ledger::GetExcludedPublishersNumberDBCallback callback) = 0;
+
+  virtual void GetCountryCodes(
+      const std::vector<std::string>& limited_countries,


Maybe more general param? Like countries, as this function could be used by other functions as well, where we wouldn't passing limited countries in

NejcZdovc · 2019-05-28T05:45:19Z

components/brave_rewards/browser/rewards_service_impl.h

@@ -505,6 +506,9 @@ class RewardsServiceImpl : public RewardsService,
  void OnTwitterPublisherInfoSaved(SaveMediaInfoCallback callback,
                                   int32_t result,
                                   ledger::PublisherInfoPtr publisher);
+  void GetCountryCodes(
+      const std::vector<std::string>& limited_countries,
