[presign] Enhance transactions and refactor #1429

anxolin · 2021-09-10T12:54:21Z

Summary

This PR is important part for improving Gnosis Safe UX, but also give us more freedom in order to better handle our transactions.

This PR Includes:

Enhance the transaction state (state, actions, redux, etc) to allow our custom logics
Refactor the previous Enhancement, which was great, but it had some limitations

Limitations on previous approach:

The previous enhancement approach was a very good solution for cancelation/replacement, because for this 2 functionalities you can implement them without redefining the state and actions parameters/data.
The past approach, would allow you to add new updaters, and new actions, but not modify the existing ones
If you need to model transactions differently, for example to add new operations, you need to necessarily modify the state/actions. As you guessed, I had to do this, so I wanted to get full control of the Transactions model

Main motivation:

Uniswap code models Transaction data out of their use cases, so they have "claim" wich we don't have, and doesn't model the presign that we do
Also, I would like to be able to add more semantics to the model. This way, i can for example include Gnosis Safe information on a Transaction, which was main main trigger in order to do this change

Changes / hints for reviewer

There's a new state state/enhancedTransactions
It has the same actions of state/transactions but using our own model. Now we have freedom of define Tx however we want!
The updaters are now a folder, and contains FinalizeTxUpdater which is the logic from Uniswap that watch tx and change the state when they are mined. Also I added CancelReplaceTxUpdater which is the logic of cancelation and replacement of transactions that Ramiro did. This way, we can keep adding updaters and separating them. Also, now we have possibility to modify the original FinalizeTxUpdater code.
addTransaction now changes slightly the parameters. You need to provide a hash, and a hash type. This is because we won't be assuming that the hash is a Ethereum transaction, its possible is for example a Gnosis Safe Hash. Other wallets might use different hashes.

Issues detected

Wrap ETH to WETH not finishing --> it finish for EOA. For Gnosis Safe will work in future PRs. Right now, we still don't have access to the original Gnosis Safe transaction, so we cannot know when is mined. Future PRs will solve this
Approve doesn't show in the tx list

To Test

Everything should work as before. Note that in this branch, we use presign, even for normal EOA. Also, note that #1416 fix for showing the loading indicator is not included in this PR.

Create an order
See it in pending order
Wait until is executed

wrap ETH
wait until is mined

github-actions · 2021-09-10T13:50:02Z

🔭 GP Swap: Gnosis Protocol v2 Swap UI

MareenG · 2021-09-22T11:11:06Z

I tested it a bit with an rinkeby Safe:
I swapped ETH to WETH - In the sidebar the status of the transaction remains as open eventhough the transaction went through.

Transaction details Safe:

Etherscan link provided by Safe:
https://rinkeby.etherscan.io/tx/0x976d26aadc2cfe1bd26ac77216104e1e49e9a8079797282b6948621be370a3ef

Cowswap:
Still open 10 min after it was approved

Etherscan link provided by Cowswap:
https://rinkeby.etherscan.io/tx/0x9b015e86a365c04443d3c11d0cadddb55d0dba3b9e5e5c6ea29ee6b868a1a0f2

elena-zh · 2021-09-22T11:31:45Z

To add, Approve transaction does not appear in the side bar:

https://rinkeby.etherscan.io/tx/0xf890fafb45a50dd98ddd915392ce3cf111bca4a04821c373fd518f67415a9e07

I'm not sure if this issue related exactly to this PR or not, but I faced the conneciton issue: I reloaded the safe, and it appears that connection was lost there. However, the Cowswap app showed still connected state, Showed safe balances, etc. But I was not able to run transactions from there:
video: https://watch.screencastify.com/v/shhTyPtZmyHyf77XMKvY

MareenG · 2021-09-22T11:38:36Z

Sorry if this as addressed somewhere before in the other presign PRs.
I feel the current flow is not so intuitive (this actually could apply for other wallets connected via WC as well)
I am missing a call for action after clicking on the "Allow Cowswap to use your Token XY" or after pressing "confirm Swap".
When connected via the Safe it feels like nothing is happening.... only if the user manually switches to the safe website.

https://user-images.githubusercontent.com/34510341/134336291-d42f7286-a6ef-4f51-8e16-de2f62be373d.mov
This is especially the case for approve tokens or ETH/WETH swaps as no waiting for confirmation modal pops up.

This also applies for other wallets( e.g. MM or imToken) connected through WC. But I usually had the app open on my mobile and could see the call for action directly there.
I was wondering what you think about adding a popup with a call for action "Confirm the transaction with your Safe" or add this in the waiting for confirmation modal.
Just wanted to mention this as I shortly felt stuck when trying it out even though I used WC now quite often. Also I am not a frequent user of the Safe in connection with Apps. So not sure what you think about it :) .

