-
Notifications
You must be signed in to change notification settings - Fork 900
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
Some wallet refactorings #27325
Some wallet refactorings #27325
Conversation
[puLL-Merge] - brave/brave-core@27325 Here's my review of the PR: DescriptionThis PR refactors Bitcoin-related code in Brave Wallet to improve code organization and security. The main changes include:
Security Hotspots
ChangesChangesBy filename:
sequenceDiagram
participant Wallet
participant BIP39
participant HDKey
participant HashUtils
Wallet->>BIP39: GenerateMnemonic()
BIP39->>HashUtils: ComputeHash()
HashUtils-->>BIP39: hash
BIP39-->>Wallet: mnemonic
Wallet->>BIP39: MnemonicToSeed()
BIP39->>HashUtils: PBKDF2()
HashUtils-->>BIP39: seed
BIP39-->>Wallet: seed
Wallet->>HDKey: SignMessage()
HDKey->>HashUtils: Blake2b/Keccak
HashUtils-->>HDKey: hash
HDKey-->>Wallet: signature
|
172a49e
to
0f17fe9
Compare
.Finalize(result.data()); | ||
|
||
return result; | ||
} | ||
|
||
std::vector<uint8_t> Blake2bHash( |
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.
Could this be reused in the ZCashSerializer too?
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.
will do!
@@ -78,7 +79,11 @@ std::optional<FilTransaction> FilTransaction::FromTxData( | |||
|
|||
auto address = FilAddress::FromAddress(tx_data->to); | |||
if (address.IsEmpty()) { | |||
address = FilAddress::FromFEVMAddress(is_mainnet, tx_data->to); | |||
auto eth_address = EthAddress::FromHex(tx_data->to); | |||
if (!eth_address.IsValid()) { |
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.
Are there plans to get rid of IsValid methods and use expected\optionl instead?
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 something I wanted to do but would be too much for this PR
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.
++
Resolves brave/brave-browser#43451
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: