Refactor how eth_requestAccounts works to make it more aligned with MetaMask #11459
Conversation
bbondy
changed the title
| UpdateKnownAccounts(); | ||
| if (pending_unlock_callback_) { | ||
| RequestEthereumPermissions(std::move(pending_unlock_callback_)); | ||
| pending_unlock_callback_ = RequestEthereumPermissionsCallback(); |
There was a problem hiding this comment.
pending_unlock_callback_ naming is confusing, I was a bit confused at first and didn't realize it's the callback for RequestEthereumPermissions, maybe pending_request_ethereum_permissions_callback_ and a comment in the header to describe it's pending due to wallet is locked and we need to unlock first?
And could you explain to me why do we need to construct a callback and assign to this member when the stored callback is used?
There was a problem hiding this comment.
Fixed in 4e506546f50e86fabd1d77c3ff2fc231a965f2f8
| // If the call was successful but the keyring is locked, then request an | ||
| // unlock. After the unlock happens a new request will be made. | ||
| if (error == mojom::ProviderError::kSuccess && | ||
| keyring_controller_->IsLocked()) { | ||
| if (pending_unlock_callback_) { | ||
| std::move(callback).Run( | ||
| std::vector<std::string>(), | ||
| mojom::ProviderError::kUserRejectedRequest, | ||
| l10n_util::GetStringUTF8(IDS_WALLET_ALREADY_IN_PROGRESS_ERROR)); | ||
| return; | ||
| } | ||
| pending_unlock_callback_ = std::move(callback); | ||
| keyring_controller_->RequestUnlock(); | ||
| delegate_->ShowPanel(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why do we need to run RequestEthereumPermissions once and handle the unlock logic here? Can this block be moved to RequestEthereumPermissions?
There was a problem hiding this comment.
I had that originally but there was some test failures that way. For example when no wallet is setup we want to open the onboarding. Basically we only want to do this request unlock logic when things would be successful but the keyring is locked.
There was a problem hiding this comment.
I see, thanks for the explanation.
| // Obtains if there's a pending unlock request | ||
| // Note that there is no need for an API to notify unlocked | ||
| // because the KeyringControllerObserver Unlocked event can be used for that. | ||
| GetPendingUnlockRequest() => (bool pending); |
There was a problem hiding this comment.
nit: How about HasPendingUnlockRequest?
There was a problem hiding this comment.
Fixed in 4e506546f50e86fabd1d77c3ff2fc231a965f2f8
bbondy
force-pushed
the
permissions
branch
4 times, most recently
from
1efc7c9
to
83611a2
Compare
| pending_request_ethereum_permissions_callback_ = | ||
| RequestEthereumPermissionsCallback(); |
There was a problem hiding this comment.
nit: I'll prefer this to be taken out if it turns out to be unnecessary.
If the selected account does not match any other account, then return all accounts.
…thJsonRpcController So we no longer need to manually create controller instances just for setting the url loader in tests.
This work starts with a small refactor to pass error codes for provider instead of only a bool success code.
It then moves the provider errors into mojo.
If an account is selected and it is connected, eth_request and eth_accounts will return only that account to be consistent with MetaMask.
If an account is connected but the wallet is locked, we'll now ask to unlock the wallet instead of showing a permission prompt with the other accounts you can connect to. Douglas is doing some UI changes to make it easier to switch which account is connected in a separate PR.
Finally we return request already in progress now instead of stacking requests.
Resolves brave/brave-browser#19750
Resolves brave/brave-browser#19401
Resolves brave/brave-browser#19641
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: