Move Zcash protobuf parsing to separate process #21627
Conversation
| if (!result) { | ||
| std::move(callback).Run(base::unexpected("Failed to start decoder")); | ||
| } | ||
| ptr->zcash_decoder_->ParseGetAddressUtxox( |
cypt4
force-pushed
the
brave_34561
branch
5 times, most recently
from
313fe64
to
2c45224
Compare
components/services/brave_wallet/zcash/zcash_decoder_unittest.cc
Outdated
Show resolved
Hide resolved
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
app/brave_generated_resources.grd
Outdated
| @@ -898,6 +898,10 @@ Or change later at <ph name="SETTINGS_EXTENIONS_LINK">$2<ex>brave://settings/ext | |||
| <message name="IDS_BRAVE_IPFS_ALWAYS_START_INFOBAR_NO" desc="Ignore proposition"> | |||
| Cancel | |||
| </message> | |||
| <!-- Brave Wallet --> | |||
| <message name="IDS_UTILITY_PROCESS_WALLET_UTILS_NAME" desc="The utility process running wallet utils"> | |||
| Brave Wallet Utils Service | |||
There was a problem hiding this comment.
Personally, I'd rather spell it out if the length is not an issue: Brave Wallet Utility Service
|
|
||
| import("//mojo/public/tools/bindings/mojom.gni") | ||
|
|
||
| static_library("zcash_decoder") { |
There was a problem hiding this comment.
component instead of static_library? I think we should have visibility setting for this target too, it should only be accessed by the service lib.
| # License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| # You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
||
| static_library("lib") { |
There was a problem hiding this comment.
component instead of static_library.
| LaunchBraveWalletUtilsService( | ||
| brave_wallet_utils_service_.BindNewPipeAndPassReceiver()); | ||
| brave_wallet_utils_service_.reset_on_disconnect(); | ||
| brave_wallet_utils_service_.reset_on_idle_timeout(base::Minutes(5)); |
There was a problem hiding this comment.
5 mins seems a bit long for an idle process? What's our use case here and can it be shorter?
There was a problem hiding this comment.
Such timeout should somehow relate to wallet's auto-lock timer(maybe just it's default value). Also makes sense to shutdown process when wallet gets locked.
There was a problem hiding this comment.
Logged follow-up issue brave/brave-browser#35568
| # License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| # You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
||
| static_library("cpp") { |
There was a problem hiding this comment.
- We don't need that many
component(). There should be not more than one. - We don't need that one
component()at all. Justsource_set()should be enough as amount of code is relatively small in this service, we have only one consumer and this consumer is not acomponent()itself
There was a problem hiding this comment.
Also let's not invent the wheel and just reuse architecture and layout of these services:
/src/components/services/patch/
/src/components/services/unzip/
/src/services/data_decoder/
There was a problem hiding this comment.
I'd tried to keep data decoder layout where services/../public/cpp folder contains service client implementation.
But i don't like how data_decoder service manages process launch model since it mixes external static delegate setup with internal buildflags.
I don't think it is a good idea to setup process launch model via static delegate since we would also need to ensure that it is initialized at the moment of usage.
So i've added buildflags in brave_wallet_utils_service to decide what process model to use.
Also data_decoder provides a set of static methods to use, but
- It always starts a new process on every such static call which could be surprising. This affects performance.
- Number of data_decoder parsing formats is limited, but in our case BraveWalletUtilsService could be extended with other coins format, rust libs, etc..
To overcome this issues, BraveWalletUtilsService acts as a factory for sub-component interfaces:
If client code wants to use separate process, it creates BraveWalletUtilsInstance manually.
If client code wants to reuse general process, it uses GetInstance() singletone.
User should explicitly request which sub-component to use. For ex. call CreateZCashDecoder.
| "//brave/components/services/brave_wallet:lib", | ||
| "//brave/components/services/brave_wallet/public/interfaces", | ||
| "//brave/components/services/brave_wallet/zcash:zcash_decoder", | ||
| "//mojo/public/cpp/system", |
There was a problem hiding this comment.
Is this used directly? mojo/public/cpp/bindings instead?
| #define BRAVE_COMPONENTS_SERVICES_BRAVE_WALLET_CONTENT_BRAVE_WALLET_UTILS_SERVICE_LAUNCHER_H_ | ||
|
|
||
| #include "mojo/public/cpp/bindings/pending_receiver.h" | ||
| #include "mojo/public/cpp/bindings/remote.h" |
| # License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| # You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
||
| static_library("content") { |
| deps = [ | ||
| "//brave/app:brave_generated_resources_grit", | ||
| "//brave/components/services/brave_wallet/public/interfaces", | ||
| "//brave/components/services/brave_wallet/zcash:zcash_decoder", |
| "//brave/components/services/brave_wallet/public/interfaces", | ||
| "//brave/components/services/brave_wallet/zcash:zcash_decoder", | ||
| "//content/public/browser", | ||
| "//mojo/public/cpp/system", |
There was a problem hiding this comment.
mojo/public/cpp/bindings instead?
| }; | ||
|
|
||
| struct RawTransaction { | ||
| array<uint8> data ; |
| void ZCashRpc::InitializeDecoderAndRunCallback( | ||
| base::OnceCallback<void(bool)> callback) { | ||
| if (IsConnected()) { | ||
| std::move(callback).Run(true); | ||
| return; | ||
| } | ||
|
|
||
| BraveWalletUtilsService::GetInstance()->CreateZCashDecoder( | ||
| zcash_decoder_.BindNewEndpointAndPassReceiver(), | ||
| base::BindOnce(&ZCashRpc::OnDecoderCreated, | ||
| weak_ptr_factory_.GetWeakPtr(), std::move(callback))); | ||
| } | ||
|
|
||
| void ZCashRpc::OnDecoderCreated(base::OnceCallback<void(bool)> callback) { | ||
| std::move(callback).Run(IsConnected()); | ||
| zcash_decoder_.reset_on_disconnect(); | ||
| } | ||
|
|
||
| bool ZCashRpc::IsConnected() { | ||
| return zcash_decoder_.is_bound(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why ZcashRpc is doing all of this?
Let's follow pattern of how data_decoder is used:
data_decoder::DataDecoder::ParseJsonIsolated(
fake_response, base::BindOnce(&PhotosService::OnJsonParsed,
weak_factory_.GetWeakPtr(), ""));
There was a problem hiding this comment.
BraveWalletUtilsService could provide various interfaces for different wallet parts, mixing their methods in a single interface would be messy
| GRrpcMessageStreamHandler::GRrpcMessageStreamHandler() = default; | ||
| GRrpcMessageStreamHandler::~GRrpcMessageStreamHandler() = default; | ||
|
|
There was a problem hiding this comment.
As far as I understand we also should move GRrpcMessageStreamHandler to zcash decoder
There was a problem hiding this comment.
GRrpcMessageStreamHandler is linked to SimpleUrlLoader, it just extracts chunks from incoming stream to decode. Not linked to decoding itself.
| @@ -14,6 +14,7 @@ | |||
| #include "base/functional/callback.h" | |||
| #include "base/strings/strcat.h" | |||
| #include "base/test/bind.h" | |||
| #include "brave/components/brave_wallet/common/proto/protobuf_utils.h" | |||
| public_deps = [ "//brave/components/api_request_helper" ] | ||
| public_deps = [ | ||
| "//brave/components/api_request_helper", | ||
| "//brave/components/services/brave_wallet/public/interfaces", |
There was a problem hiding this comment.
why this is a public dependency?
There was a problem hiding this comment.
Needed for tests, moved to test's deps
| std::optional<std::string> ResolveSerializedMessage( | ||
| const std::string& grpc_response_body) { | ||
| if (grpc_response_body.size() < kGrpcHeaderSize) { | ||
| return std::nullopt; | ||
| } | ||
| if (grpc_response_body[0] != kNoCompression) { | ||
| // Compression is not supported yet | ||
| return std::nullopt; | ||
| } | ||
| uint32_t size = 0; | ||
| base::ReadBigEndian( | ||
| reinterpret_cast<const uint8_t*>(&(grpc_response_body[1])), &size); | ||
|
|
||
| if (grpc_response_body.size() != size + kGrpcHeaderSize) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| return grpc_response_body.substr(kGrpcHeaderSize); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why this stays in common?
There was a problem hiding this comment.
Moved to services/brave_wallet/public as was discussed locally
| @@ -0,0 +1,4 @@ | |||
| include_rules = [ | |||
| "+content/public/browser/service_process_host.h", | |||
There was a problem hiding this comment.
This is typically +content/public/browser
| }; | ||
|
|
||
| struct GetAddressUtxosResponse { | ||
| array<ZCashUtxo> addressUtxos; |
Introduces BraveWalletUtilsService that is able to provide ZCashDecoder to convert protobufs to mojo structs and pass them to the mai. Resolves brave/brave-browser#34561 Review fix Review fix#2 Split service launcher Review fix#3 Review fix#4
|
[puLL-Merge] - brave/brave-core@21627 DescriptionThis PR introduces several key changes to the ChangesChanges
|
|
|
||
| ZCashDecoder::~ZCashDecoder() = default; | ||
|
|
||
| void ZCashDecoder::ParseRawTransaction(const std::string& data, |
There was a problem hiding this comment.
Can we perform additional data validation checks while we're parsing these structs here since it's still out of process? I checked the rust library and don't see them doing anything like this so it would mainly be a nice to have more than a blocker IMO.
Introduces BraveWalletUtilsService which is able to provide ZCashDecoder to convert protobufs to mojo structs and pass them to the main process. Resolves brave/brave-browser#34561
Upd. : Updated folder structure according review
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_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: