Safe-react: [Transaction List] Use unified Tx endpoint

[lukasschor]

Use new unified tx endpoint https://github.com/gnosis/safe-transaction-service/issues/81

https://safe-transaction.mainnet.staging.gnosisdev.com/api/v1/safes/

/all-transactions/

[fernandomg]

@Uxio0, currently the safe's transactions GET response returns a count, with the total count of txs for that safe.

Thinking of the Transaction Cancellation implementation (were we group txs under the same row), and the future possibility of having more than 1tx with the same nonce.

Can you return another value with the total unique (by nonce) txs for that safe?

For instance: if I have 10 txs with 4 cancel requests txs. Then the count will be 14, but this new value would be 10

[Uxio0]

Hi Fernando,

Why is that needed?

Regards

[fernandomg]

let's say I have a new safe.

1. I create a first outgoing tx, then I cancel it.
2. Next, I create a new outgoing tx, then it gets canceled.
3. Finally, I create a tx and it goes through.

I'll be rendering 3 rows for the outgoing txs (because all the txs with the same nonce will be grouped under the same row)

But I'll have 5 outgoing txs. And this is the count value I'm getting from the backend. But for the user there are 3 rows in the txs table.

--
That can be calculated on the fronted, for a small amount of txs. Now, if my safe have 1M of outgoing txs... this will be impractical, to say the less.

[Uxio0]

Ok, I understand, but you can already get that number calling the transaction endpoint with a filter on executed (/transactions/?executed=1)

But if you have 1M of outgoing txs... that will be paginated and you don't need the number, right? Even if you have less rows, there are that number of txs in the table

[fernandomg]

Let me put it in an example:

If you go to: https://safe-transaction.rinkeby.gnosis.io/api/v1/safes/0x441E604Ad49602c0B9C0B08D0781eCF96740786a/transactions/ (with and without the ?executed=1 param)

You'll get a list of 35 txs. But we will render only 29 rows out of those 35.

> txs.length
35
> _.uniqBy(txs, 'nonce').length
29
> 

That 29 filter/counter value is the one that I need for all the existing transactions for a safe.

[Uxio0]

Sorry @fernandomg, I was mistaken, the query is https://safe-transaction.rinkeby.gnosis.io/api/v1/safes/0x441E604Ad49602c0B9C0B08D0781eCF96740786a/transactions/?executed=true or https://safe-transaction.rinkeby.gnosis.io/api/v1/safes/0x441E604Ad49602c0B9C0B08D0781eCF96740786a/transactions/?executed=false

[fernandomg]

@Uxio0, let me go with a final example:

There you have 4 windows.

1. request for the whole list of txs
   
   
   
   • result: 44txs
   
   
2. request for txs _executed_
   
   
   
   • result: 29txs
   
   
3. request for txs _not executed_
   
   
   
   • result: 15txs
   
   
4. the uniqueTxs count that I need
   
   
   
   • total txs: 44, uniqBy nonce, 31.
   
   

if you happen to see the safe https://pr451--safereact.review.gnosisdev.com/safes/0x441E604Ad49602c0B9C0B08D0781eCF96740786a/transactions:

you'll see that there are 2 txs that werent executed, yet they are rendered.

[Uxio0]

I understand, you need also the pending txs not executed.

I created a new issue for the services https://github.com/gnosis/safe-transaction-service/issues/60 @lukasschor

[fernandomg]

I understand, you need also the pending txs not executed.

I created a new issue for the services gnosis/safe-transaction-service#60 @lukasschor

Yes, but only the amount. Not the list of txs. That's why my initial request was to have another attribute along with the txs list, with that counter.

Something around:

{
  "count": 44,
  "unique": 31,
  "next": null,
  "prev": null,
  "results": []
}

[fernandomg]

this is related to #431

[Uxio0]

count_unique_nonce field has been added to the /transactions/ endpoint, it's already on every environment

[Agupane]

Hi guys I need your opinion regarding this issue.

As discussed with @Uxio0 and @fernandomg, doing the implementation of the pagination on the frontend with the current endpoint's will give us problems on the frontend:

• Now there are two endpoints one for transactions and another one for incoming transactions but we show all the transactions in one tab, so, in order to paginate, we would need to fetch both endpoints, reorder them and discard the ones that don't "enter" into the page. This makes that the pagination is just an illusion, there is no real pagination. We have several issues with the performance of the app, doing this won't improve it, maybe will downgrade it more.

• Also force us to keep a register of which transactions were discarded in the last page to add them in the new page every time the users moves between pages

I think that the pagination should be solved by the backend in only one endpoint that provide us the params for fetching the transactions directly.

