Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove semi-verified state #9032

Closed
wants to merge 15 commits into from
Closed

Remove semi-verified state #9032

wants to merge 15 commits into from

Conversation

szilardszaloki
Copy link
Collaborator

Do the linking of the Uphold account to the Brave wallet during authorization.

Added the balance fetching of the anonymous wallet to the Uphold authorization flow.
Removed fetching the balances for all the wallet types (anonymous, blinded, uphold and bitflyer) and removed linking from the generate wallet flow.

Resolves brave/brave-browser#15390.

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
  • 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:

@szilardszaloki szilardszaloki requested a review from a team as a code owner June 7, 2021 15:39
zenparsing
zenparsing reviewed Jun 7, 2021
View reviewed changes
Copy link
Collaborator

@zenparsing zenparsing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'll build this branch and give it a run-through tomorrow.

components/brave_rewards/common/pref_names.h Outdated Show resolved Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/wallet/wallet.cc Outdated Show resolved Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/uphold/uphold_authorization.h Show resolved Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/uphold/uphold_authorization.cc Outdated Show resolved Hide resolved
@szilardszaloki szilardszaloki force-pushed the sszaloki-15390-removing-semi-verified-state branch from 088ad1c to dc5a50a Compare June 8, 2021 12:07
@szilardszaloki szilardszaloki force-pushed the sszaloki-15390-removing-semi-verified-state branch from c578f2b to e7a597b Compare June 8, 2021 20:58
bsclifton
bsclifton reviewed Jun 10, 2021
View reviewed changes
vendor/bat-native-ledger/src/bat/ledger/internal/uphold/uphold_authorization.cc
}

if (wallet->payment_id.empty()) {
BLOG(0, "Payment ID is empty!");
Copy link
Member

@bsclifton bsclifton Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not something we should address with this PR, but I'm curious (@zenparsing, @emerick, @szilardszaloki) if it would make sense to switch to using constants for the info levels?

brave-core/vendor/bat-native-ledger/src/bat/ledger/internal/logging/logging.h

Lines 45 to 53 in 5a4a600

// BAT Ledger verbose levels:
//
// 0 Error
// 1 Info
// 5 URL request
// 6 URL response
// 7 URL response (with large body)
// 8 Database queries
// 9 Detailed debugging (response headers, etc)

We could address by making an enum

enum LedgerLogLevel {
    Error,
    Info,
    UrlRequest = 5,
    UrlResponse,
    UrlResponseWithBody,
    DbQueries
};
// ...
if (wallet->payment_id.empty()) {
    BLOG(LedgerLogLevel::Error, "Payment ID is empty!");
    // ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a lot more informative.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like that idea.

bsclifton
bsclifton reviewed Jun 10, 2021
View reviewed changes
vendor/bat-native-ledger/src/bat/ledger/internal/uphold/uphold_authorization_unittest.cc
return "HappyPath";
}

INSTANTIATE_TEST_SUITE_P(
Copy link
Member

@bsclifton bsclifton Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice usage of parameterized tests! These are really cool 😄

For other reviewers, more info at https://source.chromium.org/chromium/chromium/src/+/main:third_party/googletest/src/googletest/include/gtest/gtest-param-test.h;l=71-93;drc=0a3a3a845e136a9a6ccd8e9b924b848840f22b7b

@szilardszaloki szilardszaloki force-pushed the sszaloki-15390-removing-semi-verified-state branch 3 times, most recently from d29db64 to 002d5f3 Compare June 14, 2021 15:53
@szilardszaloki
Performing wallet linking during Uphold authorization.
80d4900 
Adding GetAnonFunds(), OnGetAnonFunds(), TransferAnonFunds() and OnTransferAnonFunds() to UpholdAuthorization.
@szilardszaloki
Removing the WalletClaim class (wallet_claim.h and wallet_claim.cc).
9fc9b7e 
By removing the call to WalletClaim::Start() in Wallet::ClaimFunds(), we avoid fetching the balances for all the wallet types (anonymous, blinded, uphold and bitflyer) and avoid linking the Uphold account to the Brave wallet in the "generate wallet flow".
Fetching the balance of the anonymous wallet and the linking are already implemented in UpholdAuthorization::GetAnonFunds() and UpholdAuthorization::TransferAnonFunds(), respectively, in the "authorization flow".
@szilardszaloki
Deprecating kAnonTransferChecked.
21b6517 
It's cleared in the "authorization flow" to be able to tell if we need to do the linking in the "generate wallet flow".
It's set in the "generate wallet flow" on successful linking or when the device limit is reached.
Now that linking happens during authorization, we don't need it anymore.
@szilardszaloki
Adding uphold_authorization_unittest.cc and value-parameterized tests…
c088006 
… for UpholdAuthorization::GetAnonFunds().
@szilardszaloki
Refactoring UpholdAuthorization::GetAnonFunds() tests.
c519d85 
Adding ledger::database::Database mocking to support the "brave_wallet_is_not_created" scenario.
@szilardszaloki
Adding value-parameterized tests for UpholdAuthorization::TransferAno…
e1b1d3e 
…nFunds().
@szilardszaloki
Fine-tuning the "balance_server_error" scenario for UpholdAuthorizati…
27092f2 
…on::GetAnonFunds().
@szilardszaloki
Passing nullptr (instead of type::Balance::New())) to callback (Uphol…
9dc67c2 
…dAuthorization::OnGetAnonFunds()), if the Brave wallet's payment_id is empty.
@szilardszaloki
Removing BAP-related code (and adjusting test parameters).
1bdbf27
@szilardszaloki
Fixing lint findings.
38ca139
@szilardszaloki
Fixing lint findings in continuous-integration/post-init/pr-head.
918a4cc
@szilardszaloki
Addressing pull request findings.
3676eea 
Removing Wallet::ClaimFunds() and performing the call to Promotion::TransferTokens() directly in uphold_wallet.cc.
Renaming UpholdAuthorization::TransferAnonFunds() to UpholdAuthorization::LinkWallet().
@szilardszaloki
Fixing lint findings.
96e4218
@szilardszaloki
Addressing pull request findings.
fbf7d7c 
Removing brave_rewards::prefs::kAnonTransferChecked.
Making UpholdAuthorization::GetAnonFunds() and UpholdAuthorization::LinkWallet() private (using FRIEND_TEST_ALL_PREFIXES).
Updating copyright year in uphold_authorization_unittest.cc.
Adjusting TEST_P's and INSTANTIATE_TEST_SUITE_P's arguments in uphold_authorization_unittest.cc.
Passing drain_id by reference-to-const in UpholdWallet::OnTransferTokens().
@szilardszaloki
Addressing pull request findings.
6e1301e 
Removing #include "bat/ledger/option_keys.h" where applicable.
@szilardszaloki szilardszaloki force-pushed the sszaloki-15390-removing-semi-verified-state branch from 002d5f3 to 6e1301e Compare June 15, 2021 08:11
@bsclifton
Copy link
Member

Closing as this should have been covered in #9212

cc: @szilardszaloki in case I missed something 😄

@bsclifton bsclifton closed this Sep 9, 2021
@szilardszaloki szilardszaloki deleted the sszaloki-15390-removing-semi-verified-state branch February 16, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zenparsing zenparsing zenparsing left review comments

@emerick emerick emerick left review comments

@bsclifton bsclifton bsclifton left review comments

Assignees

@szilardszaloki szilardszaloki

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remove semi-verified state
4 participants
@szilardszaloki @bsclifton @emerick @zenparsing