-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/confirmations_impl.cc
Outdated
@@ -285,7 +319,7 @@ std::map<std::string, std::string> ConfirmationsImpl::GetCatalogIssuers() | |||
} | |||
|
|||
bool ConfirmationsImpl::IsValidPublicKeyForCatalogIssues( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Is that a typo in the method name (Issues vs. Issuers)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a typo 👍
a9bdaab
to
760af7d
Compare
760af7d
to
fba436f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit about unit tests, not critical; otherwise OK.
delete request_; | ||
|
||
delete confirmations_; | ||
delete mock_confirmations_client_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason not to use std::unique_ptr for these, so you can skip the explicit deletes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 fixed
":*", | ||
rebase_path("bat-native-ledger", dep_base) + ":*", | ||
"//brave/test:*", | ||
] | ||
|
||
sources = [ | ||
"src/bat/confirmations/notification_info.cc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
header files need to be included here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
UpdateConfirmationsIsReadyStatus(); | ||
NotifyAdsIfConfirmationsIsReady(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming convention for this would be MaybeNotifyConfirmationsIsReady
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaybeNotifyConfirmationsIsReady
does not seem right to me either as not using observer pattern, what about UpdateConfirmationsStatus
until this code can be refactored to use observers?
src/confirmations_impl.h
Outdated
@@ -17,11 +17,13 @@ | |||
#include "refill_tokens.h" | |||
#include "redeem_token.h" | |||
#include "payout_tokens.h" | |||
#include "unblinded_tokens.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have a header include and a forward decl? Looks like only the forward decl is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
namespace confirmations { | ||
|
||
CreateConfirmationRequest::CreateConfirmationRequest() = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convention here would be ConfirmationRequestBuilder
or just ConfirmationRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateConfirmationRequest is due to the server API naming the task CreateConfirmation as the API creates a confirmation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now these classes do not follow the builder design pattern where we could return a request object which can then also be unit tested
src/create_confirmation_request.cc
Outdated
@@ -0,0 +1,112 @@ | |||
/* This Source Code Form is subject to the terms of the Mozilla Public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should really avoid this pattern of putting everything in the src
root, it's going to cause issues with conflicting header include paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree as we discussed before, can we do this after 1.0 as we need to do this across all native libs?
|
||
namespace confirmations { | ||
|
||
FetchPaymentTokenRequest::FetchPaymentTokenRequest() = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PaymentTokenRequestBuilder
or just PaymentTokenRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FetchPaymentTokenRequest is due to the server API naming the task FetchPaymentToken as the API fetches a payment token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, but the convention in chromium is still to use *Builder
for stuff like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems a little unnecessary to even instantiate this in its current form since all the state is transient. I think we either want a Builder
pattern with set_confirmation_id
and maybe set_endpoint_version
(default to 1) or just make them static and dump the class
|
||
namespace confirmations { | ||
|
||
RequestSignedTokensRequest::RequestSignedTokensRequest() = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above builder naming convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, the API requests a signed token
#include <string> | ||
|
||
#include "confirmations_client_mock.h" | ||
#include "brave/vendor/bat-native-confirmations/src/confirmations_impl.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be using header paths like this, it indicates a problem with the include_dirs
in the config. We should expose the internal-config to tests so they can access these files using the correct paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I already did that so not sure how these paths came back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these paths came back as part of unit tests and I used an older template for the unit tests, let me fix these issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 👍
|
||
confirmations_client_->URLRequest(ads_serve_url, {}, "", "", | ||
URLRequestMethod::GET, callback); | ||
confirmations_client_->LoadURL(url, {}, "", "", method, callback); | ||
} | ||
|
||
void RedeemToken::OnFetchPaymentToken( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is huge and should be broken up, but it also has no tests that I can see which is scary given how big it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is 16 lines long? There are currently no tests for URL requests and parsing of the JSON from server responses. I will decouple the JSON parsing code and add further unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am I misreading something? Starts at line 188 and ends at line 341?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sometimes I miss collapsed code in the diff, but I looked again and don't see a method ending that I missed
This reverts commit 35f055e.
f665e89
to
ea09271
Compare
fixes #59
fixes #60