Feature: Single tx safe apps interactions should not use multisend #1792

mmv08 · 2021-01-19T07:12:46Z

This PR:

Closes Single tx safe apps interactions should not use multisend #1791 by not invoking encodeMultiSendCall if txs.length is 1

github-actions · 2021-01-19T07:12:59Z

CLA Assistant Lite All Contributors have signed the CLA.

lukasschor · 2021-01-19T07:17:35Z

I wonder, is there anywhere, where we assume all Safe App txs to be Multisend?

For example, I thought in our Dune Dashboards we approximate Safe App interactions by querying Multisend txs to a given smart contract (@tschubotz knows for sure).

Maybe there are other places as well (tx list, backend, tx review modal, etc.) where this assumption is made?

github-actions · 2021-01-19T07:18:07Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

ghost · 2021-01-19T07:26:40Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2021-01-19T07:39:58Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/volta/app

tschubotz · 2021-01-19T08:31:27Z

For example, I thought in our Dune Dashboards we approximate Safe App interactions by querying Multisend txs to a given smart contract (@tschubotz knows for sure).

Yes, but these dashboards aren't super accurate anyway. + I think a single tx instead of a multisend is better UX and cheaper. I think we should find other ways of tracking then.

Generally I'm in favor of this feature, I can't tell if this interferes with how the frontend/backend marks Safe app txs though.

mmv08 · 2021-01-19T08:49:14Z

@tschubotz @lukasschor I tested it and didn't find any quirks, everything works smoothly.

Disclaimer: currently there's a bug in gas calculations for safe apps transactions and they're failing, but everything app/tx decoding related shows as expected

ghost · 2021-01-19T09:09:59Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2021-01-19T09:24:34Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/volta/app

ghost · 2021-01-20T12:39:16Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2021-01-20T12:54:01Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/volta/app

dasanra · 2021-01-26T12:08:12Z

@francovenica you should check that you can execute transactions in safe apps normally

ghost · 2021-01-26T12:21:50Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2021-01-26T12:35:45Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/volta/app

francovenica · 2021-01-26T20:06:54Z

I have a problem with a particular tx I want to create in the Tx builder. It fails only in this PR, since I gave it a try in rinkeby prod and the tx works fine there.

My tx wants to send rinkeby DAI tokens to an account.

DAI address: 0x5592EC0cfb4dbc12D3aB100b257153436a1f0FEa
Contract ABI: https://ethereumdev.io/abi-for-erc20-contract-on-ethereum/
Method: transfer
To: any account you know
Value: 1400000000000000 (A small value, is not important)

