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.

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

Remove semi-verified state #9032

Closed

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:

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.

@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
}

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

@bsclifton bsclifton Jun 10, 2021

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?

// 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.

return "HappyPath";
}

INSTANTIATE_TEST_SUITE_P(
Copy link
Member

@bsclifton bsclifton Jun 10, 2021

Choose a reason for hiding this comment

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

@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
Adding GetAnonFunds(), OnGetAnonFunds(), TransferAnonFunds() and OnTransferAnonFunds() to UpholdAuthorization.
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".
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.
Adding ledger::database::Database mocking to support the "brave_wallet_is_not_created" scenario.
…dAuthorization::OnGetAnonFunds()), if the Brave wallet's payment_id is empty.
Removing Wallet::ClaimFunds() and performing the call to Promotion::TransferTokens() directly in uphold_wallet.cc.
Renaming UpholdAuthorization::TransferAnonFunds() to UpholdAuthorization::LinkWallet().
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().
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove semi-verified state
4 participants