-
Notifications
You must be signed in to change notification settings - Fork 895
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
[Zcash] Add shardtree crate #25092
[Zcash] Add shardtree crate #25092
Conversation
A Storybook has been deployed to preview UI for the latest push |
crate_name = "bitflags" | ||
epoch = "2" | ||
crate_type = "rlib" | ||
crate_root = "//brave/third_party/rust/chromium_crates_io/vendor/bitflags-2.6.0/src/lib.rs" |
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.
bitflags v2.6.0 is already vendored in chromium. You should be able to use that version instead. See //brave/third_party/rust/base64/v0_13/BUILD.gn
for a forwarding example.
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.
script/brave_license_helper.py
change 👍
@@ -1,7 +1,7 @@ | |||
# Copyright (c) 2019 The Brave Authors. All rights reserved. | |||
# Copyright (c) 2016 The Brave Authors. All rights reserved. |
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.
@fmarier can you take a look at the changes happening here?
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.
Hm, that's a bit weird. 2019 is the correct date.
# Copyright (c) 2016 The Brave Authors. All rights reserved. | |
# Copyright (c) 2019 The Brave Authors. All rights reserved. |
@@ -1,7 +1,7 @@ | |||
# Copyright (c) 2019 The Brave Authors. All rights reserved. | |||
# Copyright (c) 2016 The Brave Authors. All rights reserved. |
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.
Hm, that's a bit weird. 2019 is the correct date.
# Copyright (c) 2016 The Brave Authors. All rights reserved. | |
# Copyright (c) 2019 The Brave Authors. All rights reserved. |
# This Source Code Form is subject to the terms of the Mozilla Public | ||
# 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/. */ | ||
# You can obtain one at https://mozilla.org/MPL/2.0/. |
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.
That's fine.
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.
I took a dig through this dependency and not overly concerned with it. However there are two things that I think we should consider here:
- Reducing the usage of unwraps as there's quite a few and each is going to have to be individually reviewed to make sure it won't lead to browser crashes
- It would be useful to get
cargo audit
running in their CI pipeline to automatically detect reported vulnerabilities in their dependencies
Other than that, I'm not overly concerned with this dependency
// construct the subtree and cap based on the frontier containing the | ||
// witnessed position | ||
let (past_subtree, past_supertree) = self.insert_frontier_nodes::<C>( | ||
witness.tree().to_frontier().take().unwrap(), |
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.
I'm surprised at the number of unwrap fn calls in this library that aren't in tests. From what I'm seeing with a crude search is 22 instances of calls to unwrap that are not in tests. This is concerning that we're going to introduce unintentional panics which could crash the browser unless we can verify these code paths won't be hit.
I know previously we've accepted certain instances in limited circumstances, but this is a lot more than what we've brought in previously. Could we request that they update this to reduce our risk of crashes?
Resolves brave/brave-browser#40408
Adds shardtree crate + zcash_client_backend part which is used for shard tree seriazation ( https://github.com/brave/librustzcash/tree/zcash_client_backend_for_shielding )
Audit : https://github.com/brave/reviews/issues/1724
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 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: