Remove semi-verified state

[szilardszaloki]

Resolves brave/brave-browser#15390 and brave/brave-browser#16468.

• new Uphold wallet state machine:
  • previously there were 6 possible statuses in which an Uphold wallet could be:
    • NOT_CONNECTED, CONNECTED, VERIFIED, DISCONNECTED_NOT_VERIFIED, DISCONNECTED_VERIFIED, PENDING
  • now there are only 4 of them:
    • NOT_CONNECTED, VERIFIED, DISCONNECTED_VERIFIED, PENDING
  • created a diagram on how the new state machine works
• previously the user could tip in the CONNECTED state (that is, with an unverified Uphold wallet):
  • this state is gone, as is the possibility of tipping in the equivalent new state (which would be PENDING)
• Uphold wallet status migration:
  • because of the above (and because we have users, who transitioned into inconsistent states - such as the semi-VERIFIED state), we need a state migration
  • the migration itself is implemented in StateMigrationV10
  • implemented the wallet info endpoint (Get Wallet) client to be able to tell if the wallet is linked according to Rewards services
  • we have 24 browser tests for it (RewardsStateBrowserTest/UpholdStateMachine.Migration/*)
  • also created a diagram on how the state migration works
• Uphold authorization flow:
  • previously we did quite a couple of things in the authorization flow:
    • authorization with Uphold (the OAuth handshake)
    • getting the user object from Uphold
    • creating a BAT card for the user (if one didn't already exist)
  • now we only do the authorization part (the OAuth handshake) and we leave the rest to the generate wallet flow
  • wrote a thorough description on how the current implementation works and here's the state diagram on how the new implementation works
• Uphold generate wallet flow:
  • refactored BAT card handling in UpholdCard
  • removed anon_transfer_checked: in the old state machine, it might have made sense to try to save an API call on transferring the anon funds when the wallet was in the VERIFIED state (because the wallet was already verified, so why would we have initiated the anon funds transfer process). In the new state machine, however, if the wallet is in the VERIFIED state, we don't even get to the point where we would transfer the anon funds again, so keeping track of whether we have already done it, doesn't really make much sense.
  • here's a detailed explanation on how the current implementation works and here's the state diagram on how the new implementation works
• Uphold fetch balance flow:
  • previously the fetch balance flow returned 0.0 if the Uphold wallet status was CONNECTED, now it returns 0.0 if it's not VERIFIED
• added 4 new notifications:
  • Error: BAT unavailable:
    
  • Error: Blocked account:
    
  • Error: Pending account:
    
  • Error: Restricted account:
    
  • updated the Rewards panel

There are a couple of issues to address:

• the state machine is inherently asynchronous and this needs to be addressed (e.g. there can be multiple generate wallet flows running at the same time - not concurrently, but can have their subtasks interleaved)
• make sure other Uphold flows interact with the new state machine properly
• adjustments need to be made to the UI because of the new PENDING state (show different error states to the user, that is, why the wallet is in the PENDING state) - the current PENDING state UI does not suffice
• make sure the user is able to opt out of the PENDING state by manually disconnecting
• we have to migrate users who are already in the CONNECTED or the DISCONNECTED_NOT_VERIFIED state into valid states in the new state machine (since these states are being removed) and those, too, who transitioned to the VERIFIED state incorrectly (e.g. semi-VERIFIED state), or transitioned into an inconsistent state (e.g. VERIFIED, but uphold_wallet->token.empty() || uphold_wallet->address.empty())
• browser tests for StateMigrationV10
• full unit test coverage of the new authorization flow
• full unit test coverage of the new generate wallet flow

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:

• The associated issue milestone is set to the smallest version that the
  changes has landed on
• All relevant documentation has been updated, for instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
  • https://github.com/brave/brave-browser/wiki/QA-Guide
  • https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

Regression testing on Uphold use-cases.

[bsclifton]

Ran through the following scenario:

1. Compiled this locally
2. New profile
3. Claimed grant (staging). This sets wallet to 30 BAT
   
4. Verify via brave://rewards-internals that I'm in NOT_CONNECTED status
5. Connect to my sandbox account (verify wallet button on brave://rewards; put user/pass, enter auth code, follow verify email)
6. I am shown the following:
   

While my account is a valid account, I never did the ID verify / KYC process. Should this be PENDING? Or should it block the linking (removing that semi-verified state) and leave as NOT_CONNECTED. I can see from the code (and from when we talked earlier) the intention is to not share the address if not fully verified - just trying to understand that part 😄

If blocking is intended (which I think it is - this is the intention of the PR, right?), the modal text could use some polish. It says:
You need a minimum of 15 BAT to create an Uphold wallet. Please try again after you have verified your wallet with Uphold.

Maybe we should reword this to be something like:
The wallet you've signed into hasn't been verified yet. Please login to the account via Uphold.com and follow the ID verification steps. Once verified, you'll be able to link the account!
cc: @Miyayes

[szilardszaloki]

The basic idea is indeed to defer setting the wallet status to VERIFIED to the latest possible moment, that is, after a successful linkage. This is the point when we save the BAT card ID as the wallet's address as well. But the one you're seeing happens in the authorization flow, right before POST https://api.uphold.com/oauth2/token.
I have a fully KYC'd account, so I'm faking this scenario now, but I don't see a wallet_status_change event with a NOT_CONNECTED ==> PENDING transition in brave://rewards-internals Event logs. Did you?
You shouldn't be able to get into the PENDING state for sure.

[zenparsing]

In general, the nested callback pattern (where a sub-flow is executed with a callback which itself is bound to a callback for the outer flow) becomes difficult to read when the subflows are defined within the same file.

Totally fine to structure this flow how you think best, but FWIW I tend to prefer a more linear callback approach within a single file (or class).

[bsclifton]

The basic idea is indeed to defer setting the wallet status to VERIFIED to the latest possible moment, that is, after a successful linkage. This is the point when we save the BAT card ID as the wallet's address as well. But the one you're seeing happens in the authorization flow, right before POST https://api.uphold.com/oauth2/token.
I have a fully KYC'd account, so I'm faking this scenario now, but I don't see a wallet_status_change event with a NOT_CONNECTED ==> PENDING transition in brave://rewards-internals Event logs. Did you?
You shouldn't be able to get into the PENDING state for sure.
Great point - no, I did NOT see it go into PENDING status 😄

So I think we'd only need to consider the text change, which can help provide more insight. Since I have 30 available, the 15 BAT part is confusing (as the real root cause is needing to do verify). I would imagine the check for 15 BAT happens first - so this modal may be used in two places (we'd need to check)

[szilardszaloki]

TL;DR:
I followed up on @bsclifton's finding above and found that we shouldn't redirect to this modal at all (this applies to the currently live version, too).

---

Here's how the authorization goes:

1. We forward to the Rewards Service the actions produced by the user on the Rewards page here:
   

   brave-core/components/brave_rewards/resources/page/reducers/rewards_reducer.ts

   Lines 284 to 297 in 8224c9a

   	case types.PROCESS_REWARDS_PAGE_URL: {
   	const path = action.payload.path
   	const query = action.payload.query
   	const ui = state.ui
   	chrome.send('brave_rewards.processRewardsPageUrl', [path, query])
   	ui.modalRedirect = 'show'
   	state = {
   	...state,
   	ui
   	}
   	break
   	}

2. The authorization action gets picked up by the Rewards Service here:
   

   brave-core/components/brave_rewards/browser/rewards_service_impl.cc

   Lines 2887 to 2901 in 8224c9a

   	if (action == "authorization") {
   	if (wallet_type == ledger::constant::kWalletUphold ||
   	wallet_type == ledger::constant::kWalletBitflyer) {
   	ExternalWalletAuthorization(
   	wallet_type,
   	query_map,
   	base::BindOnce(
   	&RewardsServiceImpl::OnProcessExternalWalletAuthorization,
   	AsWeakPtr(),
   	wallet_type,
   	action,
   	std::move(callback)));
   	return;
   	}
   	}

3. The Rewards Service calls out to the Bat Ledger Service via Mojo

4. UpholdAuthorization::Authorize() will eventually get called:
   

   brave-core/vendor/bat-native-ledger/src/bat/ledger/internal/uphold/uphold_authorization.cc

   Lines 32 to 60 in 8224c9a

   	void UpholdAuthorization::Authorize(
   	const base::flat_map<std::string, std::string>& args,
   	ledger::ExternalWalletAuthorizationCallback callback) {
   	auto wallet = GetWallet(ledger_);
   	if (!wallet) {
   	BLOG(0, "Wallet is null");
   	callback(type::Result::LEDGER_ERROR, {});
   	return;
   	}
   	const auto current_one_time = wallet->one_time_string;
   	// we need to generate new string as soon as authorization is triggered
   	wallet->one_time_string = util::GenerateRandomHexString();
   	const bool success = ledger_->uphold()->SetWallet(wallet->Clone());
   	if (!success) {
   	callback(type::Result::LEDGER_ERROR, {});
   	return;
   	}
   	auto it = args.find("error_description");
   	if (it != args.end()) {
   	const std::string message = args.at("error_description");
   	BLOG(1, message);
   	if (message == "User does not meet minimum requirements") {
   	callback(type::Result::NOT_FOUND, {});
   	return;
   	}

   and this is the code we're talking about:
   

   brave-core/vendor/bat-native-ledger/src/bat/ledger/internal/uphold/uphold_authorization.cc

   Lines 53 to 60 in 8224c9a

   	auto it = args.find("error_description");
   	if (it != args.end()) {
   	const std::string message = args.at("error_description");
   	BLOG(1, message);
   	if (message == "User does not meet minimum requirements") {
   	callback(type::Result::NOT_FOUND, {});
   	return;
   	}

   Now, as per the Uphold API Reference, Brave should redirect the user to https://sandbox.uphold.com/authorize/<client_id> (at least in the sandbox environment) and Uphold will redirect them back to Brave with a temporary code and with the state Brave provided earlier.
   
   If authorization succeeds, the redirect URL looks something like this:
   brave://rewards/uphold/authorization?code=3a2455212f689f57259f25fcb6bd9bd4b1a0e6ad&state=0D10BF35F853244A8A4C44E789D348ACF4029A332682520D64B8B56D39BB795A and args contains code and state at that point. @bsclifton's args (and actually mine, too, since it just came to my mind that I also have a non-KYC'd test account 🙂) contained an error_description ("User does not meet minimum requirements"), which doesn't make too much sense, since we both had 30 BATs and 15 is the minimum requirement.
   
   And here comes the twist: there's no check for 15 BATs happening here, since the message "User does not meet minimum requirements" refers to the OAuth handshake and is part of the URL to which Uphold redirects non-KYC'd users:
   brave://rewards/uphold/authorization?error=invalid_request&error_description=User%20does%20not%20meet%20minimum%20requirements (this feature was requested by us to be able to ensure that non-KYC'd users cannot login - reached out to them, just to make sure).

5. type::Result::NOT_FOUND gets processed here:
   

   brave-core/components/brave_rewards/resources/page/reducers/rewards_reducer.ts

   Lines 298 to 308 in 8224c9a

   	case types.ON_PROCESS_REWARDS_PAGE_URL: {
   	const data = action.payload.data
   	const ui = state.ui
   	chrome.send('brave_rewards.getExternalWallet')
   	// NOT_FOUND
   	if (data.result === 9) {
   	ui.modalRedirect = 'batLimit'
   	break
   	}

   and the modal gets put together here:
   

   brave-core/components/brave_rewards/resources/page/components/settingsPage.tsx

   Lines 252 to 265 in 8224c9a

   	if (ui.modalRedirect === 'batLimit') {
   	// NOTE: The minimum BAT limit error is currently Uphold-specific
   	const text = getLocale('redirectModalBatLimitText')
   	return (
   	<ModalRedirect
   	id={'redirect-modal-bat-limit'}
   	titleText={getLocale('redirectModalBatLimitTitle')}
   	errorText={text.replace('$1', String(getMinimumBalance(walletType)))}
   	buttonText={getLocale('redirectModalClose')}
   	walletType={walletType}
   	onClick={this.actions.hideRedirectModal}
   	/>
   	)
   	}

   and here's where 15 comes from:
   

   brave-core/components/brave_rewards/resources/page/components/connect_wallet_modal.tsx

   Lines 18 to 23 in 8224c9a

   	export function getMinimumBalance (provider: string) {
   	switch (provider) {
   	case 'uphold': return 15
   	default: return 0
   	}
   	}

Currently, I don't see why we should redirect to this modal, since I don't see an enforced minimum requirement on our end. What do you think?
cc: @Miyayes

[jumde]

• @szilardszaloki - Before merge could you verify if this change fixes this issue: Show error modal when wallet verify fails because user tries verifying 2 different accounts at same custodian with same Wallet ID brave-browser#16468 - Thanks!

[szilardszaloki]

• @szilardszaloki - Before merge could you verify if this change fixes this issue: Show error modal when wallet verify fails because user tries verifying 2 different accounts at same custodian with same Wallet ID brave-browser#16468 - Thanks!

@jumde Sure thing!🙂
This is not a special case with the new state machine though, since the authorization flow already prevents the user from re-authorizing (if they try to re-authorize, it's a no-op - even with the same Uphold account). But if we want to redirect to a modal and show an error message, we have to create two of them: one for the PENDING state ("Hmm, it looks like your Brave Rewards wallet is already pending...") and one for the VERIFIED state ("Hmm, it looks like your Brave Rewards wallet has already been verified...").
cc: @Miyayes

[szilardszaloki]

FYI: the Uphold wallet state diagram just got updated.

[szilardszaloki]

continuous-integration/post-init/pr-head is getting marked as unstable due to pylint findings (no Python files changed here).
As per this Slack convo, they're fine to ignore for now.