Refactor signing code #25386

supermassive · 2024-08-30T02:50:35Z

Resolves brave/brave-browser#41121

Refactored parts of core/ui responsible of signing transactions/messages:

Merged SignTransactionRequest and SignAllTransactionsRequest into SignSolTransactionsRequest. This is SOL only. Both dApp signTransaction and signAllTransactions are still there but internally there is only one implementation.
Refactored SimulationService to run SOL simulations separately for SignSolTransactionsRequest and TransactionInfo. Also instead of sending ids instead of corresponding payloads from ui to core.
TxService.GetTransactionMessageToSign is moved to proxies. Each explicitly specifies what is returned (vs mojo union).
Added EthereumSignatureVRS EthereumSignatureBytes SolanaSignature FilecoinSignature to distinguish signature type in type-safe manner.

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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Browser with this PR should behave in the same way as previous versions for these scenarios:

Adding HW accounts for Ledger/Trezor for ETH, SOL, FIL
Transaction signing with Ledger for ETH, SOL, FIL
Message signing with Ledger for ETH, SOL
Transaction signing with Trezor for ETH
Message signing with Trezor for ETH

brave-builds · 2024-09-10T08:49:45Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-09-10T09:00:33Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-09-10T09:09:44Z

A Storybook has been deployed to preview UI for the latest push

yrliou

core code LGTM but this needs an issue and a manual test plan filled, thanks.

goodov

sources-gni-reviewers lgtm

simoarpe · 2024-09-13T10:14:42Z

android/java/org/chromium/chrome/browser/app/BraveActivity.java

-                return;
-            }
-            maybeShowPendingTransactions();
-            maybeShowSignTxRequestLayout();


Maybe I'm not grasping the whole picture here, but what happened to the ETH pending transactions? The method maybeShowSignTxRequestLayout() was in charge of processing both (SOL + ETH pending transaction requests).

maybeShowSignTxRequestLayout -> SIGN_TRANSACTION
maybeShowSignAllTxRequestLayout -> SIGN_ALL_TRANSACTIONS
were both in charge of SOL transactions only.

Now both cases are covered by a single type
maybeShowSignSolTransactionsRequestLayout -> SIGN_SOL_TRANSACTIONS

Okay, then there's something sketchy going on: the method maybeShowSignSolTransactionsRequestLayout is called for all transactions (ETH too). ETH pending transactions need it otherwise the panel won't show up.

Here is a quick recording. Precondition: 1 ETH pending transaction. Observe how the breakpoint for the method maybeShowSignSolTransactionsRequestLayout is hit and processed anyway.

Screencast.from.13-09-2024.13.53.02.webm

Here is another recording, same ETH pending transaction, but the method maybeShowSignSolTransactionsRequestLayout was commented. Result: the wallet panel is never displayed.

Screencast.from.13-09-2024.13.59.18.webm

Let's continue on DMs.

simoarpe · 2024-09-16T09:55:01Z

android/java/org/chromium/chrome/browser/app/BraveActivity.java