When I queue the 2 tx in the Tx builder and send it then it works just fine. But if I just queue 1 tx and send it then it always fail.
Again, this only happens in this PR, I tried the same in rinkeby prod (https://rinkeby.gnosis-safe.io/app/#/) and it works fine there

Safe and env: https://rinkeby.gnosis-safe.io/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions
Look for the tx on Jan 26, 2021 - 16:36:07 and the 3 above that one
The tx hashes for etherscan:
0x744da69f7381e24e1186ffc5938dd45473c68fda96b220e1cea91fe1e54d01a6 (2 transfer in 1 tx, succesful tx)
0x1420c7fb6dde70e9b7f977e9586b257815078019cc72fde7e2e55ecf0927c04c (1 transfer in 1 tx, failed tx)
0xe533260d38524e5caca5fbaad8db3a6c42ef81c625b89f0143745600344db115 (1 transfer in 1 tx, failed tx)
0xe4d576966ba7a1ba0896c8866e735f0f2dea267a5a846d6b115d1bbb7b496a6a ((1 transfer in 1 tx done in rinkeby prod, successful)

Snapshots:
In this one I did 2 transfers queued in the Tx builder and sent it. The tx was successful

In this one I did 1 transfer in the tx builder and failed. The tx above that one is the same case

NOTE: The "Contract interaction" Tx is also a Tx builder type of tx, using the transfer method to 1 account. The reason is seen as "Contract interaction" and not as "tx builder" is because that one was done in the rinkeby prod env, and Richard told me that if you see those type of tx in another environment the label gets confused.

If none of this has to do with this ticket then let me know.

I tried other tx and the main idea of showing no "action" label when only 1 tx is being sent is working fine.

francovenica · 2021-01-26T20:19:25Z

Another example with the IDLE app. A withdrawal of tokens, the failed one below (on Jan 26, 2021 - 16:27:55) was done in this PR and the successful one was done in rinkeby prod (highlighted in the snapshot)
Same safe as before: https://pr1792--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

Tx hashes for etherscan:
0x26cd5d8260da07a6f994dedb623a43947f7e3c3c4e33cfca7961851f0a9b4297 (failed one)
0xc434bbbb55421768138d612cc1fc6db082ea2399470716df458f2bedce6521a8 (successful one)

rmeissner · 2021-01-26T21:59:06Z

src/routes/safe/components/Apps/components/ConfirmTransactionModal.tsx

@@ -110,7 +111,7 @@ export const ConfirmTransactionModal = ({
    gasCostFormatted,
    txEstimationExecutionStatus,
  } = useEstimateTransactionGas({
-    txData: encodeMultiSendCall(txs),
+    txData: txData || '',
    txRecipient: MULTI_SEND_ADDRESS,


The receipient needs to change. currently the tx is always sent to the multisend address, which is wrong

🤦‍♂️ 🤦‍♂️ 🤦‍♂️ 🤦‍♂️ 🤦‍♂️

rmeissner · 2021-01-26T22:00:10Z

src/routes/safe/components/Apps/components/ConfirmTransactionModal.tsx

@@ -137,8 +138,6 @@ export const ConfirmTransactionModal = ({
  }

  const confirmTransactions = async () => {
-    const txData = encodeMultiSendCall(txs)
-
    await dispatch(
      createTransaction(
        {


For to and operation here we need to make the same changes as mentined above

rmeissner · 2021-01-26T22:02:29Z

src/routes/safe/components/Apps/components/ConfirmTransactionModal.tsx

@@ -110,7 +111,7 @@ export const ConfirmTransactionModal = ({
    gasCostFormatted,
    txEstimationExecutionStatus,
  } = useEstimateTransactionGas({
-    txData: encodeMultiSendCall(txs),
+    txData: txData || '',
    txRecipient: MULTI_SEND_ADDRESS,
    operation: DELEGATE_CALL,


The opration should be a CALL for single transactions

…re/no-multisend-for-1-tx

ghost · 2021-01-27T07:20:48Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2021-01-27T07:36:14Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/volta/app

ghost · 2021-01-27T07:57:48Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2021-01-27T08:11:37Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/volta/app

francovenica · 2021-01-27T12:02:13Z

The issues I've reported were fixed
Single tx made with the Tx builder and the tx made with Idle now work fine

Safe: https://pr1792--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

rmeissner

Only minor thing might be that we should do nothing for an empty array of transactions, but that is maybe a different issue (as it existed before)

ghost · 2021-01-27T16:13:39Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/rinkeby/app

ghost · 2021-01-27T16:28:38Z

Travis automatic deployment:
https://pr1792--safereact.review.gnosisdev.com/volta/app

Don't use multisend if txs.length = 1

c425e01

mmv08 requested review from rmeissner, fernandomg and dasanra January 19, 2021 07:12

calc txData inside useMemo

b435111

Merge branch 'development' into feature/no-multisend-for-1-tx

0c7570c

fernandomg approved these changes Jan 20, 2021

View reviewed changes

dasanra approved these changes Jan 26, 2021

View reviewed changes

Merge branch 'development' into feature/no-multisend-for-1-tx

82dc48d

dasanra requested a review from francovenica January 26, 2021 12:07

rmeissner suggested changes Jan 26, 2021

View reviewed changes

rmeissner reviewed Jan 26, 2021

View reviewed changes

Merge branch 'development' of github.com:gnosis/safe-react into featu…

ec1918d

…re/no-multisend-for-1-tx

fix sending txs from apps, use correct operation/recipient

9c27663

francovenica approved these changes Jan 27, 2021

View reviewed changes

rmeissner approved these changes Jan 27, 2021

View reviewed changes

Merge branch 'development' into feature/no-multisend-for-1-tx

1b35b86

dasanra merged commit d6343a9 into development Jan 27, 2021

dasanra deleted the feature/no-multisend-for-1-tx branch January 27, 2021 16:35

github-actions bot locked and limited conversation to collaborators Jan 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Single tx safe apps interactions should not use multisend #1792

Feature: Single tx safe apps interactions should not use multisend #1792

mmv08 commented Jan 19, 2021

github-actions bot commented Jan 19, 2021

lukasschor commented Jan 19, 2021

github-actions bot commented Jan 19, 2021 •

edited

Loading

ghost commented Jan 19, 2021

ghost commented Jan 19, 2021

tschubotz commented Jan 19, 2021

mmv08 commented Jan 19, 2021

ghost commented Jan 19, 2021

ghost commented Jan 19, 2021

ghost commented Jan 20, 2021

ghost commented Jan 20, 2021

dasanra commented Jan 26, 2021

ghost commented Jan 26, 2021

ghost commented Jan 26, 2021

francovenica commented Jan 26, 2021 •

edited

Loading

francovenica commented Jan 26, 2021 •

edited

Loading

rmeissner Jan 26, 2021 •

edited

Loading

mmv08 Jan 27, 2021

rmeissner Jan 26, 2021

rmeissner Jan 26, 2021

ghost commented Jan 27, 2021

ghost commented Jan 27, 2021

ghost commented Jan 27, 2021

ghost commented Jan 27, 2021

francovenica commented Jan 27, 2021

rmeissner left a comment

ghost commented Jan 27, 2021

ghost commented Jan 27, 2021

Feature: Single tx safe apps interactions should not use multisend #1792

Feature: Single tx safe apps interactions should not use multisend #1792

Conversation

mmv08 commented Jan 19, 2021

github-actions bot commented Jan 19, 2021

lukasschor commented Jan 19, 2021

github-actions bot commented Jan 19, 2021 • edited Loading

ESLint Summary View Full Report

ghost commented Jan 19, 2021

ghost commented Jan 19, 2021

tschubotz commented Jan 19, 2021

mmv08 commented Jan 19, 2021

ghost commented Jan 19, 2021

ghost commented Jan 19, 2021

ghost commented Jan 20, 2021

ghost commented Jan 20, 2021

dasanra commented Jan 26, 2021

ghost commented Jan 26, 2021

ghost commented Jan 26, 2021

francovenica commented Jan 26, 2021 • edited Loading

francovenica commented Jan 26, 2021 • edited Loading

rmeissner Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

mmv08 Jan 27, 2021

Choose a reason for hiding this comment

rmeissner Jan 26, 2021

Choose a reason for hiding this comment

rmeissner Jan 26, 2021

Choose a reason for hiding this comment

ghost commented Jan 27, 2021

ghost commented Jan 27, 2021

ghost commented Jan 27, 2021

ghost commented Jan 27, 2021

francovenica commented Jan 27, 2021

rmeissner left a comment

Choose a reason for hiding this comment

ghost commented Jan 27, 2021

ghost commented Jan 27, 2021

github-actions bot commented Jan 19, 2021 •

edited

Loading

francovenica commented Jan 26, 2021 •

edited

Loading

francovenica commented Jan 26, 2021 •

edited

Loading

rmeissner Jan 26, 2021 •

edited

Loading