feat(wallet): support CoW 🐮 orders in Safe Sign #20203

onyb · 2023-09-18T15:12:10Z

Summary of changes:

Add support for parsing EIP-712 messages for CoW orders, and render the parsed fields in the Safe Sign UI previously designed for 0x swaps.
Add support for unknown tokens in Safe Sign UI.

Resolves brave/brave-browser#33093

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

👉 Case A: CoW Swap trade

Before	After

👉 Case B: CoW Swap trade with custom recipient

Before	After

It's also possible to view the raw message by clicking on the Details button.

👉 Case C: CoW swap trade selling native asset (ETH on Ethereum, XDAI on Gnosis)

⚠️ These trades cost gas, since they are real transactions. Also, we do not have the ability to display custom recipients in this case, for the time being.

Before	After

👉 Case D: 0x Swap involving unknown tokens

Before	After

components/brave_wallet/common/brave_wallet.mojom

components/brave_wallet/common/eth_request_helper.cc

supermassive

wallet core lgtm

mkarolin

strings++

brave-builds · 2023-09-20T09:56:05Z

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

kdenhartog · 2023-09-20T23:49:35Z

Just working on some dynamic testing on swap.cow.fi. I'm not seeing either of the before or after screens being presented when running this dev branch. It's still showing all the old UI for me. Is there a flag that needs to be enabled for this to work?

kdenhartog · 2023-09-20T23:52:01Z

0x swaps are still working good for me though. Ignore the network fee not showing, I'm fairly certain that's because I don't have an API key I need enabled.

josheleonard · 2023-09-20T23:52:10Z

components/brave_wallet_ui/common/slices/api.slice.ts

+            if (!decimals) {
+              throw new Error(errorMessage)
+            }


Should we check for the error message presence instead?

josheleonard · 2023-09-20T23:52:41Z

components/brave_wallet_ui/common/slices/api.slice.ts

+            if (!symbol) {
+              throw new Error(errorMessage)
+            }


shouldnt we just check if there is an error message?

josheleonard · 2023-09-20T23:53:18Z

components/brave_wallet_ui/components/extension/sign-panel/cow_swap_order.tsx

+// Hooks
+import { useAccountOrb, useAddressOrb } from '../../../common/hooks/use-orb'
+
+const makeUnknownToken = (


how about?

Suggested change

const makeUnknownToken = (

const UNKOWN_TOKEN = (

I think makeUnknownToken makes more sense since it is a function. I'd use UNKNOWN_TOKEN for a constant.

josheleonard · 2023-09-20T23:55:05Z

components/brave_wallet_ui/components/extension/swap/index.tsx

+  const buyTokenResult = makeToken(
+    buyToken,
+    buyAssetNetwork,
+    buyTokenSymbol,
+    buyTokenDecimals
+  )
+  const sellTokenResult = makeToken(
+    sellToken,
+    sellAssetNetwork,
+    sellTokenSymbol,
+    sellTokenDecimals
+  )


memoize these objet to prevent re-renders of child components

onyb · 2023-09-21T08:45:08Z

@kdenhartog You need to wrap ETH into WETH in order to benefit from gasless swaps using EIP-712 messages. If you select ETH as your sell asset, you'll need to pay for gas fees - which is actually wrapping it into WETH behind the scenes.

@StephenHeaps also had the same feedback by the way. We should handle this case the same way we parse 0x calldata into the ETHSwap transaction type. I'm currently working with the CoW team to implement a calldata parser for this particular case.

anxolin

Amazing! this looks slick.

I just checked the PR. I wouldn't mind trying it out, do you think is easy for me to run this locally?

components/brave_wallet/common/eth_request_helper.cc

anxolin · 2023-09-21T08:58:46Z

components/brave_wallet/common/brave_wallet.mojom

+  string buy_token;
+  string buy_amount;
+  string receiver;
+  string deadline;


I would suggest adding the feeAmount and kind (that can be either sell or buy)
And maybe partiallyFillable

see:
https://github.com/cowprotocol/contracts/blob/5c25837f585d79dc5450fb80f66c41f596c3198d/src/ts/order.ts#L151

Or this is just for the view that shows the basic params, and the rest are visible in the details?

Or this is just for the view that shows the basic params, and the rest are visible in the details?

That's right. I think we'll need to extend the current design to accommodate for those extra fields. Would be a good next step for us to make the signing screen even more informative.

anxolin · 2023-09-21T09:03:09Z

components/brave_wallet_ui/common/slices/api.slice.ts

+                }
+              ]
+      }),
+      getEthTokenSymbol: query<


I see you get decimals, symbol. You might want to consider fetching the name as well

