-
Notifications
You must be signed in to change notification settings - Fork 1k
Moves promotions to new endpoint structure #6422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2af2dda
to
0197c8a
Compare
0197c8a
to
aaab482
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.
iOS changes lgtm
vendor/bat-native-ledger/src/bat/ledger/internal/attestation/attestation_desktop.cc
Outdated
Show resolved
Hide resolved
ConfirmCallback callback); | ||
|
||
std::unique_ptr<endpoint::PromotionServer> promotion_server_; |
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.
Just an observation, but I don't think these need to be unique_ptr
s - I think you could just have endpoint::PromotionServer
.
vendor/bat-native-ledger/src/bat/ledger/internal/attestation/attestation_iosx.cc
Outdated
Show resolved
Hide resolved
GetAvailable::~GetAvailable() = default; | ||
|
||
std::string GetAvailable::GetUrl(const std::string& platform) { | ||
const std::string payment_id = ledger_->state()->GetPaymentId(); |
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.
Just an observation: another option would be to take the payment ID as a parameter to Request
. That would probably make these classes more testable, since there would be no need to mock out this payment ID in the ledger or ledger client level.
...ve-ledger/src/bat/ledger/internal/endpoint/promotion/get_recover_wallet/get_recover_wallet.h
Show resolved
Hide resolved
...or/bat-native-ledger/src/bat/ledger/internal/endpoint/promotion/post_captcha/post_captcha.cc
Outdated
Show resolved
Hide resolved
...or/bat-native-ledger/src/bat/ledger/internal/endpoint/promotion/post_captcha/post_captcha.cc
Outdated
Show resolved
Hide resolved
.../src/bat/ledger/internal/endpoint/promotion/post_suggestions_claim/post_suggestions_claim.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/endpoint/promotion/promotion_server.h
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/wallet/wallet_claim.h
Outdated
Show resolved
Hide resolved
5264b09
to
cec8ef4
Compare
cec8ef4
to
6baff0d
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.
Looks good, and was clearly a lot of work. Given the amount of code, I wasn't able to look really closely at every line, but I think I covered most of it.
Resolves brave/brave-browser#11203
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Check this paths:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.