Safe-react: v1 Decode transaction preview - Safe apps

[lukasschor]

What is this feature about? (1 sentence)

This will display decoded transaction info on the tx review screen when making transactions.

Why is it needed? What is the value? For whom do we build it?

• This is directly connected to the problem area "No decoded transaction details when creating a transaction" on our roadmap. It was identified as top priority.
• As a user, I want to know what a Safe Transaction does, before initiating it so that I don't sign anything wrong.
• Currently we only decode transaction information for transactions in the transaction list and when triggering a custom transaction via ABI.
  
• When initiating Safe App interactions, tx data is displayed in encoded form only.
  

High-level overview of the feature

• Decode transaction information in the transaction preview modals, including the ones for the Safe app.
  
  
  
  • That's important in order to not have many different types of tx modals inside the app.
  
  
• There is a lot of custom logic for decoding transactions. It probably makes sense to let the backend handle that.

• Let's limit complexity by e.g. not decoding nested Multisends but rather just 1 level.

• During release planning March 9, we decided to split this ticket up:

  • v1: Only for Safe app txs (Since that's where it is most important)
  • v2: Contracts interaction (Both via ABI and with raw tx data) - #2042
  • v3: Create a generic modal component so we can use it as a base for all the modals in safe-react - #2049
  • v4: Make a full refactor of safe-app review modal using the new GenericModal component - #2043

Misc

• Kick off doc: https://docs.google.com/document/d/1G4AttQUc8nlWimfqiVLqdozpnZ3tljVzwt32yzFU7Fg/edit#

[tschubotz]

I think none of the Safe apps does just a simple ERC20 token transfer, if I'm not mistaken. Only if you add a transfer call via the tx builder.

What a lot of Safe apps do though is first calling ERC20 approve - so I think that would be better suited as a first step.

[lukasschor]

To decode all contract interactions where backend has the ABI, we need a new backend endpoint

[nicosampler]

@Uxio0 do you think is it possible to expose in a new endpoint the logic you are using to decode TXs?
So I could consume that endpoint with the array of TX a Safe-App want to send.

[Uxio0]

@nicosampler I could expose the ABIs, but there's a lot of custom logic for decoding (multisend, nested transactions...).

[lukasschor]

Decode transaction data during tx creation flow (Safe Apps / Contract interactions)

[posthnikova]

1) The first layout option would be to divide the popup into 2 columns and show selected transaction decoding in the right column.

When there are many transactions this part should scroll:

2) The second option is to only show the number of transactions, all details are hidden into a separate view, you can open them one by one.

[nicosampler]

+1 to option 1

[Agupane]

IMO I prefer the second option, it will be also scrollable right?

[tschubotz]

Thanks for the proposals!
Here are my thoughts:

Option 1:

• I like that bigger modal
• I think we don't take advantage of that bigger modal since the decoded data is quite cramped on the side.
• Also, currently the act that I see tx1,tx2, etc. bring no advantage. If available, could we display the destination and function name there? Similar to how we do it on mobile?
  
• I think that scrolling would be fine.

Option 2:

• I think these subpages on the modal could also work. It's similar to the tx details on mobile actually.
• I'm not the biggest fan of the accordion components. I would rather open another sub page for a specific tx since there could be quite some tx data to display.

General thoughts:

• I would love if there's still a way to inspect the raw tx data. (Like we do e.g. here)
• I think we should try to take advantage of the full modal width somehow. I like how we currently show decoded data in the contract interaction flow
  
• Safe app tx can also consist of just a single tx without a multisend, i.e. it would be similar to a simple contract interaction.

Let me know if this makes sense or you need more clarification on what I mean. Also let me know in case you need more input, happy to do a session together with you!

[Agupane]

As discussed with @fernandomg we propose this approach:

UI:

Using both #1 and #2 + Tobi's approach mixed
1) First the user will see something like this
2) Then if he expands a multiSend transaction he will look a modal with a list of transactions
3) In case he expands a normal transaction he will see something around this
4) In case a multiSend transaction contains more multiSend transactions the behaviour will be repeated from step 2) until the user reaches a normal transaction and will be able to see it like 3)

| 1 | 2 | 3 |
|---|---|---|
||||

Backend/Decoded data:

• The endpoint we need would be something like this:

POST /decodeData

{
 "hexData": "0x."
}