As discussed with @Uxio0 he said that doing this will cause problems on the backend because there are filters that aren't available for both types of transactions. My suggestion was to create a new endpoint just for this functionality

We have also discussed in the idea of dividing the transactions tab into two tabs: one for each type of transaction, doing this will directly solve the problem and the pagination will be easier. BUT we need opinions because this will affect the user experience (@lukasschor), we think that the user will expect to have all the transactions merged into one tab, like the transactions of a normal bank account.

Please feel free to add comments and opinions !

CC: @rmeissner @pablofullana @mikheevm

[Uxio0]

Hi @Agupane!

• Now there are two endpoints one for transactions and another one for incoming transactions but we show all the transactions in one tab, so, in order to paginate, we would need to fetch both endpoints, reorder them and discard the ones that don't "enter" into the page. This makes that the pagination is just an illusion, there is no real pagination. We have several issues with the performance of the app, doing this won't improve it, maybe will downgrade it more.

Could you explain the performance issues you are talking about? From my opinion the biggest impact would be not processing and generating the list every time that's retrieved from the service if the list does not change. Endpoints already support weak Etag. That's something that can be already done in frontend with the current implementation.

• Also force us to keep a register of which transactions were discarded in the last page to add them in the new page every time the users moves between pages

I agree on this, pagination is not easy. The time filters made it easier, but as you say, it's something difficult to manage from the frontend.

I think that the pagination should be solved by the backend in only one endpoint that provide us the params for fetching the transactions directly.

As discussed with @Uxio0 he said that doing this will cause problems on the backend because there are filters that aren't available for both types of transactions. My suggestion was to create a new endpoint just for this functionality

Not just that. I don't want to have different objects in the same endpoint breaking typed clients, so I would need a proposal of an endpoint were every object is the same (with the same fields, even if some are null for some cases, e.g. incoming ether transfers will not have confirmations). It will not be easy also to do that in the backend as it will involve merging completely different tables, I'd be happy to implement that if we can think on an endpoint with all the use cases and the filters that will be needed in that case.

We have also discussed in the idea of dividing the transactions tab into two tabs: one for each type of transaction, doing this will directly solve the problem and the pagination will be easier. BUT we need opinions because this will affect the user experience (@lukasschor), we think that the user will expect to have all the transactions merged into one tab, like the transactions of a normal bank account.

I, as a user of the old multisig would like to have this feature, as I don't want the incoming txs messing with the transactions I'm preparing, but a filter will work too. Regular banks don't have "New owner", "You changed your password", "You are approving a new credit card" in the middle of the regular money transfers, but I understand that some users will expect to see all the transfers in the same tab.

[Agupane]

Could you explain the performance issues you are talking about? From my opinion the biggest impact would be not processing and generating the list every time that's retrieved from the service if the list does not change. Endpoints already support weak Etag. That's something that can be already done in frontend with the current implementation.

Yep I agree with you but I also think that we would need to make two requests instead of one, now maybe it's not a big problem, but if we go mobile it will be a problem for slow connections, lets say we fetch a page of 100 txs, we will need to make two request and we will receive 100 txs from both endpoints than we need to reorder/discard unused to generate the list, instead of fetching directly the 100 transactions that we need

Usings etags would be a really good approach to improve the performance, should be implemented on the frontend of course :) but the problem of fetching twice the data at least the first time, will be still there

Not just that. I don't want to have different objects in the same endpoint breaking typed clients, so I would need a proposal of an endpoint were every object is the same (with the same fields, even if some are null for some cases, e.g. incoming ether transfers will not have confirmations). It will not be easy also to do that in the backend as it will involve merging completely different tables, I'd be happy to implement that if we can think on an endpoint with all the use cases and the filters that will be needed in that case.

A merge of both object would be what I expect, making just null the fields which are not present in one transaction, for instance:

Now:
Incoming transaction: { executionDate: '01-01-2020', blockNumber: 1, transactionHash: 0x..123, to: 0x..123, value: 5, tokenAddress: 0x..456, from: 0x...456}

Other Transaction: { executionDate: '01-01-2020', blockNumber: 1, transactionHash: 0x..123, to: 0x..123, value: 5,  data: 0x...789, gasToken: 0x...123, 
operation: 0, confirmations: [], ......}

Merged obj from "transaction": { executionDate: '01-01-2020', blockNumber: 1, transactionHash: 0x..123, to: 0x..123, value: 5,  , tokenAddress:null from: null, data: 0x...789, gasToken: 0x...123, 
operation: 0, confirmations: [], ......}

