Fix WalletConnect `eth_signTransaction` requests handling #1729

jeanregisser · 2022-01-06T15:54:09Z

Description

WalletConnect v1 utils strips some important tx fields (on the dapp side) which prevents eth_signTransaction calls from working.
See all details in #1559.

Ideally this could/should be fixed in WalletConnect v1 utils too, but it's not clear whether such a fix would be accepted and it may take quite some time before it's released and deployed. We're discussing this in this Slack thread.

Working around it in Valora fixes the issue directly and also ensures we're more robust with missing or incorrect tx params.

Note: I'll open another PR to re-use the same logic for eth_sendTransaction. That one already works but the new logic added in eth_signTransaction is a bit better and should be shared.

Tested

Added new unit tests
Manually, swapping on https://mento.finance/ works once connected via WalletConnect with the following situations:
- when I only have CELO in my balance
- when I have only cUSD in my balance

How others should test

See above.

Related issues

Backwards compatibility

Yes

codecov · 2022-01-06T15:59:35Z

Codecov Report

Merging #1729 (da74de0) into main (32f6322) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1729      +/-   ##
==========================================
+ Coverage   76.51%   76.53%   +0.02%     
==========================================
  Files         520      520              
  Lines       19291    19308      +17     
  Branches     3520     3521       +1     
==========================================
+ Hits        14760    14777      +17     
  Misses       4459     4459              
  Partials       72       72