Oh I didn't know name was also part of the ERC20 interface. Good to know! We don't need the token name for this particular case (it doesn't get displayed), but I'll add it another PR.

yep, it is, although is optional (although most contracts have them).

Not sure if your provider accounts for it, but there's some contracts that implement these methods but return bytes instead of the string. One that comes to mind is SAI, so I would try this out. They have different ABI for the same method, technically they don't respect the standard, but most dapps handle this case.

Thanks for the info. Created an issue to track this: brave/brave-browser#33162

anxolin · 2023-09-21T09:03:36Z

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

+
+        // set to true once Swap+Send is supported
+        expectRecipientAddress={false}
+      />


nice refactor! ❤️

anxolin · 2023-09-21T09:09:39Z

components/brave_wallet_ui/components/extension/sign-panel/cow_swap_order.tsx

+          senderLabel={senderLabel}
+          senderOrb={senderOrb}
+          recipientOrb={recipientOrb}
+          recipientLabel={recipientLabel}


Probably out of scope for this PR, but, one thing that would help with the security is to show a warning if the receiver account and the signing account is not the same (or is not 0x0, since that in the smart contract refers to the owner of the funds).

Changing the receiver is a great CoW Swap feature, it allows you to SWAP and send in one operation. However, i think is nice to make the user aware the destination of the funds is different than the origin of them

Indeed that's a good point. The addresses should also link to the explorer. Will take inputs from the design team for a future iteration.

++ seems like a good follow up item

Adds support for parsing EIP-712 messages for CoW orders, and renders the information in the Safe Sign UI previously designed for 0x swaps.

brave-builds · 2023-09-21T10:57:21Z

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

brave-builds · 2023-09-21T11:12:54Z

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

StephenHeaps

iOS++.

iOS will get ETHSwap UI for free 🔥:

And I've opened brave/brave-ios#8114 for integrating the Safe Sign UI with this signature request on iOS.

brave-builds · 2023-09-21T15:13:26Z

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

josheleonard

desktop frontend ++

kdenhartog

Just one blocking question to confirm we aren't introducing a casting issue - rest seems like good follow up issues.

components/brave_wallet/common/eth_request_helper.cc

kdenhartog · 2023-09-25T08:44:37Z

components/brave_wallet_ui/components/extension/sign-panel/cow_swap_order.tsx

+          senderLabel={senderLabel}
+          senderOrb={senderOrb}
+          recipientOrb={recipientOrb}
+          recipientLabel={recipientLabel}


++ seems like a good follow up item

kdenhartog · 2023-09-25T08:56:54Z

components/brave_wallet/browser/eth_data_parser.cc

@@ -341,6 +342,55 @@ GetTransactionInfoFromData(const std::vector<uint8_t>& data) {
                                                    "uint128",   // sell amount
                                                    "uint128"},  // buy amount
                           tx_args);
+  } else if (selector == kCowOrderSellEthSelector) {


This function could use a bit of refactoring. It's starting to get pretty complex for a parsing function that's handling untrusted data supplied by a page. Given that an error in this logic can easily lead to a rule of 2 violation we should split this up better.

Let's do a follow up issue for this.

kdenhartog

LGTM for the one sec concern I had

onyb requested a review from a team as a code owner September 18, 2023 15:12

onyb self-assigned this Sep 18, 2023

onyb requested review from a team as code owners September 18, 2023 15:12

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 Sep 18, 2023

onyb requested a review from kdenhartog September 18, 2023 15:45

supermassive reviewed Sep 19, 2023

View reviewed changes

components/brave_wallet/common/brave_wallet.mojom Show resolved Hide resolved

supermassive reviewed Sep 19, 2023

View reviewed changes

components/brave_wallet/common/eth_request_helper.cc Show resolved Hide resolved

supermassive approved these changes Sep 19, 2023

View reviewed changes

mkarolin approved these changes Sep 19, 2023

View reviewed changes

onyb force-pushed the f/wallet/cowswap branch 2 times, most recently from 2380d1c to 4fb5ec0 Compare September 20, 2023 09:37

josheleonard reviewed Sep 20, 2023

View reviewed changes

josheleonard suggested changes Sep 20, 2023

View reviewed changes

anxolin reviewed Sep 21, 2023

View reviewed changes

onyb added 4 commits September 21, 2023 12:42

feat(wallet): support CoW 🐮 orders in Safe Sign

f09d4f0

Adds support for parsing EIP-712 messages for CoW orders, and renders the information in the Safe Sign UI previously designed for 0x swaps.

review(supermassive): add comments

c75d3ff

fix storybook build

88e8efe

review(kdenhartog): add ETHSwap parser for XDAI/ETH sell swaps

4aa6766

onyb force-pushed the f/wallet/cowswap branch from 4fb5ec0 to 4aa6766 Compare September 21, 2023 10:42

review(josheleonard): frontend review fixes

e075c8e

StephenHeaps approved these changes Sep 21, 2023

View reviewed changes

fix buy native token case

f1b4ae4

onyb mentioned this pull request Sep 21, 2023

Handle optional or bytes decimals and symbol fields in IERC20 brave/brave-browser#33162

Open

josheleonard approved these changes Sep 22, 2023

View reviewed changes

kdenhartog reviewed Sep 25, 2023

View reviewed changes

kdenhartog approved these changes Sep 25, 2023

View reviewed changes

onyb merged commit 617a068 into master Sep 25, 2023
15 checks passed

onyb deleted the f/wallet/cowswap branch September 25, 2023 11:17

github-actions bot added this to the 1.60.x - Nightly milestone Sep 25, 2023

srirambv mentioned this pull request Oct 26, 2023

Safe Sign support for CoW 🐮 orders brave/brave-browser#33093

Closed

feat(wallet): support CoW 🐮 orders in Safe Sign #20203

feat(wallet): support CoW 🐮 orders in Safe Sign #20203

Conversation

onyb commented Sep 18, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

👉 Case A: CoW Swap trade

👉 Case B: CoW Swap trade with custom recipient

👉 Case C: CoW swap trade selling native asset (ETH on Ethereum, XDAI on Gnosis)

👉 Case D: 0x Swap involving unknown tokens

supermassive left a comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

brave-builds commented Sep 20, 2023

kdenhartog commented Sep 20, 2023

kdenhartog commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onyb commented Sep 21, 2023 • edited Loading

anxolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Sep 21, 2023

brave-builds commented Sep 21, 2023

StephenHeaps left a comment • edited Loading

Choose a reason for hiding this comment

brave-builds commented Sep 21, 2023

josheleonard left a comment

Choose a reason for hiding this comment

kdenhartog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdenhartog left a comment

Choose a reason for hiding this comment

onyb commented Sep 18, 2023 •

edited

Loading

onyb commented Sep 21, 2023 •

edited

Loading

StephenHeaps left a comment •

edited

Loading