MareenG · 2021-09-22T11:45:21Z

I swapped ETH to WETH - In the sidebar the status of the transaction remains as open eventhough the transaction went through.

I retried it and all remain in the open status.

MareenG · 2021-09-22T11:47:20Z

Buy and Sell orders work nicely

anxolin · 2021-09-23T10:31:54Z

Thanks @MareenG and @elena-zh for the tests!

I will add some of the defects on the description, to keep track.
Also, im trying to add all pending things here: #1305

Some answers to the issues you brought up:

I swapped ETH to WETH - In the sidebar the status of the transaction remains as open eventhough the transaction went through.

I will review this, it shouldn't be the case. Elena also notice an issue with wrapping, maybe is related.

To add, Approve transaction does not appear in the side bar:

Will review! definitely an issue

I reloaded the safe, and it appears that connection was lost there.

It seems lost from the other side, on the Gnosis Safe. I think is independent of this PR, and not sure how to reproduce. Probably nice to verify this, and we could open an issue if we have an issue in our side.

I feel the current flow is not so intuitive (this actually could apply for other wallets connected via WC as well)
I am missing a call for action after clicking on the "Allow Cowswap to use your Token XY"

@MareenG true, we are aware, thanks for bringing it up. Not part of the scope of this PR. @michelbio is doing it here #1338 and also part of the pending tasks in the summary issue.

anxolin · 2021-09-23T12:10:56Z

@elena-zh the issue with the approval should be fixed now

The other issue with wrapping/unwrapping won't be fixed in this PR, but will be in a future PR :)
It's all part of the plan :)

Once we have this, we will just need to monitor the safe transaction until we have a hash of the real transaction. Only with this piece of information, we will be able to tell when the tx is mined

W3stside · 2021-09-23T14:13:00Z

src/custom/state/enhancedTransactions/hooks/TransactionHooksMod.tsx

+ * @param tx to check for recency
+ */
+export function isTransactionRecent(tx: EnhancedTransactionDetails): boolean {
+  return new Date().getTime() - tx.addedTime < 86_400_000


is this a mod? if so make the magic number go away pls

Because is a mod, i would avoid to change it. But is a good point, however, is not part of this PR, plus I think Leandro touched near this in the orders PR, so I think is not worth it

alfetopito

First part, tested the behaviour, will review the code next.

Tested using a rinkeby safe with a single owner

Token enabling

Working, with caveats.
Transaction is displayed on activities panel but remains pending forever
.
When the button is clicked, there's no feedback in the UI.
Which is fine most of the time with Metamask where the feedback is in the browser but weird in this case.

Trading

Working, no issues

Wrapping

Not working.
Tried to unwrap, since I only have WETH in this account.
No feedback in the UI, just an error logged to the console:

alfetopito

No issues with the code, only the one reported about unwrapping

alfetopito · 2021-09-23T16:11:18Z

src/custom/hooks/useApproveCallback/useApproveCallbackMod.ts

+import { useTokenContract } from 'hooks/useContract'
+import { useActiveWeb3React } from 'hooks/web3'
+import { useTokenAllowance } from 'hooks/useTokenAllowance'
+import { HashType } from '@src/custom/state/enhancedTransactions/reducer'


What is the mod in this file? Only this import?
Also, path

Suggested change

import { HashType } from '@src/custom/state/enhancedTransactions/reducer'

import { HashType } from 'state/enhancedTransactions/reducer'

elena-zh · 2021-09-24T10:39:54Z

Hey @anxolin , I have retested your fix.
Approve token transaction appears in the recent activity log, however, as for the Wrap transaction, it hangs in the Open status, as the app refers to a not existing TxHash (related to Safe connection only: everything works fine for EOA wallets).
https://rinkeby.etherscan.io/tx/0x54eb77c31e67eb849775065f6958cd0418fce22178f48e0e3669db959c18f6c9

Buy/Sell orders work fine (1sig and multisig).

elena-zh · 2021-09-24T10:50:25Z

As for connection issue, I have faced it again today when I opened the PR to test: there was no connection in Safe, however, Cowswap showed me an active connection.
I created a separate issue for this case #1473

anxolin · 2021-10-07T18:47:04Z

Most of the issues described are either solved in this PR or in other PRs. I tracked them so we can a review.

Merging to consolidate all the waterfalls. Reviews are still welcomed, the idea is:

Capture the comments of pending issues in: https://docs.google.com/document/d/1_I0yPgocBqbKGnFLC1L3nvlS3nMXZcNLOPhvR8YAxi4/edit#
Aggregate all pending PRs into develop
Solve all the issues :)
Finish the Gnosis Safe / Presign tasks: [Epic] Support of Smart Contract Wallets #1305
Do testing once all is fixed and finished. Note that these functionalities required too many PRs to implement in iterations. For QA probably is too many, this is why I want to close a release next week so we can test it all merged together.