Impacted Files	Coverage Δ
packages/mobile/src/transactions/send.ts	`53.48% <100.00%> (ø)`
packages/mobile/src/walletConnect/request.ts	`70.83% <100.00%> (+15.99%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32f6322...da74de0. Read the comment docs.

jeanregisser · 2022-01-06T16:13:11Z

packages/mobile/src/walletConnect/request.ts

+        rawTx.gas = undefined
+        rawTx.gasPrice = undefined
+      }
+      const tx: CeloTx = yield call(normalizer.populate.bind(normalizer), rawTx)


Though I tested this successfully in real conditions, do we need some gasPrice inflation somewhere? or does contractkit do the right thing?
The wallet send flow does inflate it using:

wallet/packages/mobile/src/web3/gas.ts

Lines 33 to 47 in 1455340

async function fetchGasPrice(currency: Currency): Promise<BigNumber> {

const contractKit = await getContractKitAsync()

const [gasPriceMinimum, address] = await Promise.all([

contractKit.contracts.getGasPriceMinimum(),

getCurrencyAddress(currency),

])

const latestGasPrice = await gasPriceMinimum.getGasPriceMinimum(address)

const inflatedGasPrice = latestGasPrice.times(GAS_PRICE_INFLATION_FACTOR)

Logger.debug(

TAG,

'fetchGasPrice',

`Result price in ${currency} with inflation: ${inflatedGasPrice.toString()}`

)

return inflatedGasPrice

}

But I'm not entirely familiar with this matter.

cc @jmrossy @nategraf

It looks like this eventually makes an eth_gasPrice RPC call to the supporting Geth node. This will suggest an appropriate gas price, which by default is 5x the GPM. So this seems like it should result in a setting that will work.

https://github.com/celo-org/celo-blockchain/blob/8cd59acbb3ff70033c45867d2d62dbda0032c36a/contracts/gasprice_minimum/gasprice_minimum.go#L55-L60

Great! Thanks for checking 💪

nategraf · 2022-01-06T17:25:48Z

packages/mobile/src/walletConnect/request.ts

+
+        rawTx.feeCurrency = feeCurrencyAddress
+        // Note: we're resetting gas and gasPrice here because if the feeCurrency has changed, we need to recalculate them
+        rawTx.gas = undefined


One caveat here is that there are cases where gas estimation will fail even if the transaction will end up clearing. Clearing the gas may result in errors for the dApp. E.g. If a dApp developer is doing a two step approve and exchange and requesting both signatures together, they will set the gas on the second transaction because if estimateGas is run before the approve completes, execution will fail.

I saw in the Slack thread there is some mention of estimateGas being broken for non-CELO fee currencies, but I am not aware of any issue right now. It is true though that using a non-CELO fee currency uses more gas, by about 50k, so if the caller estimates using CELO and this code switches it to cUSD, the transaction may end up with too little gas.

Of these two scenarios, failures due to clearing the gas value seem more likely to me than failures due to not clearing it.

Interesting problem, nice catch!

I think not having CELO to pay for gas is a somewhat common situation, especially for Valora users where they are encouraged to cash in cUSD/cEUR and not CELO. dApps are also usually not very good at catching it and show unhelpful errors when this happens.

My point is that the switch from CELO fee to cUSD/cEUR fee might happen somewhat often here. Don't really have the data to back it up, just my intuition (we could look it up).

How about just plain adding 50k to the gas field when this happens? Would that help solve for both use-cases or are there other complications I might be missing?

E.g. If a dApp developer is doing a two step approve and exchange and requesting both signatures together, they will set the gas on the second transaction because if estimateGas is run before the approve completes, execution will fail.

Do you mean that the dapp may call eth_signTransaction without having called estimateGas on their end and sent individually?
So if it does that for approve and exchange, it will indeed fail for us.
Well, great catch!! 🥇

So yeah ideally we should really not touch what the dapp is providing. But since we have this bug with WalletConnect v1 utils, we have to do something.
I really like what @gnardini is proposing then.

In addition, I'll also limit the workaround to when type is not set. Because in that case the TX data is sent as is.
See https://github.com/WalletConnect/walletconnect-monorepo/blob/c6b26481c34848dbc9c49bb0d024bda907ec4599/packages/helpers/utils/src/ethereum.ts#L45
Decided against this to avoid surprises for dapp developers. IMHO the feeCurrency adjustment is good to have even if WalletConnect was not stripping any fields. Instead I'm proposing an explicit custom field __skip_normalization.

Adding 50k to the gas value when setting a non-CELO fee currency seems reasonable. That should be the correct value as it is the value currently set in the intrinsicGasForAlternativeFeeCurrency on-chain 👌

MuckT · 2022-01-06T20:13:43Z

@jeanregisser I locally brought these changes into the #1412 and the e2e test around sign a transaction is passing.

…rrency is changed

…eanregisser/fix-wc-sign

jeanregisser · 2022-01-07T16:00:44Z

packages/mobile/src/walletConnect/request.ts

+      // Provide an escape hatch for dapp developers who don't want any normalization
+      if (rawTx.__skip_normalization) {
+        // Remove this custom field which may cause issues down the line
+        delete rawTx.__skip_normalization
+        tx = rawTx
+      } else {


I haven't tested this escape hatch end-to-end yet.
Could this cause issues between the dapp and Valora processing it?

Of course because of the WC v1 utils stripping fields, someone wanting to use it would have to set both type and __skip_normalization.

I'll document this if we decide this escape hatch is viable.

gnardini

Everything looks good to be, great job on this!

jmrossy

LGTM with two optional comments

jmrossy · 2022-01-08T05:02:09Z

packages/mobile/src/walletConnect/request.ts

+      const rawTx = { ...params[0] }
+      let tx
+      // Provide an escape hatch for dapp developers who don't want any normalization
+      if (rawTx.__skip_normalization) {


Will WC strip this field out though?

Yes it will for v1 requests, so developers wanting to use it would have to set both type = null and __skip_normalization.

jmrossy · 2022-01-08T05:03:09Z

packages/mobile/src/walletConnect/request.ts

@@ -29,14 +36,56 @@ export function* handleRequest({ method, params }: { method: string; params: any
  yield call(unlockAccount, account)

  switch (method) {
-    case SupportedActions.eth_signTransaction:
-      return (yield call(wallet.signTransaction.bind(wallet), params[0])) as EncodedTransaction
+    case SupportedActions.eth_signTransaction: {


Should we normalize eth_send and eth_sign the same way?

Yes we should, I'll open a PR applying the same logic to both.

ValoraQA · 2022-01-13T09:45:53Z

Hey @MuckT we have verified the issue on Latest Android Internal build V:1.24.1 and Latest iOS TestFlight build V:1.24.1 and updated the status in Gsheet

Sheet link : https://docs.google.com/spreadsheets/d/15nGo3iJAEVJ76wHae9PvYuWI7S2bovKAQPX_edtwPEI/edit#gid=2037765399

Observation : There is no option to connect wallet to given link and also we would require cREAL for the transaction in our account balance

We have raised query for the same : Query link

Devices : Google Pixel 2XL(11.0) , Google Pixel (10.0) , iPhone 12(14.7), iPhone 13 Mini (15.1.1)
CC: @jeanregisser
Thanks..!!

ValoraQA · 2022-01-17T12:34:15Z

Hey @MuckT we have verified the issue on Latest iOS Test Flight build V:1.24.1 and updated the status in Gsheet

Sheet link: https://docs.google.com/spreadsheets/d/15nGo3iJAEVJ76wHae9PvYuWI7S2bovKAQPX_edtwPEI/edit#gid=2037765399

Observation : There is no option to connect wallet to given link and also we would require cREAL for the transaction in our account balance

We have raised query for the same : Query link

Devices: iPhone XS Max (13.5.1)
CC: @jeanregisser
Thanks..!!

ValoraQA · 2022-01-20T05:05:17Z

Hey @MuckT we have verified the issue on Latest Android Internal release build V:1.24.1 and updated the status in Gsheet

Sheet link : https://docs.google.com/spreadsheets/d/15nGo3iJAEVJ76wHae9PvYuWI7S2bovKAQPX_edtwPEI/edit#gid=2037765399

Observation : There is no option to connect wallet to given link and also we would require cREAL for the transaction in our account balance

We have raised query for the same : Query link

Devices : Samsung Galaxy A8+(8.0)
CC: @jeanregisser
Thanks..!!

ValoraQA · 2022-01-21T11:24:10Z

Hey @MuckT we have verified the issue on Latest Android Internal Release build V:1.24.1 and updated the status in Gsheet

Sheet link: https://docs.google.com/spreadsheets/d/15nGo3iJAEVJ76wHae9PvYuWI7S2bovKAQPX_edtwPEI/edit#gid=2037765399

Observation: There is no option to connect wallet to given link and also we would require cREAL for the transaction in our account balance

We have raised query for the same : Query link

Devices: Redmi Note 5 Pro (9.0)
CC: @jeanregisser
Thanks..!!

jmrossy · 2022-01-23T04:32:58Z

@ValoraQA You can test mento-fi with the pre-release version here:
https://mento-fi-q5d61619k-celotools.vercel.app/

ValoraQA · 2022-01-24T12:08:17Z

Hey @MuckT we have verified the issue on Latest Android Internal Release build and Latest iOS Test Flight Internal Build V:1.24.1 and observed that we are able to swap tokens on "https://mento-fi-q5d61619k-celotools.vercel.app/"

Observation: After clicking on "Swap" button it asks the transaction permission twice on the valora app

Attachment : mento.mp4

Devices: OnePlus 7T (11.0) , iPhone 13 mini (15.1.1)

CC: @jmrossy

Thanks..!!

jeanregisser added 5 commits January 6, 2022 12:34

Normalize WC signTransaction tx param

8ddecf0

Added test for signTransaction tx normalization

4eaef13

Improve test

70efbbb

More test improvements

cc5255d

Removed logs

79b1709

jeanregisser changed the title ~~Fix WalletConnect eth_signTransaction~~ Fix WalletConnect eth_signTransaction requests handling Jan 6, 2022

jeanregisser commented Jan 6, 2022

View reviewed changes

nategraf reviewed Jan 6, 2022

View reviewed changes

Merge branch 'main' into jeanregisser/fix-wc-sign

c4bcae7

MuckT mentioned this pull request Jan 6, 2022

WalletConnect V1 E2E Tests #1412

Merged

jeanregisser added 3 commits January 7, 2022 16:21

Increase gas instead of estimating again to avoid failures when feeCu…

61731b5

…rrency is changed

Provide an escape hatch to skip normalization

ed2076b

Merge remote-tracking branch 'origin/jeanregisser/fix-wc-sign' into j…

6a6273f

…eanregisser/fix-wc-sign

jeanregisser commented Jan 7, 2022

View reviewed changes

gnardini approved these changes Jan 7, 2022

View reviewed changes

jeanregisser added 2 commits January 7, 2022 18:15

Only pad gas if the new feeCurrency is non-CELO

d9f2e13

Tweak comment

c6b1c32

jmrossy approved these changes Jan 8, 2022

View reviewed changes

Merge branch 'main' into jeanregisser/fix-wc-sign

da74de0

MuckT added the automerge Have PR merge automatically when checks pass label Jan 8, 2022

mergify bot merged commit 05009d9 into main Jan 8, 2022

mergify bot deleted the jeanregisser/fix-wc-sign branch January 8, 2022 18:35

ValoraQA mentioned this pull request Jan 13, 2022

'Sign a transaction' failing with WalletConnect - Optional Gas Required #1559

Closed

MuckT linked an issue Jan 18, 2022 that may be closed by this pull request

Wallet Connect V1 Actions Failing in Valora #1556

Closed

ValoraQA mentioned this pull request Jan 24, 2022

[Query] How to connect on"mento.finance" site if user visits the link via Desktop / Mweb as there is no option of "Wallet connect" #1760

Closed

MuckT mentioned this pull request Jan 25, 2022

Wallet Connect V1 Actions Failing in Valora #1556

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix WalletConnect `eth_signTransaction` requests handling #1729

Fix WalletConnect `eth_signTransaction` requests handling #1729

jeanregisser commented Jan 6, 2022 •

edited

Loading

codecov bot commented Jan 6, 2022 •

edited

Loading

jeanregisser Jan 6, 2022 •

edited

Loading

nategraf Jan 6, 2022

jeanregisser Jan 7, 2022

nategraf Jan 6, 2022

gnardini Jan 7, 2022

jeanregisser Jan 7, 2022 •

edited

Loading

nategraf Jan 7, 2022

MuckT commented Jan 6, 2022

jeanregisser Jan 7, 2022 •

edited

Loading

gnardini left a comment

jmrossy left a comment

jmrossy Jan 8, 2022

jeanregisser Jan 10, 2022 •

edited

Loading

jmrossy Jan 8, 2022

jeanregisser Jan 10, 2022

ValoraQA commented Jan 13, 2022 •

edited

Loading

ValoraQA commented Jan 17, 2022 •

edited

Loading

ValoraQA commented Jan 20, 2022

ValoraQA commented Jan 21, 2022

jmrossy commented Jan 23, 2022

ValoraQA commented Jan 24, 2022

	async function fetchGasPrice(currency: Currency): Promise<BigNumber> {
	const contractKit = await getContractKitAsync()
	const [gasPriceMinimum, address] = await Promise.all([
	contractKit.contracts.getGasPriceMinimum(),
	getCurrencyAddress(currency),
	])
	const latestGasPrice = await gasPriceMinimum.getGasPriceMinimum(address)
	const inflatedGasPrice = latestGasPrice.times(GAS_PRICE_INFLATION_FACTOR)
	Logger.debug(
	TAG,
	'fetchGasPrice',
	`Result price in ${currency} with inflation: ${inflatedGasPrice.toString()}`
	)
	return inflatedGasPrice
	}

Fix WalletConnect eth_signTransaction requests handling #1729

Fix WalletConnect eth_signTransaction requests handling #1729

Conversation

jeanregisser commented Jan 6, 2022 • edited Loading

Description

Tested

How others should test

Related issues

Backwards compatibility

codecov bot commented Jan 6, 2022 • edited Loading

Codecov Report

jeanregisser Jan 6, 2022 • edited Loading

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

jeanregisser Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuckT commented Jan 6, 2022

jeanregisser Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

gnardini left a comment

Choose a reason for hiding this comment

jmrossy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValoraQA commented Jan 13, 2022 • edited Loading

ValoraQA commented Jan 17, 2022 • edited Loading

ValoraQA commented Jan 20, 2022

ValoraQA commented Jan 21, 2022

jmrossy commented Jan 23, 2022

ValoraQA commented Jan 24, 2022

Fix WalletConnect `eth_signTransaction` requests handling #1729

Fix WalletConnect `eth_signTransaction` requests handling #1729

jeanregisser commented Jan 6, 2022 •

edited

Loading

codecov bot commented Jan 6, 2022 •

edited

Loading

jeanregisser Jan 6, 2022 •

edited

Loading

jeanregisser Jan 7, 2022 •

edited

Loading

jeanregisser Jan 7, 2022 •

edited

Loading

jeanregisser Jan 10, 2022 •

edited

Loading

ValoraQA commented Jan 13, 2022 •

edited

Loading

ValoraQA commented Jan 17, 2022 •

edited

Loading