Merged obj from incoming transaction: { executionDate: '01-01-2020', blockNumber: 1, transactionHash: 0x..123, to: 0x..123, value: 5, tokenAddress: 0x..456, from: 0x...456, data: 0x...789, gasToken: 0x...123, 
operation: null, confirmations: null, ......}

Let me know if I was clear enough :)

I, as a user of the old multisig would like to have this feature, as I don't want the incoming txs messing with the transactions I'm preparing, but a filter will work too. Regular banks don't have "New owner", "You changed your password", "You are approving a new credit card" in the middle of the regular money transfers, but I understand that some users will expect to see all the transfers in the same tab.

I agree with you, maybe we would need more feedback from the users regarding this, WDYT @lukasschor ?

[fernandomg]

@rmeissner, @mikheevm, any thoughts on this?

[mikheevm]

I agree with Agus on this. It's very difficult to merge these two lists together given not any transaction is rendered, we need to sort them by date submitted, etc.

Also this will include further complexity when we add sorting, filters, etc

I would say the best solution (from frontend perspective) is to merge these two endpoints into one

[lukasschor]

@rmeissner @Uxio0 Any further thoughts on this?

[rmeissner]

I do agree that 1 endpoint makes it easier on the UI (always did). But we should properly define the endpoint and what we expect. Just saying "merge all properties and null the rest" is for me bad practise. Then we should define a proper polymorphic endpoint (e.g. add a type parameter with incoming_erc20, incoming_erc721 or outgoing_tx) this way we can add more potential transaction types in the future (e.g. incoming_erc1155 or maybe outgoing_ercX).

Also in general it is not that hard to merge these to lists with the current sorting (timestamp) ... it takes 2 requests and one time iterating through these lists.

Currently the backend doesn't apply any more magic (and this is currently not planned) -> e.g. filtering or grouping cancelation transactions. If this is a requirement please not them down clearly (e.g. specify what is a cancelation transaction).

Also we are kind of promoting our backend to be used be everyone, so having very specialized views for the clients are kind of weird. A solution could be to have a client gateway (basically a separate service in between the interface and the gnosis services).

[mikheevm]

Also in general it is not that hard to merge these to lists with the current sorting (timestamp) ... it takes 2 requests and one time iterating through these lists.

It's not about merging two lists into one, we already do this, this is about writing all the logic to get actually 100 transactions to show up, keep in mind we also have cancellation transactions which are not shown as a separate transaction but only in the details (I guess you don't want them to count for a separate one)

[mikheevm]

we should properly define the endpoint and what we expect. Just saying "merge all properties and null the rest" is for me bad practise.

I don't think this is a problem at all, in the end they're all transactions on the ethereum blockchain

[rmeissner]

keep in mind we also have cancellation transactions which are not shown as a separate transaction

In this case the backend issue should be "Add possibility to filter cancellation transactions" ;)

I don't think this is a problem at all, in the end they're all transactions on the ethereum blockchain

This is bad API design because currently they have all different properties, as the incoming transfers are not really parsed transactions, but parsed events (so there is even a difference on the very low level)

[mikheevm]

In this case the backend issue should be "Add possibility to filter cancellation transactions" ;)

Yeah, if cancellation transaction would be treated as confirmation with type "REJECTION", it would solve this issue too. Currently we have only EXECUTION and CONFIRMATION or something like that, and Uxio told me that it's going to get removed 😃

[Uxio0]

Yeah, if cancellation transaction would be treated as confirmation with type "REJECTION", it would solve this issue too. Currently we have only EXECUTION and CONFIRMATION or something like that, and Uxio told me that it's going to get removed 😃

No, currently there's only ConfirmationType=CONFIRMATION inside every confirmation (that's something inherited from the alpha of the tx service when working with Adolfo), so you are not getting any information from that 😉

[pablofullana]

guys, any agreement on how to proceed?

1. updating the backend by adding a new endpoint returning both incoming and outgoing transactions merged in one list. without breaking current API design in terms of already existing endpoints.
2. merging both lists on the frontend and dealing with complex and low-performing logic from now on.
3. displaying two different lists in the UI instead of just one.

_still_ open for discussion

[Uxio0]

guys, any agreement on how to proceed?

1. updating the backend by adding a new endpoint returning both incoming and outgoing transactions merged in one list. without breaking current API design in terms of already existing endpoints.
2. merging both lists on the frontend and dealing with complex and low-performing logic from now on.
3. displaying two different lists in the UI instead of just one.

_still_ open for discussion

Hi Pablo, we are going for the 1, but we are still deciding how to do it as it's not trivial

[lukasschor]

This depends on this backend ticket: https://github.com/gnosis/safe-transaction-service/issues/81

Putting it in the icebox for now.