alfetopito · 2021-10-07T19:10:57Z

@anxolin can this branch be deleted?

Not doing it yet because there might be stuff pointing to it, and if I remove then those will point to develop

anxolin changed the base branch from presign/base to presign/safe-client September 10, 2021 13:39

anxolin marked this pull request as ready for review September 10, 2021 14:03

anxolin requested review from alfetopito and W3stside September 10, 2021 14:03

anxolin changed the title ~~[presign] Enhance transactions~~ [presign] Enhance transactions and refactor Sep 10, 2021

anxolin requested a review from ramirotw September 10, 2021 14:20

anxolin mentioned this pull request Sep 10, 2021

[Epic] Support of Smart Contract Wallets #1305

Closed

53 tasks

anxolin changed the base branch from presign/safe-client to presign/base September 23, 2021 09:19

W3stside reviewed Sep 23, 2021

View reviewed changes

alfetopito reviewed Sep 23, 2021

View reviewed changes

ramirotw mentioned this pull request Oct 7, 2021

Handle cancel state for ethereum txs #1532

Merged

anxolin force-pushed the presign/base branch from 0522c24 to ce007ff Compare October 7, 2021 17:06

anxolin added 7 commits October 7, 2021 18:52

Add enhancement of transactions

ebaa0e8

Fix tests

d0bdd84

Fix paths

79a7208

Export reducer

ecebcd3

Remove Gnosis Updater

748709d

Remove update safe tx action

c9f964e

Remove safeTransaction

c271efd

anxolin added 4 commits October 7, 2021 19:14

Re-add params

a93dc98

Remove safeTransaction

c890ce2

Remove safe tx

d17e456

Fix approve workflow

5b32325

anxolin force-pushed the presign/enhanced-transactions branch from 030864b to 5b32325 Compare October 7, 2021 18:23

Fix code style issues with Prettier

8922b40

anxolin merged commit 88ef399 into presign/base Oct 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[presign] Enhance transactions and refactor #1429

[presign] Enhance transactions and refactor #1429

anxolin commented Sep 10, 2021 •

edited

Loading

github-actions bot commented Sep 10, 2021

MareenG commented Sep 22, 2021

elena-zh commented Sep 22, 2021

MareenG commented Sep 22, 2021 •

edited

Loading

MareenG commented Sep 22, 2021 •

edited

Loading

MareenG commented Sep 22, 2021

anxolin commented Sep 23, 2021

anxolin commented Sep 23, 2021

W3stside Sep 23, 2021

anxolin Oct 7, 2021

alfetopito left a comment

alfetopito left a comment

alfetopito Sep 23, 2021

elena-zh commented Sep 24, 2021

elena-zh commented Sep 24, 2021

anxolin commented Oct 7, 2021

alfetopito commented Oct 7, 2021

	import { HashType } from '@src/custom/state/enhancedTransactions/reducer'
	import { HashType } from 'state/enhancedTransactions/reducer'

[presign] Enhance transactions and refactor #1429

[presign] Enhance transactions and refactor #1429

Conversation

anxolin commented Sep 10, 2021 • edited Loading

Summary

Changes / hints for reviewer

Issues detected

To Test

github-actions bot commented Sep 10, 2021

MareenG commented Sep 22, 2021

elena-zh commented Sep 22, 2021

MareenG commented Sep 22, 2021 • edited Loading

MareenG commented Sep 22, 2021 • edited Loading

MareenG commented Sep 22, 2021

anxolin commented Sep 23, 2021

anxolin commented Sep 23, 2021

W3stside Sep 23, 2021

Choose a reason for hiding this comment

anxolin Oct 7, 2021

Choose a reason for hiding this comment

alfetopito left a comment

Choose a reason for hiding this comment

Token enabling

Trading

Wrapping

alfetopito left a comment

Choose a reason for hiding this comment

alfetopito Sep 23, 2021

Choose a reason for hiding this comment

elena-zh commented Sep 24, 2021

elena-zh commented Sep 24, 2021

anxolin commented Oct 7, 2021

alfetopito commented Oct 7, 2021

anxolin commented Sep 10, 2021 •

edited

Loading

MareenG commented Sep 22, 2021 •

edited

Loading

MareenG commented Sep 22, 2021 •

edited

Loading