-
Notifications
You must be signed in to change notification settings - Fork 896
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
Implement ZCash address discovery #21150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iOS++
needs-security-label removed since feature is under the flag. And will have separate sec-review before the release |
components/brave_wallet/browser/zcash/zcash_wallet_service_tasks.h
Outdated
Show resolved
Hide resolved
should_resume = true; | ||
} | ||
} | ||
data_ = data_.substr(pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need extra pos variable.
data_ = std::string(data_view);
should be enough
} | ||
|
||
if (data_to_read_ && data.size() >= (kGrpcHeaderSize + *data_to_read_)) { | ||
if (!ProcessMessage(data.substr(0, kGrpcHeaderSize + *data_to_read_))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is header part of message?
should that be data.substr(kGrpcHeaderSize, *data_to_read_)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is
[puLL-Merge] - brave/brave-core@21150 DescriptionThis pull request implements additional functionalities and improvements for the Zcash component within the Brave Wallet, focusing on the handling of Zcash accounts, receiving addresses, and the streamlining of transactions and discovery processes. This change aims to provide better support for Zcash operations and enhance the overall user experience while interacting with Zcash-related features in the Brave Wallet. ChangesHere's a breakdown of the changes organized by file: BUILD files
Protobuf files
C++ Source files
Unit Tests
Security Hotspots
|
namespace brave_wallet { | ||
|
||
class GetTransparentUtxosContext | ||
: public base::RefCountedThreadSafe<GetTransparentUtxosContext> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these RefCountedThreadSafe? Use of ref-counting should be limited to cases where something is truly shared and also this class does not appear to be thread safe
https://www.chromium.org/developers/coding-style/important-abstractions-and-data-structures/#scoped_refptrt-baserefcounted-baserefcountedthreadsafe
} | ||
|
||
void DiscoverNextUnusedZCashAddressTask::WorkOnTask() { | ||
if (!callback_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is passed an empty callback it will leak
Resolves brave/brave-browser#33662
Resolves
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: