Add eth_signTransaction and eth_sendRawTransaction #16247

darkdh · 2022-12-06T02:14:18Z

Add eth_sendRawTransaction which doesn't need confirmation like MetaMask because it doesn't have account actions
Add eth_signTransaction by adding additional signOnly and signedTransaction passed along with eth_sendTransaction code flow
Use idl for transaction request params parsing
Introduce a new state "Signed" which indicates the transaction is signed but not submitted. When we confirm that dapps has broadcast it into blocks, it will be moved to "Confirmed" state. "Signed" transaction won't be taken into account when calculating nonce and it will be dropped when its nonce became too low.
Confirmation panel is same as same transaction but post confirmation panel is different

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one

eth_signTransaction is basically same as eth_sendTransaction but without sending. And eth_sendRawTransaction is just forwarding request to rpc endpoint without account involved

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 lint, 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:

Normal flow

Setup wallet
Setup https://github.com/bbondy/eth-manual-tests
Change to any test network and visit http://localhost:8081/request.html
Click ethereum.enable and grant permission
Fill to address next to eth_signTransaction and click it
Check transactions on brave://wallet/crypto/portfolio/ETH and there should be a "Signed" transaction
Go back to the console of http://localhost:8081/request.html
Paste the return signed transaction in console to eth_sendRawTransaction and click
Check transactions on brave://wallet/crypto/portfolio/ETH and previously "Signed" transaction should be "Confirmed" after a while

Nonce drop

Setup wallet
Setup https://github.com/bbondy/eth-manual-tests
Change to any test network and visit http://localhost:8081/request.html
Click ethereum.enable and grant permission
Fill to address next to eth_signTransaction and click it
Fill to address next to eth_signTransaction (1559) and click it
Check transactions on brave://wallet/crypto/portfolio/ETH and there should be TWO "Signed" transactions
Go back to the console of http://localhost:8081/request.html
Paste one of the return signed transactions in console to eth_sendRawTransaction and click
Check transactions on brave://wallet/crypto/portfolio/ETH and previously one "Signed" transaction should be "Confirmed" and one "Signed" transaction should be deleted after a while

android/android_browser_tests.gni

brave-builds · 2022-12-06T19:34:52Z

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

brave-builds · 2022-12-06T20:25:32Z

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

brave-builds · 2022-12-06T21:59:17Z

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

wchen342

++

bbondy

Looks good, just some questions and a minor text correction.

bbondy · 2022-12-07T18:14:29Z

components/resources/wallet_strings.grdp

@@ -510,6 +511,8 @@
  <message name="IDS_BRAVE_WALLET_ACCOUNT_FILTER_ALL_ACCOUNTS" desc="Account Filter Selector All Accounts">All Accounts</message>
  <message name="IDS_BRAVE_WALLET_TRANSACTION_SUBMITTED_TITLE" desc="Transaction submitted title">Transaction submitted</message>
  <message name="IDS_BRAVE_WALLET_TRANSACTION_SUBMITTED_DESCRIPTION" desc="Transaction submitted description">Transaction has been successfully sent to the network and awaits confirmation.</message>
+  <message name="IDS_BRAVE_WALLET_TRANSACTION_SIGNED_TITLE" desc="Transaction signed title">Transaction signed</message>
+  <message name="IDS_BRAVE_WALLET_TRANSACTION_SIGNED_DESCRIPTION" desc="Transaction signed description">Transaction has been signed and will be sent to network by dapps and awaits confirmation</message>


The transaction has been signed and will be sent to the network by the DApp

bbondy · 2022-12-07T18:16:10Z

components/brave_wallet/common/brave_wallet.mojom

@@ -982,7 +984,8 @@ enum TransactionStatus {
  Submitted = 3,
  Confirmed = 4,
  Error = 5,
-  Dropped = 6
+  Dropped = 6,
+  Signed = 7 // dapps will submit the transaction


Does this need to be flagged to iOS or Android for handling in any switches?For Android is there anything in this PR that is needed?

For Desktop, do we need anything in any of these which have other switch cases?

components/brave_wallet_ui/components/shared/style.tsx

components/brave_wallet_ui/components/extension/confirm-transaction-panel/confirm-solana-transaction-panel.tsx

components/brave_wallet_ui/components/desktop/portfolio-transaction-item/index.tsx (You have one covered in this file but there's another place)

I just did git grep \\.Dropped to get this list.

Yes, iOS and Android need to handle the new tx status on UI. cc @SergeyZhukovsky @StephenHeaps.
git grep TransactionStatus\\. is more precise so you can see what is being used on Android.
I will check desktop UI for missing parts.

I don't see other changes needed on desktop UI except components/brave_wallet_ui/components/shared/style.tsx to update its opacity

I will open two issues for iOS and Android as follow-up

brave/brave-browser#27224 and brave/brave-ios#6606

brave-builds · 2022-12-07T21:15:50Z

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

brave-builds · 2022-12-07T23:07:59Z

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

darkdh requested a review from bbondy December 6, 2022 02:14

darkdh requested a review from a team as a code owner December 6, 2022 02:14

darkdh self-assigned this Dec 6, 2022

darkdh requested review from a team as code owners December 6, 2022 02:14

github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/storybook-url Deploy storybook and provide a unique URL for each build labels Dec 6, 2022

wchen342 reviewed Dec 6, 2022

View reviewed changes

android/android_browser_tests.gni Show resolved Hide resolved

darkdh force-pushed the eth_sign_transaction branch 2 times, most recently from f151950 to 628422f Compare December 6, 2022 18:45

darkdh force-pushed the eth_sign_transaction branch from 628422f to b134c0f Compare December 6, 2022 19:43

darkdh force-pushed the eth_sign_transaction branch from 8dac776 to 1602724 Compare December 6, 2022 21:16

wchen342 approved these changes Dec 7, 2022

View reviewed changes

bbondy requested changes Dec 7, 2022

View reviewed changes

darkdh force-pushed the eth_sign_transaction branch from 8980246 to bc1a0fe Compare December 7, 2022 20:24

darkdh removed the request for review from a team December 7, 2022 20:25

darkdh force-pushed the eth_sign_transaction branch from bc1a0fe to 8be035a Compare December 7, 2022 20:29

bbondy approved these changes Dec 7, 2022

View reviewed changes

darkdh force-pushed the eth_sign_transaction branch from 8be035a to bb75b2c Compare December 7, 2022 20:33

darkdh added 6 commits December 7, 2022 14:25

Use idl for eth transaction parsing

beb3fad

Add eth_sendRawTransaction

5a32b3f

Add eth_signTransaction

f889dce

Update tests

918d11b

presubmit fix

3454d93

Fix optional signedTransaction on Android

95f1188

review feedback

6e3b544

darkdh force-pushed the eth_sign_transaction branch from bb75b2c to 6e3b544 Compare December 7, 2022 22:25

darkdh added this to the 1.48.x - Nightly milestone Dec 7, 2022

This was referenced Dec 7, 2022

Apply eth_signTransaction UI changes (1.48.x brave-core) brave/brave-ios#6606

Closed

Apply eth_signTransaction UI changes (Android) brave/brave-browser#27224

Open

darkdh merged commit 85227ab into master Dec 8, 2022

darkdh deleted the eth_sign_transaction branch December 8, 2022 00:06

srirambv mentioned this pull request Feb 3, 2023

Support eth_signTransaction and eth_sendRawTransaction brave/brave-browser#23582

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add eth_signTransaction and eth_sendRawTransaction #16247

Add eth_signTransaction and eth_sendRawTransaction #16247

darkdh commented Dec 6, 2022 •

edited

Loading

brave-builds commented Dec 6, 2022

brave-builds commented Dec 6, 2022

brave-builds commented Dec 6, 2022

wchen342 left a comment

bbondy left a comment

bbondy Dec 7, 2022

bbondy Dec 7, 2022

bbondy Dec 7, 2022

darkdh Dec 7, 2022

darkdh Dec 7, 2022 •

edited

Loading

darkdh Dec 7, 2022

darkdh Dec 7, 2022

brave-builds commented Dec 7, 2022

brave-builds commented Dec 7, 2022

Add eth_signTransaction and eth_sendRawTransaction #16247

Add eth_signTransaction and eth_sendRawTransaction #16247

Conversation

darkdh commented Dec 6, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Normal flow

Nonce drop

brave-builds commented Dec 6, 2022

brave-builds commented Dec 6, 2022

brave-builds commented Dec 6, 2022

wchen342 left a comment

Choose a reason for hiding this comment

bbondy left a comment

Choose a reason for hiding this comment

bbondy Dec 7, 2022

Choose a reason for hiding this comment

bbondy Dec 7, 2022

Choose a reason for hiding this comment

bbondy Dec 7, 2022

Choose a reason for hiding this comment

darkdh Dec 7, 2022

Choose a reason for hiding this comment

darkdh Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

darkdh Dec 7, 2022

Choose a reason for hiding this comment

darkdh Dec 7, 2022

Choose a reason for hiding this comment

brave-builds commented Dec 7, 2022

brave-builds commented Dec 7, 2022

darkdh commented Dec 6, 2022 •

edited

Loading

darkdh Dec 7, 2022 •

edited

Loading