Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

(Feature) Decode multiSend tx details#1106

Merged
fernandomg merged 88 commits intodevelopmentfrom
feature/#865-decoding-multiSend
Aug 4, 2020
Merged

(Feature) Decode multiSend tx details#1106
fernandomg merged 88 commits intodevelopmentfrom
feature/#865-decoding-multiSend

Conversation

@fernandomg
Copy link
Contributor

@fernandomg fernandomg commented Jul 8, 2020

This PR closes #865, by properly presenting tx's decoded data for multiSend txs in the tx's expanded view.

multiSend-details-v2

note: I converted it to draft because, despite it's functional and can be tested, there are pending components refactor required. See 42ed908

@fernandomg fernandomg self-assigned this Jul 8, 2020
@ghost
Copy link

ghost commented Jul 8, 2020

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

2 similar comments
@ghost
Copy link

ghost commented Jul 8, 2020

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

@ghost
Copy link

ghost commented Jul 8, 2020

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

@fernandomg fernandomg marked this pull request as draft July 8, 2020 20:54
@ghost
Copy link

ghost commented Jul 13, 2020

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

@lukasschor
Copy link
Contributor

I loaded this Safe to see how a Sablier Safe App interaction looks like with decoded Multisend: 0xCfF7eB7C0cAc85188dF7748486A3ab8a20F56315

While doing that, I saw that there is a "Cancellation transaction" being displayed, which is not the case on the production version:
image

So seems like there is something broken with the tx <> cancellation transaction merging.

@ghost
Copy link

ghost commented Jul 13, 2020

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

@lukasschor
Copy link
Contributor

image

Could we make the function & parameter names bold?

Also, Richard had the idea of using the function name in the overview to make it easier to see which Transaction should be expanded to get some information on specific things. So instead of "Transaction 1" it could say "Transaction 1 (approve)". I think this might be an improvement that does not require too much effort, but happy to get your thoughts on this. I'm also fine leaving it out and opening a separate ticket for this.

@francovenica
Copy link
Contributor

Regarding the names of the Tx's. They are shown in the Tx details but not in the app operation itself:
Example: this is a "Supply" in compound, but in the tx details the only thing you can see is the "mint" and "approve" so you don't know that it was a Supply unless you remember it. Also the when you Supply the modal won't show that you are minting and approving
image.png
image.png

So we could put that this is a "Supply" operation and not just the "mint" and "approve" that is internally done.

This is an example of one app. I still gotta see how it looks on the others

@lukasschor
Copy link
Contributor

lukasschor commented Jul 14, 2020

Regarding the names of the Tx's. They are shown in the Tx details but not in the app operation itself:
Example: this is a "Supply" in compound, but in the tx details the only thing you can see is the "mint" and "approve" so you don't know that it was a Supply unless you remember it. Also the when you Supply the modal won't show that you are minting and approving
image.png
image.png

So we could put that this is a "Supply" operation and not just the "mint" and "approve" that is internally done.

This is an example of one app. I still gotta see how it looks on the others

The issue is that the "Supply" name is a human-readable name given by the Safe App. But on the transaction level this information is basically encoded as a approve and mint. We cannot take any information from the Safe app to rely on, as this would create false security to the user and a malicious app could take advantage of this. So the base is the direct transaction data, which we then should aim to decode as much as possible over time to show to the user the consequences of the transaction in a human-readable manner.

And yes, we will also display the decoded Data in the Safe Apps modal, but this is not part of this PR.

@ghost
Copy link

ghost commented Jul 16, 2020

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

@github-actions
Copy link

github-actions bot commented Jul 30, 2020

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 2 0
Ignored 3 N/A
  • Result: ✅ success

  • Annotations: 2 total


[warning] @typescript-eslint/explicit-module-boundary-types

Require explicit return and argument types on exported functions' and classes' public class methods


Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Jul 30, 2020

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

2 similar comments
@ghost
Copy link

ghost commented Jul 30, 2020

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

@ghost
Copy link

ghost commented Jul 30, 2020

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

@francovenica
Copy link
Contributor

I repeated the same test I did the last time, trying every app possible in rinkeby and checking that they look fine in the tx and its details

The safe: https://pr1106--safereact.review.gnosisdev.com/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

image.png

image.png

@ghost
Copy link

ghost commented Jul 31, 2020

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

# Conflicts:
#	src/logic/addressBook/store/selectors/index.ts
#	src/routes/safe/components/Balances/SendModal/screens/AddressBookInput/index.tsx
#	src/routes/safe/store/models/types/transaction.ts
#	src/store/index.ts
@ghost
Copy link

ghost commented Aug 3, 2020

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

@fernandomg
Copy link
Contributor Author

@mikheevm, please, don't forget to review recent changes

export interface AddressBookMap extends Map<string> {
toJS(): AddressBookMapSerialized
get(key: string): List<AddressBookEntryRecord>
get(key: string, notSetValue: unknown): List<AddressBookEntryRecord>
Copy link
Contributor

@mmv08 mmv08 Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return type is a little bit off here, it can also return typeof notSetValue

@francovenica
Copy link
Contributor

Tried one more time in a new safe and incognito mode and it worked fine
I've tried Compound, Sablier, Idle and The Tx builder (replicating a compound contract method).

Looks good to me

Safe for testing: https://pr1106--safereact.review.gnosisdev.com/app/#/safes/0x5D0edD9272a653Bf6b3b49e9856c7030942d2003/transactions

image.png

@github-actions
Copy link

github-actions bot commented Aug 4, 2020

CLA Assistant Lite All Contributors have signed the CLA.

@ghost
Copy link

ghost commented Aug 4, 2020

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

@fernandomg fernandomg merged commit a0ed0a1 into development Aug 4, 2020
@fernandomg fernandomg deleted the feature/#865-decoding-multiSend branch August 4, 2020 18:32
@github-actions github-actions bot locked and limited conversation to collaborators Aug 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Critical Only for bugs in released apps, needs to be fixed asap and hotfix needs to be shipped. Feature 👑 New functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Decoding] Multisend

5 participants