-
-    private void maybeShowSignAllTxRequestLayout() {
+    // TODO(apaymyshev): refactor this to have a better name.
+    private void maybeShowSignSolTransactionsRequestLayout() {


I'm going to take care of this in my next PR, entangle the chains and and detach the logic that is not related to SOL transactions!

simoarpe

👍 Android part looks good!

onyb

frontend ++

Nice work on the typing. Please add a test plan covering the common hardware wallet signing scenarios.

onyb · 2024-09-17T06:00:47Z

components/brave_wallet_ui/components/extension/connect-hardware-wallet-panel/index.tsx

@@ -99,11 +97,16 @@ export const ConnectHardwareWalletPanel = ({
  const request = signMessageData?.at(0)
  const isSigning = request && request.id !== -1

-  const { account: messageAccount } = useAccountQuery(request?.accountId)
+  const { account: singMessageAccount } = useAccountQuery(request?.accountId)


github-actions · 2024-09-18T06:23:00Z

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

Description

This PR makes significant changes to the Brave Wallet codebase, particularly focusing on the handling of Solana transactions and hardware wallet interactions. The changes streamline the process of signing Solana transactions, improve type safety, and refactor several components to handle hardware wallet signatures more consistently across different cryptocurrencies.

Changes

Changes

brave_wallet.mojom:
- Removed SignTransactionRequest and SignAllTransactionsRequest, replaced with SignSolTransactionsRequest.
- Introduced new types for hardware wallet signatures: EthereumSignatureVRS, EthereumSignatureBytes, SolanaSignature, and FilecoinSignature.
- Updated various method signatures to use these new types.
brave_wallet_service.cc and brave_wallet_service.h:
- Replaced methods for handling separate sign transaction and sign all transactions requests with a single method for Solana transactions.
- Updated signature processing methods to use the new signature types.
eth_transaction.cc and eth_transaction.h:
- Modified ProcessVRS method to work with byte arrays instead of strings.
eth_tx_manager.cc and eth_tx_manager.h:
- Updated methods to use the new EthereumSignatureVRS type.
solana_transaction.cc and solana_transaction.h:
- Modified to work with the new SolanaSignature type.
Various UI components (React):
- Updated to work with the new SignSolTransactionsRequest instead of separate sign transaction and sign all transactions requests.
- Modified hardware wallet interaction logic to use the new signature types.
iOS changes:
- Updated CryptoStore, SignTransactionView, and other components to work with the new SignSolTransactionsRequest.
- Modified mock objects and preview content to reflect the new API changes.

Possible Issues

The changes are extensive and touch many parts of the codebase, which could introduce integration issues.
Some platform-specific code (e.g., iOS) may need further testing to ensure compatibility with the new types and methods.

Security Hotspots

The handling of hardware wallet signatures has changed significantly. Ensure that all signature verification processes are still secure and correctly implemented across all supported cryptocurrencies.
The removal of separate methods for signing single and multiple transactions in favor of a unified approach should be carefully reviewed to ensure it doesn't introduce any security vulnerabilities.

github-actions bot assigned supermassive Aug 30, 2024

github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet feature/web3/wallet/core labels Aug 30, 2024

supermassive force-pushed the refactor_signing_code branch 3 times, most recently from f578b42 to 0c60869 Compare September 1, 2024 13:02

supermassive force-pushed the refactor_signing_code branch 4 times, most recently from 98134f8 to c5993a3 Compare September 10, 2024 08:14

github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Sep 11, 2024

supermassive force-pushed the refactor_signing_code branch from 16f257f to 04ce21e Compare September 11, 2024 15:23

github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Sep 11, 2024

supermassive marked this pull request as ready for review September 12, 2024 02:10

supermassive requested review from a team as code owners September 12, 2024 02:10

supermassive force-pushed the refactor_signing_code branch from 04ce21e to 451ffaf Compare September 12, 2024 05:26

github-actions bot added the puLL-Merge label Sep 12, 2024

StephenHeaps approved these changes Sep 12, 2024

View reviewed changes

yrliou approved these changes Sep 12, 2024

View reviewed changes

supermassive force-pushed the refactor_signing_code branch from 4fbdfbe to e2b7211 Compare September 13, 2024 03:22

goodov approved these changes Sep 13, 2024

View reviewed changes

simoarpe reviewed Sep 13, 2024

View reviewed changes

supermassive force-pushed the refactor_signing_code branch from e2b7211 to 27aee45 Compare September 16, 2024 09:42

simoarpe reviewed Sep 16, 2024

View reviewed changes

simoarpe approved these changes Sep 16, 2024

View reviewed changes

onyb approved these changes Sep 17, 2024

View reviewed changes

supermassive force-pushed the refactor_signing_code branch from 27aee45 to 3e6d74a Compare September 17, 2024 11:02

supermassive and others added 9 commits September 18, 2024 13:21

Refactor signing code

bfc8d15

fix ci

2b01093

Fix iOS SignTransactionView navigation title.

0c4d60c

update

934763f

fix review

193ccf9

fix review

1bbb141

fix ci

9a2f078

fix trezor issues

efa8fc9

fix eslint

1284c5f

supermassive force-pushed the refactor_signing_code branch from 8016689 to 1284c5f Compare September 18, 2024 06:22

supermassive merged commit 2a4e387 into master Sep 18, 2024
17 of 18 checks passed

supermassive deleted the refactor_signing_code branch September 18, 2024 08:57

github-actions bot added this to the 1.72.x - Nightly milestone Sep 18, 2024

srirambv mentioned this pull request Nov 6, 2024

Refactor wallet's hardware signing code brave/brave-browser#41121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor signing code #25386

Refactor signing code #25386

supermassive commented Aug 30, 2024 •

edited

Loading

brave-builds commented Sep 10, 2024

brave-builds commented Sep 10, 2024

brave-builds commented Sep 10, 2024

yrliou left a comment

goodov left a comment

simoarpe Sep 13, 2024

supermassive Sep 13, 2024

simoarpe Sep 13, 2024

simoarpe Sep 16, 2024

simoarpe left a comment

onyb left a comment

onyb Sep 17, 2024

github-actions bot commented Sep 18, 2024

Changes

Refactor signing code #25386

Refactor signing code #25386

Conversation

supermassive commented Aug 30, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Sep 10, 2024

brave-builds commented Sep 10, 2024

brave-builds commented Sep 10, 2024

yrliou left a comment

Choose a reason for hiding this comment

goodov left a comment

Choose a reason for hiding this comment

simoarpe Sep 13, 2024

Choose a reason for hiding this comment

supermassive Sep 13, 2024

Choose a reason for hiding this comment

simoarpe Sep 13, 2024

Choose a reason for hiding this comment

simoarpe Sep 16, 2024

Choose a reason for hiding this comment

simoarpe left a comment

Choose a reason for hiding this comment

onyb left a comment

Choose a reason for hiding this comment

onyb Sep 17, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2024

Description

Changes

Possible Issues

Security Hotspots

supermassive commented Aug 30, 2024 •

edited

Loading