Confirmations implementation #1469

tmancey · 2019-01-26T09:21:39Z

fixes brave/brave-browser#3117

Submitter Checklist:

Test Plan:

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

vendor/bat-native-ledger/src/ledger_impl.cc

emerick · 2019-01-26T12:48:45Z

components/brave_rewards/browser/rewards_service_impl.cc

@@ -1218,7 +1218,7 @@ void RewardsServiceImpl::OnURLFetchComplete(
    return;
  }

-  callback(response_code == 200, body, headers);
+  callback(response_code, body, headers);


@nejc Would be good if you could verify that this commit was done correctly, as far as Rewards is concerned.

emerick · 2019-01-26T12:57:17Z

components/brave_ads/browser/ads_service_impl.cc

@@ -578,18 +580,18 @@ void AdsServiceImpl::ShowNotification(std::unique_ptr<ads::NotificationInfo> inf
 }

 void AdsServiceImpl::SetCatalogIssuers(std::unique_ptr<ads::IssuersInfo> info) {
-  // TODO(Terry Mancey): https://github.com/brave/brave-browser/issues/2906
-  (void)info;
+  rewards_service_->SetCatalogIssuers(std::move(info));


Should this return early if (!connected())? I see other methods in here doing that...

Good catch 👍, ideally we should be checking bounds for the rewards service however the lifetime of the rewards service outlasts Ads

components/brave_ads/browser/ads_service_impl.cc

emerick · 2019-01-26T13:01:43Z

components/services/bat_ledger/bat_ledger_client_mojo_proxy.cc

+    ledger::URL_METHOD method,
+    ledger::URLRequestCallback callback) {
+  if (!Connected()) {
+    callback(418, "", std::map<std::string, std::string>());


This HTTP response seems random (I copied it from similar functionality in bat_ledger). Should it just be a generic 400 (and preferably some kind of constant - Chromium probably has definitions for these).

This was chosen as an unlikely response that we would get in the real world so that we knew the service had failed. @bridiver your thoughts please? If we do want to use a constant they are available in src/net/http/http_status_code_list.h

this shouldn't be here at all. The mojo proxy shouldn't have any logic in it

bridiver · 2019-01-31T22:40:20Z

components/brave_rewards/browser/rewards_service_impl.cc

+    return;
+  }
+
+  auto* ads_service = brave_ads::AdsServiceFactory::GetForProfile(profile_);  // NOT OWNED


we are we calling this from rewards_service if it calls ads_service?

bridiver · 2019-01-31T22:40:35Z

components/brave_rewards/browser/rewards_service_impl.cc

+}
+
+void RewardsServiceImpl::SetConfirmationsIsReady(const bool is_ready) {
+  if (!Connected()) {


this is only necessary when we're using the mojo interfaces

bridiver · 2019-01-31T22:44:32Z

components/brave_rewards/browser/rewards_service_impl.cc

+  fetcher->Start();
+}
+
+void RewardsServiceImpl::SaveConfirmationsState(const std::string& name,


We don't need individual methods for these. We just need a generic Save and Load method like AdsService which can be used by all state files

bridiver · 2019-01-31T22:45:03Z

components/brave_rewards/browser/rewards_service_impl.cc

+  bat_ledger_->AdSustained(info->ToJson());
+}
+
+void RewardsServiceImpl::URLRequest(const std::string& url,


why are we adding this? We already have LoadURL

bridiver · 2019-01-31T22:46:18Z

components/brave_rewards/browser/rewards_service_impl.cc

@@ -112,6 +112,36 @@ class LogStreamImpl : public ledger::LogStream {
  DISALLOW_COPY_AND_ASSIGN(LogStreamImpl);
 };

+class ConfirmationsLogStreamImpl : public confirmations::LogStream {


we don't need duplicates of all this stuff for Confirmations

bridiver · 2019-01-31T22:46:42Z

components/brave_rewards/browser/rewards_service_impl.cc

@@ -257,6 +287,27 @@ time_t GetCurrentTimestamp() {
  return base::Time::NowFromSystemTime().ToTimeT();
 }

+std::string LoadOnFileTaskRunner(const base::FilePath& path) {


don't need duplicates of this stuff either

bridiver

we need to go through this together because there are a lot of issues here that need to be addressed

Upgrading remaining grant endpoints to v4

bridiver · 2019-02-06T02:09:59Z

components/brave_rewards/browser/BUILD.gn

@@ -94,6 +94,8 @@ source_set("browser") {
    }

    deps += [
+      "//brave/vendor/bat-native-ads",


this needs a brave_ads_enabled guard

bridiver · 2019-02-06T02:10:34Z

components/services/bat_ledger/BUILD.gn

@@ -20,6 +20,8 @@ static_library("lib") {

  deps = [
    "//base",
+    "//brave/vendor/bat-native-ads",


same here with brave_ads_enabled

bridiver · 2019-02-06T02:33:10Z

components/brave_rewards/browser/rewards_service_impl.h

 #include "bat/ledger/ledger_client.h"
 #include "brave/components/services/bat_ledger/public/interfaces/bat_ledger.mojom.h"
 #include "brave/components/brave_rewards/browser/rewards_service.h"
+#include "brave/components/brave_ads/browser/ads_service.h"


there is no reason for these to be here. Only add includes to the header file if they are needed for the declaration. Everything else should go in the cc file

bridiver · 2019-02-06T02:34:25Z

components/brave_rewards/browser/rewards_service_impl.h

@@ -10,14 +10,19 @@
 #include <memory>
 #include <string>

+#include "bat/ads/issuers_info.h"
+#include "bat/ads/notification_info.h"


this can be a forward declaration

bridiver · 2019-02-06T02:34:56Z

components/brave_rewards/browser/rewards_service_impl.h

 #include "bat/ledger/ledger.h"
 #include "bat/ledger/wallet_info.h"
 #include "base/files/file_path.h"
 #include "base/observer_list.h"
 #include "base/memory/weak_ptr.h"
+#include "bat/confirmations/confirmations_client.h"


this doesn't appear to be needed at all

bridiver · 2019-02-06T02:35:23Z

components/brave_rewards/browser/rewards_service_impl.h

@@ -10,14 +10,19 @@
 #include <memory>
 #include <string>

+#include "bat/ads/issuers_info.h"


this can be a forward declaration

bridiver · 2019-02-06T02:41:01Z

vendor/bat-native-ledger/src/bat_helper.h

    CurrentReconciles current_reconciles_;
    bool auto_contribute_ = false;
    bool rewards_enabled_ = false;
  };

+  struct GRANTS_PROPERTIES_ST {


why is a vector of grant responses called GRANTS_PROPERTIES_ST? Also we should stop using this all caps naming

I was following the naming conventions of the other response structs, I can update

bridiver · 2019-02-06T02:43:25Z

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

+                                      ledger::OnLoadCallback callback) = 0;
+  virtual void ResetConfirmationsState(const std::string& name,
+                                       ledger::OnResetCallback callback) = 0;
+  virtual uint32_t SetConfirmationsTimer(const uint64_t time_offset) = 0;


we shouldn't have duplicates of this either

bridiver · 2019-02-06T02:45:17Z

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

+                                       ledger::OnResetCallback callback) = 0;
+  virtual uint32_t SetConfirmationsTimer(const uint64_t time_offset) = 0;
+  virtual void KillConfirmationsTimer(uint32_t timer_id) = 0;
+  virtual void SetConfirmationsIsReady(const bool is_ready) = 0;


why is this both a Ledger and LedgerClient method? I'm very confused about what it's supposed to do

bridiver · 2019-02-06T02:46:44Z

components/brave_ads/browser/ads_service_impl.cc

@@ -584,6 +592,20 @@ void AdsServiceImpl::ShowNotification(
              timer_id, notification_id));
 }

+void AdsServiceImpl::SetCatalogIssuers(std::unique_ptr<ads::IssuersInfo> info) {


why are we calling this on ads service if it calls rewards service?

components/brave_rewards/browser/rewards_notification_service_impl.cc

bridiver · 2019-02-06T02:54:52Z

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

+      const ledger::URL_METHOD method,
+      ledger::URLRequestCallback callback) = 0;
+
+  virtual void SaveConfirmationsState(const std::string& name,


this stuff should not be added to LedgerClient, LedgerImpl should just proxy to the LedgerClient methods for SetTimer, LoadURL, etc..

setGrants -> setGrant

Adding test to reflect change

tmancey · 2019-02-13T10:40:16Z

Closing PR as superseded by #1645

Add options for installer signing key on Mac

tmancey added PR/Pending Review feature/ads labels Jan 26, 2019

tmancey added this to the 0.62.x - Nightly milestone Jan 26, 2019

tmancey self-assigned this Jan 26, 2019

tmancey requested review from bridiver, mrose17, emerick and NejcZdovc January 26, 2019 09:21

emerick reviewed Jan 26, 2019

View reviewed changes

tmancey force-pushed the issues/2912-0.62.x branch 2 times, most recently from ec86ca4 to 2cc4133 Compare January 27, 2019 10:45

tmancey changed the title ~~Confirmations implementation~~ Confirmations implementation 0.62.x Jan 28, 2019

tmancey force-pushed the issues/2912-0.62.x branch 3 times, most recently from 6141c86 to e7301ed Compare January 29, 2019 15:09

ryanml mentioned this pull request Jan 30, 2019

WIP: Confirmations UI integration #1509

Closed

18 tasks

tmancey force-pushed the issues/2912-0.62.x branch 2 times, most recently from 1410050 to 193f900 Compare January 31, 2019 18:54

bridiver reviewed Jan 31, 2019

View reviewed changes

bridiver requested changes Jan 31, 2019

View reviewed changes

tmancey force-pushed the issues/2912-0.62.x branch 3 times, most recently from 011c7d7 to ca3621b Compare February 4, 2019 22:32

ryanml force-pushed the issues/2912-0.62.x branch from 9b5662f to 56ca660 Compare February 5, 2019 18:14

Added bat-native-confirmations dep

484dd72

ryanml added 2 commits February 5, 2019 19:40

Adding Ads claim process to Rewards panel

c84d6b7

Adding grant reducer tests

ffd166c

Upgrading remaining grant endpoints to v4

tmancey force-pushed the issues/2912-0.62.x branch from 56ca660 to ffd166c Compare February 5, 2019 19:40

Bump bat-native-confirmations dep

279c24c

bridiver reviewed Feb 6, 2019

View reviewed changes

components/brave_rewards/browser/rewards_notification_service_impl.cc Outdated Show resolved Hide resolved

bridiver reviewed Feb 6, 2019

View reviewed changes

ryanml and others added 4 commits February 5, 2019 21:09

Refactoring grant prefix fetch in to method

551161a

Sending promotionId with captcha solution

ae91f37

setGrants -> setGrant

Bump challenge-bypass-ristretto-ffi dep

ff40ce8

Small bug fix for last grant deletion

a2ef113

Adding test to reflect change

ryanml force-pushed the issues/2912-0.62.x branch from 375e361 to a2ef113 Compare February 11, 2019 15:02

tmancey removed the PR/Pending Review label Feb 13, 2019

tmancey closed this Feb 13, 2019

tmancey deleted the issues/2912-0.62.x branch February 21, 2019 20:03

tmancey removed this from the 0.62.x - Dev milestone Feb 27, 2019

tmancey changed the title ~~Confirmations implementation 0.62.x~~ Confirmations implementation Feb 27, 2019

fmarier pushed a commit that referenced this pull request Oct 29, 2019

Merge pull request #1469 from brave/mac-installer-sign-pkg

17fac2a

Add options for installer signing key on Mac

petemill pushed a commit that referenced this pull request Jul 27, 2020

Merge pull request #1469 from brave/mac-installer-sign-pkg

facbad1

Add options for installer signing key on Mac

petemill pushed a commit that referenced this pull request Jul 28, 2020

Merge pull request #1469 from brave/mac-installer-sign-pkg

dafcdd7

Add options for installer signing key on Mac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Confirmations implementation #1469

Confirmations implementation #1469

tmancey commented Jan 26, 2019 •

edited

Loading

emerick Jan 26, 2019

emerick Jan 26, 2019

tmancey Jan 27, 2019 •

edited

Loading

emerick Jan 26, 2019

tmancey Jan 27, 2019 •

edited

Loading

bridiver Feb 6, 2019

bridiver Jan 31, 2019

bridiver Jan 31, 2019

bridiver Jan 31, 2019

bridiver Jan 31, 2019

bridiver Jan 31, 2019

bridiver Jan 31, 2019

bridiver left a comment

bridiver Feb 6, 2019

bridiver Feb 6, 2019

bridiver Feb 6, 2019

bridiver Feb 6, 2019

bridiver Feb 6, 2019

bridiver Feb 6, 2019

bridiver Feb 6, 2019

ryanml Feb 6, 2019

bridiver Feb 6, 2019

bridiver Feb 6, 2019

bridiver Feb 6, 2019

bridiver Feb 6, 2019

tmancey commented Feb 13, 2019

Confirmations implementation #1469

Confirmations implementation #1469

Conversation

tmancey commented Jan 26, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmancey Jan 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmancey Jan 27, 2019 • edited Loading

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

bridiver 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

tmancey commented Feb 13, 2019

tmancey commented Jan 26, 2019 •

edited

Loading

tmancey Jan 27, 2019 •

edited

Loading

tmancey Jan 27, 2019 •

edited

Loading