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.

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

Re-land zcash keyring support #23383

Merged
merged 4 commits into from
May 6, 2024
Merged

Re-land zcash keyring support #23383

merged 4 commits into from
May 6, 2024

Conversation

cypt4
Copy link
Collaborator

@cypt4 cypt4 commented May 2, 2024

Resolves brave/brave-browser#32303

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • 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 wiki
    • npm run presubmit wiki, 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:

@cypt4 cypt4 requested review from a team and bridiver as code owners May 2, 2024 08:29
@github-actions github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) feature/web3/wallet feature/web3/wallet/core puLL-Merge labels May 2, 2024
@cypt4
Copy link
Collaborator Author

cypt4 commented May 2, 2024

/build ios

cypt4 and others added 4 commits May 6, 2024 17:09
* 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 <34129+bridiver@users.noreply.github.com>
@cypt4 cypt4 force-pushed the brave_37201_3 branch from d4a81e8 to a920aa2 Compare May 6, 2024 10:31
Copy link
Contributor

github-actions bot commented May 6, 2024

[puLL-Merge] - brave/brave-core@23383

Here is my review of the pull request:

Description

This PR adds support for Orchard key generation and address derivation in the Brave Wallet. The main motivation seems to be expanding Brave Wallet's ZCash capabilities to enable shielded transactions through the Orchard protocol.

Changes

Changes

  • components/brave_wallet/browser/BUILD.gn, components/brave_wallet/browser/internal/BUILD.gn
    • Added new dependencies and source files related to Orchard key generation
  • components/brave_wallet/browser/internal/hd_key_zip32.cc, components/brave_wallet/browser/internal/hd_key_zip32.h, components/brave_wallet/browser/internal/hd_key_zip32_unittest.cc
    • Implemented Orchard key generation logic following ZIP-32 specification
  • components/brave_wallet/browser/zcash/rust/*
    • Added Rust implementation of Orchard extended spending key derivation
  • components/brave_wallet/browser/zcash/zcash_keyring.cc, components/brave_wallet/browser/zcash/zcash_keyring.h
    • Updated to derive and store Orchard keys
    • Added new methods to get shielded addresses and raw Orchard address bytes
  • components/brave_wallet/common/*
    • Defined feature flag for enabling Orchard
    • Updated common utilities to check Orchard feature flag
  • components/brave_wallet/common/zcash_utils.cc, components/brave_wallet/common/zcash_utils.h
    • Added utilities for parsing and constructing unified addresses with Orchard
  • Unit tests
    • Added tests for Orchard key derivation and unified address handling

Security Hotspots

  1. Orchard key generation and derivation
    • Ensure the cryptographic operations are implemented securely without any bugs or weaknesses. The Rust implementation should be carefully audited.
  2. Parsing and construction of unified addresses with Orchard
    • Vulnerabilities in the parsing logic could potentially allow creation of malformed addresses. The parsing and serialization code should be properly tested.
  3. Feature flag for enabling Orchard
    • Access to the Orchard functionality is gated by a feature flag. Verify the flag is set securely and can't be bypassed.

Overall, this is a significant change that introduces a complex new ZCash protocol to the wallet. The implementation should undergo thorough security testing and auditing, especially the core cryptographic components. Users' privacy and funds security depends on the robustness of the Orchard integration.

@bridiver
Copy link
Collaborator

bridiver commented May 6, 2024

This is a re-land disabled by default and @brave/crypto-wallets-core already approved the original PR so force merging

@bridiver bridiver changed the title Reapply zcash keyring support Reland zcash keyring support May 6, 2024
@bridiver bridiver enabled auto-merge (squash) May 6, 2024 16:16
@bridiver bridiver disabled auto-merge May 6, 2024 16:16
@bridiver bridiver merged commit f5dc515 into master May 6, 2024
19 checks passed
@bridiver bridiver deleted the brave_37201_3 branch May 6, 2024 16:16
@bridiver bridiver changed the title Reland zcash keyring support Re-land zcash keyring support May 6, 2024
@github-actions github-actions bot added this to the 1.67.x - Nightly milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) feature/web3/wallet/core feature/web3/wallet puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ZCash] Implement Orchard Keyring
2 participants