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

use mojo for wallet controllers #9527

Merged
merged 7 commits into from
Jul 23, 2021
Merged

use mojo for wallet controllers #9527

merged 7 commits into from
Jul 23, 2021

Conversation

bridiver
Copy link
Collaborator

@bridiver bridiver commented Jul 21, 2021 •
edited
Loading

Resolves brave/brave-browser#17105

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:

@bridiver bridiver requested review from iefremov and a team as code owners July 21, 2021 22:20
bridiver
bridiver commented Jul 21, 2021
View reviewed changes
chromium_src/ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
#if BUILDFLAG(BRAVE_WALLET_ENABLED)
#include "brave/ios/browser/api/wallet/brave_wallet_service_factory.h"
#endif
// #if BUILDFLAG(BRAVE_WALLET_ENABLED)
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 is all intentional per conversation with @kylehickinson

bridiver
bridiver commented Jul 21, 2021
View reviewed changes
browser/brave_wallet/android/brave_wallet_native_worker.cc
auto callback = base::BindOnce([](const std::string& mnemonic) {});
keyring_controller_->GetMnemonicForDefaultKeyring(std::move(callback));

// TODO(bridiver) - convert this to callback
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as ios these will be fixed in the follow-up. The current handling for callbacks in JNI is subject to multiple race conditions so not much point in fixing this now and @SergeyZhukovsky agrees

bbondy
bbondy reviewed Jul 21, 2021
View reviewed changes
browser/brave_wallet/android/brave_wallet_native_worker.h Outdated
mojo::Remote<brave_wallet::mojom::KeyringController> keyring_controller_;
mojo::Remote<brave_wallet::mojom::AssetRatioController>
asset_ratio_controller_;
mojo::Remote<brave_wallet::mojom::SwapController> swap_controller_;
Copy link
Member

Choose a reason for hiding this comment

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

swap_controller_ is unused in android right now

kylehickinson
kylehickinson approved these changes Jul 22, 2021
View reviewed changes
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

iOS++ will have a follow up PR re-adding the keyring controller as a keyed service

darkdh
darkdh reviewed Jul 22, 2021
View reviewed changes
components/brave_wallet/browser/keyring_controller_unittest.cc Outdated
EXPECT_EQ(controller.GetMnemonicForDefaultKeyring(), mnemonic);
controller.Unlock("brave", GetIgnoreResultCallback());
EXPECT_FALSE(controller.IsLocked());
// EXPECT_EQ(controller.GetMnemonicForDefaultKeyring(), mnemonic);
Copy link
Member

Choose a reason for hiding this comment

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

we can check the result using base::BindLambdaForTesting and capture a variable to make sure the callback is called instead of commenting these all out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, that was a mistake, I forgot to get back to these

components/brave_wallet/browser/keyring_controller_unittest.cc Outdated

// unlock with wrong password
EXPECT_FALSE(controller.Unlock("brave123"));
EXPECT_TRUE(controller.GetMnemonicForDefaultKeyring().empty());
controller.Unlock("brave123", GetIgnoreResultCallback());
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we check the callback to make sure it is called with false that indicate unlock failure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea, this is a mistake I missed

bbondy
bbondy approved these changes Jul 22, 2021
View reviewed changes
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

lgtm!

@bridiver bridiver force-pushed the wallet-mojo branch 3 times, most recently from 7adde67 to eb54ec6 Compare July 22, 2021 16:16
SergeyZhukovsky
SergeyZhukovsky approved these changes Jul 22, 2021
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++

darkdh
darkdh approved these changes Jul 22, 2021
View reviewed changes
components/brave_wallet/browser/keyring_controller.cc
}

void KeyringController::NotifyWalletBackupComplete() {
prefs_->SetBoolean(kBraveWalletBackupComplete, true);
Copy link
Member

Choose a reason for hiding this comment

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

I think this need to be cleared in KeyringController::Reset

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't change any logic here, if that's a problem, it's a bug in the original code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and it certainly does seem like a bug, just not one introduced in this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can you file an issue for it? Lots of people are waiting on this so I don't want to push it back any further for pre-existing issues and also my intention was not to change any logic so I didn't introduce any problems

Copy link
Member

Choose a reason for hiding this comment

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

I will take care of it in this issue
brave/brave-browser#17137

components/brave_wallet/browser/keyring_controller.h
// Must unlock before using this API otherwise it will return nullptr
HDKeyring* GetDefaultKeyring();
bool IsDefaultKeyringCreated();

bool IsLocked() const;
void Lock();
bool Unlock(const std::string& password);
// bool Unlock(const std::string& password);
Copy link
Member

Choose a reason for hiding this comment

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

this comment can be removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll remove in the follow-up so I don't trigger another CI run with the change

@bridiver
Copy link
Collaborator Author

linux test failures are unrelated

@bridiver bridiver merged commit 9ca35e9 into master Jul 23, 2021
@bridiver bridiver deleted the wallet-mojo branch July 23, 2021 00:00
@bridiver bridiver added this to the 1.29.x - Nightly milestone Jul 23, 2021
@kylehickinson kylehickinson mentioned this pull request Jul 26, 2021
Merged
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh approved these changes

@kylehickinson kylehickinson kylehickinson approved these changes

@bbondy bbondy bbondy approved these changes

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@iefremov iefremov Awaiting requested review from iefremov iefremov is a code owner

@deeppandya deeppandya Awaiting requested review from deeppandya

Assignees

@bridiver bridiver

Labels
None yet
Projects
None yet
Milestone
1.29.x - Release
Development

Successfully merging this pull request may close these issues.

Use mojo for wallet controllers
5 participants
@bridiver @kylehickinson @bbondy @darkdh @SergeyZhukovsky