The response should be like this (based on how the client gateway it's working now)

type Response = {
 txData: DecodedTransaction
}

The types are:

type Params = {
  name: string; 
  type: string;
  value: string;
}

type DataDecoded = {
  method: string;
  params: Params[];
}

type DecodedTransaction = {
  hexData: string; 
  dataDecoded: DataDecoded | DecodedTransaction[]; // dataDecoded will be DecodedTransaction[] for isMultiSend === true
  isMultisend: boolean;
}

We expect that the decodification will go for the first level, in case there are nested multiSend transactions it won't return dataDecoded value, just hexData so we can ask for decoded data later in case we need it

[Agupane]

cc @tschubotz @rmeissner @Uxio0 @dasanra @pablofullana

[rmeissner]

cc @jpalvarezl <- responsible for client gateway implementations

[fernandomg]

cc @posthnikova about the UI flow feedback

[posthnikova]

I used @Agupane's example with some changes after Tobi's comments. The first step is a list of actions, details are in a separate view.

The list inside the first view could scroll, action details could also scroll:

If available, could we display the destination and function name there?

@tschubotz Yes, done.

I think we don't take advantage of that bigger modal since the decoded data is quite cramped on the
side.

Big modal only looks good when there are 2 columns, otherwise the line of text is too long, it's hard to read.

Safe app tx can also consist of just a single tx without a multisend, i.e. it would be similar to a simple
contract interaction.

Yes, I thought non-multisend transactions don't change.

I would love if there's still a way to inspect the raw tx data

Full data could be very long, I would prefer to expand it with 'Show more' link.

We expect that the decodification will go for the first level

@Agupane Then in your example step 2 wouldn't exist, we show step 3 with hex data.

[Agupane]

Hi @posthnikova thanks for the feedback! I´m just curious about this:

@Agupane Then in your example step 2 wouldn't exist, we show step 3 with hex data.

What happens if some transaction inside a multiSend transaction is also a multiSend? with the approach we suggested with @fernandomg it should be a kind of recursive behavior that would let the user also decode nested multiSend transactions but with the new proposed UI it won't be possible right?, please correct me if I'm wrong

[posthnikova]

What happens if some transaction inside a multiSend transaction is also a multiSend?

The spec says we only decode 1 level. But according to your approach it will look like this.

[rmeissner]

Personally I find this confusing from the UI, as it was not immediately clear to me that the 2 multisend actions are on the same level as the other 3 (different background color, no blockies, no action).

Edit: Ahhhhhh the different background color is the hover state .... sorry was not super clear.

Edit 2: I would expect something like:

| <> Multisend                        2 actions |
| () 0xbadd....adad            safeTransferFrom |

[Agupane]

The spec says we only decode 1 level. But according to your approach it will look like this.

Yes exactly, for example, we have one multiSend transaction (let's call it X) that contains for example 3 inner transactions:

• safeTransferFrom
• changeMasterCopy
• another multiSend -> lets call it Y

When we ask the backend to decode the transaction X we will get that information (the three inner transactions) but the transaction Y won't be decoded to avoid overprocessing on the backend. We will only get the hash of it so if the user decides to see the details of that transaction we will ask the backend for it on demand. That's what I said "the decodification will for the first level". That does mean that the user should be able to see the entire data if he want's and we would need to have some kind of recursive UI or modals for allowing that

[rmeissner]

Different question: how do the different dialogs overlay each other? (e.g. does the one with the list of tx closes when going deeper, or is it just a different UI state of the same dialog, or does it overlay the other dialog)

[posthnikova]

@rmeissner Would that work for you?

@Agupane Thanks for clarification. So it could have multiple levels:

Different question: how do the different dialogs overlay each other?

This is the same modal, no overlays, you just go back and forth.

[rmeissner]

@posthnikova yes I prefer this UI :) thank you very much (also thanks for clearifying the modal behaviour)

[tschubotz]

Thanks for the screens! I like them

Yes, I thought non-multisend transactions don't change.

We don't have generic tx decoding for non-multisend txs currently though. Do I understand your comment correctly that you wouldjust reuse this?

If so, then we would need just some small changes like: Adding the Safe app name on top.

In case there are many actions, would there we a scroll?

[posthnikova]

We don't have generic tx decoding for non-multisend txs currently though. Do I understand your
comment correctly that you wouldjust reuse this?

@tschubotz Yes, I meant this screen. This is weird that contract interactions don't have a Safe name in the header while all other tx types have it.

In case there are many actions, would there we a scroll?

Yes, when there are more than 5 actions the list scrolls. Popup with details could also scroll.

[tschubotz]

@tschubotz Yes, I meant this screen.

Nice, then let's reuse it.

This is weird that contract interactions don't have a Safe name in the header while all other tx types have it.

What do you mean? They all look similar to me 🤔

[tschubotz]

One more comment to:

I think the destination ("to"/recipient) should be displayed on the detail screen as well. Would it be reuse the styles from the generic contract interaction screen? That way, the styles could be reused.

[posthnikova]

What do you mean? They all look similar to me

@tschubotz Weird, sometimes there's no Safe address on the review step https://prnt.sc/zutsmo. I've no idea why.

I think the destination ("to"/recipient) should be displayed on the detail screen as well.

There is a recipient in the designs, you were looking at a scrolling view.

Would it be reuse the styles from the generic contract interaction screen? That way, the styles could
be reused.

I'm not sure which styles to reuse. The same info is shown differently in different places. In tx details for decoded data a recipient is shown as 'Send 0 ETH to...'.

The same information in contract interaction popups (non-multisend) is shown as 'Recipient ..., Value 0 ETH'.

In multisend popups it says 'Transaction n' and 0 ETH.

I was thinking maybe use the 'Send 0 ETH to...' format for all these cases so it's more aligned with tx details and mobile app.

So current contract interaction (non-multisend) popup styles need to be updated.

[tschubotz]

@tschubotz Weird, sometimes there's no Safe address on the review step https://prnt.sc/zutsmo. I've no idea why.

I found it: When doing a contract interaction with ABI, then it's missing (your screenshot). When doing a contract interaction with raw tx data, then it's all good (my screenshot) 💡 lol, nice catch. I think this should all look nicely with the header, as you suggested.

There is a recipient in the designs, you were looking at a scrolling view.

Make sense, thanks!

I'm not sure which styles to reuse. The same info is shown differently in different places. In tx details for decoded data a recipient is shown as 'Send 0 ETH to...'.

I was thinking maybe use the 'Send 0 ETH to...' format for all these cases so it's more aligned with tx details and mobile app.

Yes, I think this makes most sense. The tx details was never worked on, so I wouldn't take them as reference (and fix them in #1887).

So current contract interaction (non-multisend) popup styles need to be updated.

I think that's okay. I think the goal of this ticket should be to align the review screens, not matter from where the tx was started.

[posthnikova]

Changes after quick user tests: https://invis.io/EY108QA8Q3XK

What's been done:
• Recipient added to the parent transaction
• Undecoded data moved up, show how many bytes, don't expand, there's a copy button
• There will be no second level multisends, show contract interaction with action 'multisend' instead. Inside 'multisend' action there's undecoded data.
• Removed identicons in the actions list, show tx type 'contract interaction' instead.

Contract interaction (non-multisend): https://invis.io/EY108QA8Q3XK#/445629652_Contract_Interaction_-Review-_1272px_

Actions list scroll: https://invis.io/EY108QA8Q3XK#/445629655_Transaction_Builder_-Scroll-_1272px_

All types of parameters:

[tschubotz]

Thanks! I think this looks nice.

Could you add a case where we are not able to decode one of the actions, please?
I would assume:

• On the list, it could just be a "contract interaction" without any function name.
• On the details view we would just see Eth amount, target address and data (not the actual data, just the length and the copy button, just like on the other screens)

About the Multisend inside a Multisend: With our current interface, it's impossible to create such a multisend inside a multisend. The reason for this is quite technical. We don't allow users of the interface and Safe apps to create so called "delegate calls". The reason for that in turn was security. You can do a lot of harm with them. So it needs to be clear in the interface when a delegate call happens and hence we just left it out for simplicity.
I think it's good to have this kept in mind when designing, but I would take the multisend inside the multisend out of the screens for zeplin just to be clear about it.

(I have to check if there is a theoretical option to create such real nested multisends via the API)
Edit: Yes, via the api that's possible. But even then it wouldn't be visible in the tx creation popup.

[posthnikova]

Could you add a case where we are not able to decode one of the actions, please?

@tschubotz On mobile we decided to show 'Action #' in the list and details https://github.com/gnosis/safe/wiki/Transaction-decoding-v2%3A-design-decisions#4-what-to-show-when-method-is-not-available

About the Multisend inside a Multisend: With our current interface, it's impossible to create such a
multisend inside a multisend.

True, I was confused by examples on Slack https://gnosisinc.slack.com/archives/CSPB31JHW/p1613742762004400, they are actually not nested. It will show as contract interaction with action name 'multisend', the details look like this https://invis.io/EY108QA8Q3XK#/445185688_Multisend_1st_Level_1272px_.

[Uxio0]

@lukasschor @tschubotz Currently we have 2 types of _nested_ transactions:

• Multisend transactions
• Calls to Gnosis Safe execTransaction (so there's a Safe transaction inside a Safe transaction)

Should we keep decoding calls to execTransaction for this feature?

[tschubotz]

Should we keep decoding calls to execTransaction for this feature?

You the data part of exec transaction?

I would say not for this feature since this would be a special handling of Safe txs. The clients don't use this, so no need to have it in the backend. What do you think?

[Uxio0]

You the data part of exec transaction?

I mean, when you run a Safe tx that calls another Safe execTransaction the inner tx is also decoded (it's an special case coded in the tx decoder). I don't have any problem returning it, as it's already done, but I can remove it if it's confusing. I would return it, I think having more information should not harm, but I don't have a strong opinion about it.

This case would apply, for example, when one Safe 1 is owner of other Safe 2 and calls executeTransaction of the Safe 2

[tschubotz]

Ah, it already done. In that case, I'd return it, I don't think it's confusing.
Not sure if @dasanra @fernandomg @Agupane @rmeissner see this differently.

[fernandomg]

I agree, lets keep all the data, we can discard it in the UI if we consider it's not necessary.

[Uxio0]

This feature is already in every environment (including production) https://github.com/gnosis/safe-transaction-service/issues/218 . Swagger is wrong somehow, payload should be:

{
 "data": "0x."
}

[tschubotz]

For testing this, https://rmeissner.github.io/transaction-templates/ could come in handy.

Example json: https://github.com/rmeissner/transaction-templates/blob/main/templates/easy_auction.json

cc @liliya-soroka @francovenica