Add Orchard support to Zcash keyring #22870
Conversation
cypt4
force-pushed
the
brave_37201_2
branch
2 times, most recently
from
1a8d839 to
2a2c6f2
Compare
github-actions
bot
added
the
CI/run-audit-deps
components/brave_wallet/browser/internal/hd_key_zip32_unittest.cc
Outdated
Show resolved
Hide resolved
components/brave_wallet/browser/internal/hd_key_zip32_unittest.cc
Outdated
Show resolved
Hide resolved
components/brave_wallet/browser/internal/hd_key_zip32_unittest.cc
Outdated
Show resolved
Hide resolved
|
@darkdh , @rillian - I've moved Orchard related code to the separate component, because of 2 reasons:
|
| # 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/. | ||
|
|
||
| import("//build/rust/rust_static_library.gni") |
There was a problem hiding this comment.
wait, how did this end up in components/brave_wallet/zcash? It was supposed to go in the existing brave_wallet/browser/zcash directory. Adding yet another top level directory is exactly the opposite of what we want here. I'll move this to the correct place in my changes
There was a problem hiding this comment.
Moved to components/brave_wallet/zcash/rs
|
|
||
| namespace brave_wallet { | ||
|
|
||
| HDKeyZip32::HDKeyZip32(std::unique_ptr<orchard::ExtendedSpendingKey> esk) |
There was a problem hiding this comment.
to me it's a bit confusing that HDKeyZip32 does not have zcash or orchard anywhere in the name directory, but that's an internal wallet thing I guess for you guys to sort out so just providing some feedback. Technically you could use ExtendedSpendingKey directly if you wanted, but I didn't want to make any architectual changes outside of the rust related code
|
|
||
| ExtendedSpendingKeyImpl::~ExtendedSpendingKeyImpl() = default; | ||
|
|
||
| std::unique_ptr<ExtendedSpendingKey> |
There was a problem hiding this comment.
this can be changed to base::expected with brave/brave-browser#37858
I just left it like this for now because I'm not sure what the errors should map to
| deps = [ | ||
| "//brave/components/brave_wallet/rust:rust_lib", | ||
| "//brave/third_party/rust/orchard/v0_7:lib", | ||
| "//third_party/rust/cxx/v1:lib", |
There was a problem hiding this comment.
this should not be needed as it automatically gets added to rust_static_library as a public dep when cxx_bindings are enabled. What was the exact error you saw?
| bytes: &[u8] | ||
| ) -> Box<OrchardExtendedSpendingKeyResult> { | ||
| Box::new(OrchardExtendedSpendingKeyResult::from( | ||
| ExtendedSpendingKey::master(&bytes).map_err(Error::from)) |
There was a problem hiding this comment.
nit: bytes is already a slice here, you don't need to pass it by reference.
| impl fmt::Display for Error { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match &self { | ||
| Error::Zip32(e) => write!(f, "Error: {}", e.to_string()), |
There was a problem hiding this comment.
nit: orchard implements the Display trait for Zip32Error, so this can just be
write!(f, "Zip32 Error: {e}"),No need to call to_string().
|
[puLL-Merge] - brave/brave-core@22870 Here is my review of the PR in the desired format: DescriptionThis PR adds support for Orchard shielded transactions in the Brave Wallet for ZCash. It implements the ZIP 32 protocol for deriving Orchard keys and allows generating unified shielded addresses that include an Orchard component. The main motivation seems to be to expand the ZCash functionality in the Brave Wallet to allow shielded transactions using the latest Orchard protocol, in addition to the existing transparent transactions. ChangesChanges
Security Hotspots
In summary, the main security considerations are around safe key generation, protecting private keys, and validating external inputs. The changes look well-structured and have test coverage. Getting an external security review of the cryptography code would provide additional assurance. |
This reverts commit 9e9c99e.
* Add Orchard support to Zcash keyring Resolves brave/brave-browser#32303 * Review fix * Review fix * Review fix * Review fix * Switch enable_orchard buildflag * Review fix * encapsulate rust code inside wrappers so we don't expose cxx types * Build fix * Build&Review fix --------- Co-authored-by: bridiver <[email protected]>
* Add Orchard support to Zcash keyring Resolves brave/brave-browser#32303 * Review fix * Review fix * Review fix * Review fix * Switch enable_orchard buildflag * Review fix * encapsulate rust code inside wrappers so we don't expose cxx types * Build fix * Build&Review fix --------- Co-authored-by: bridiver <[email protected]>
Resolves brave/brave-browser#32303
Pr targets brave_36128 until it is merged
Adds Orchard key generation to the ZCashKeyring.
Orchard keys are generated using Orchard crate. Orchard uses different keys generation strategy described here.
ExtendedSpendingKey is a sort of account private key. Using this key we can generate public or internal address then.
Also added orchard address support to Unified Addresses . Now we can generate address string using either transparent and orchard address parts.
Audit: https://github.com/brave/reviews/issues/1585
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 presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: