Skip to content
This repository was archived by the owner on Jun 24, 2022. It is now read-only.

[presign] Safe swap tx display #1652

Merged
merged 14 commits into from
Oct 28, 2021
Merged

[presign] Safe swap tx display #1652

merged 14 commits into from
Oct 28, 2021

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Oct 18, 2021

Summary

Edited 26/10/2021

I added a more fine grained message now.

It will show different messages depending on the state of the transaction, and the number of signers.

Checkout the messages here:
https://github.com/gnosis/cowswap/pull/1652/files#diff-f591f447e126d570bf5ffdc1f646a7826958496985659162101b05fe999b1f53R69

image

Original

Continue with #1651 to show a Gnosis Safe context also for Swaps

This PR displays the Gnosis Safe tx context for a swap (note that before this PR only approve/wrap/unwrap transactions would have this)

Also, it strikes through the label that informs about the missing signatures in the case of orders that are canceled/expired.
Screenshot at Oct 16 11-02-22

Scope

Doesn't include all the fixes for bugs reported in other PRs. We will address them independently. The scope of this PR is to display the current context on the swap for Gnosis Safe transactions.

To Test

  1. Try to swap using a Gnosis Safe with 2 signatures. Verify the whole flow
  2. Make sure that canceled and expired tx are displayed correctly. Note: we cannot prevent the user from pre-signed an expired order, we can only show that is expired. I will make some suggestions to Gnosis Safe team today asking them to improve how they show our trades

@anxolin anxolin force-pushed the presign/safe-swap-tx-info branch from 9234fdf to d63f594 Compare October 18, 2021 12:19
@anxolin anxolin force-pushed the presign/safe-swap-tx-display branch from 5ad71e0 to e80f7d2 Compare October 18, 2021 12:44
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Not sure whether this is reported somewhere else, but my order got matched before the UI realized I got both signatures - and it's still like that for the last few minutes
screenshot_2021-10-18_11-07-57

After a refresh there was no change, still says 1 signature required

@W3stside
Copy link
Contributor

same comment as @alfetopito

@elena-zh
Copy link

Changes LGTM!
Some issues I faced:

  1. Expired transaction appears to be in 'signing' state for a while when press on the 'Clear activity' button (and is never cleared out)https://watch.screencastify.com/v/I3dHRPz0Ed0vC9xld5au
  2. The issue might be related to mentioned above: I have crossed out '1 more signature required' message, however, i have signed an order by 2 owners
    I have signed
  3. I see the block about Qty of signers when there is only 1 signer needed (with '0 of 1...' message). It remains to be displayed when an order is filled
    filled 0 out of 1
  4. Just to mention as I reported a couple of weeks ago, number of signers is changed for the past orders. As a result, there is no crossed out message when an order expires
    not crossed

Thanks!

@anxolin anxolin changed the base branch from presign/safe-swap-tx-info to develop October 26, 2021 10:03
@anxolin anxolin force-pushed the presign/safe-swap-tx-display branch from 34977ff to d2a8e4f Compare October 26, 2021 10:03
@anxolin
Copy link
Contributor Author

anxolin commented Oct 26, 2021

@alfetopito

Not sure whether this is reported somewhere else, but my order got matched before the UI realized I got both signatures - and it's still like that for the last few minutes

It can happen, this comes from the fact we have two different datasources for these two things. It's possible the Safe API has some delay sometimes. I wrote it in #1305 so we can iterate and see if we can do something to improve it. One solution is to hide the signatures information if the tx is executed.

@anxolin
Copy link
Contributor Author

anxolin commented Oct 26, 2021

@elena-zh

Expired transaction appears to be in 'signing' state for a while when press on the 'Clear activity' button (and is never cleared out)https://watch.screencastify.com/v/I3dHRPz0Ed0vC9xld5au

It's a fair request, but I would not do it only because we will remove very soon the clear activity button. @alfetopito killed that in his "load orders from API" PRs

The issue might be related to mentioned above: I have crossed out '1 more signature required' message, however, i have signed an order by 2 owners

I think is related to leandro's comment. There might be a delay. We depend on Gnosis Safe reporting the signature. Eventually it should appear. Probably we could remove also the signer information if it's canceled...

I see the block about Qty of signers when there is only 1 signer needed (with '0 of 1...' message). It remains to be displayed when an order is filled

good catch. Will address this.

Just to mention as I reported a couple of weeks ago, number of signers is changed for the past orders. As a result, there is no crossed out message when an order expires

Also good catch. Will also review this one.

@anxolin anxolin marked this pull request as ready for review October 26, 2021 11:01
@elena-zh
Copy link

Hey @anxolin ,
in this PR I see changes related to the case when an order is signed, and not yet executed.

But still I see changing number of safe owners for the past/opened transactions
past and open transactions

and still see 0 of 1 signers for the case when there is only 1 signer in the safe
0 of 1
0 of 1 still

Could we show '1 of 1 signers' (or 2 of 2) message (as you implemented for the case when transaction is waiting for the execution) when an order is signed and is being executed at the moment?
I think, it might fix the last issue (0 of 1 signers).

@anxolin
Copy link
Contributor Author

anxolin commented Oct 26, 2021

in this PR I see changes related to the case when an order is signed, and not yet executed.

It shouldn't be the case. Those changes were done in this other one: #1694

All the other comments, looks like all my latest changes were not applied. I will try to reproduce...

@anxolin
Copy link
Contributor Author

anxolin commented Oct 26, 2021

In local, I see it works, but the demo URL doesn't have the latest changes

@alfetopito
Copy link
Contributor

One solution is to hide the signatures information if the tx is executed.

Yes, I like that idea. Nonce and link to the safe should be enough once tx is executed.

@elena-zh
Copy link

elena-zh commented Oct 27, 2021

Hey @anxolin , thank you! I have been able to retest the recent changes.
Flow with 2 signers looks good to me besides few issues:

  1. Currently, more than often I face this issue Order is in Open status, but 'valid to' time is less than a current one #1362. It is not a part of this PR, but it would be nice to take a look at in the near future, as I see a delay with more than 1 minute: order valid to date is in the past, but it is waiting for signatures or so.
  2. As I mentioned in the [Safe] Handle the case: Enough signatures, but not transaction #1694, It would be nice not to show 'Enough signatures' when an order is signed, and goes to the 'Open' state'. Currently, it shows that 1 signature is still required in the 'Open' status
    executed

As for cases when there is only 1 signer in the Safe, I see a message that there is no signatures yet, but I have signed and executed the order
no yet

And, this flow seems to be a bit complicated:

  1. I see that an order is signing with no Gnosis safe block
  2. then I see the order is still in signing state along with a message that there is no signatures in the Gnosis safe block
  3. Then an order becomes in 'Open' status, message that there is no signatures is still there
  4. And then an order is Expried/confirmed

I'd suggest to make it a bit easier:

  1. The order is in 'Signing' state when an order is signed and transaction is being executed at the moment, no Gnosis safe block
  2. The order is in 'Open' state with the message that no signatures required: when the order is executed
  3. And then an order is Expired/confirmed
    WDYT?

@anxolin
Copy link
Contributor Author

anxolin commented Oct 27, 2021

  1. Currently, more than often I face this issue Order is in Open status, but 'valid to' time is less than a current one Order is in Open status, but 'valid to' time is less than a current one #1362. It is not a part of this PR, but it would be nice to take a look at in the near future, as I see a delay with more than
    1 minute: order valid to date is in the past, but it is waiting for signatures or so.

Okay, yes not from this PR. Taking note, will handle that, although we don't have capacity for that one right now. Thanks for bringing it up anyways.

  1. As I mentioned in the [Safe] Handle the case: Enough signatures, but not transaction [Safe] Handle the case: Enough signatures, but not transaction #1694, It would be nice not to show 'Enough signatures' when an order is signed, and goes to the 'Open' state'. Currently, it shows that 1 signature is still required in the 'Open' status

This was handled in that PR

As for cases when there is only 1 signer in the Safe, I see a message that there is no signatures yet, but I have signed and executed the order

Same case I believe

And, this flow seems to be a bit complicated:

Agree, similar to what was implemented in the other PR.

[Safe] Handle the case: Enough signatures, but not transaction
@anxolin
Copy link
Contributor Author

anxolin commented Oct 27, 2021

@elena-zh I merged #1694 so this PR should already contain the fix to your (2.) issue

@alfetopito
Copy link
Contributor

Good, but an order I placed the order day is still stuck on needing one more signature even though it has been executed.

Because of local cache I assume?
screenshot_2021-10-27_14-11-45

@anxolin
Copy link
Contributor Author

anxolin commented Oct 28, 2021

Good, but an order I placed the order day is still stuck on needing one more signature even though it has been executed.

Can you hard refresh and let me know if it was fixed? that's weird.

I just tried an for me it works...

Screenshot at Oct 28 10-52-47

Screenshot at Oct 28 10-52-59

@fairlighteth
Copy link
Contributor

@anxolin could @alfetopito 's reported issue be similar to the one I had? In my case it seemed to not update the signatures/execution because of having a backlog of pending orders on my Rinkeby Safe.

@elena-zh
Copy link

hey @anxolin , I have just retested all the changes. and they look good to me!
The only nitpick I can mention is that it would be nice to alight section with enough number of signers to all elements in 1 vertical line
align

@anxolin
Copy link
Contributor Author

anxolin commented Oct 28, 2021

@anxolin could @alfetopito 's reported issue be similar to the one I had? In my case it seemed to not update the signatures/execution because of having a backlog of pending orders on my Rinkeby Safe.

@michelbio , by seeing his screenshot, i feel he was using an outdated UI, cause his order is FILLED and still shows the orders signature information, but it should look like my screenshot.

@anxolin
Copy link
Contributor Author

anxolin commented Oct 28, 2021

hey @anxolin , I have just retested all the changes. and they look good to me!
The only nitpick I can mention is that it would be nice to alight section with enough number of signers to all elements in 1 vertical line

@elena-zh , @biocom did some fix to the styles and should be in this branch... however I see the changes were not applied. there must be an issue with the auto-deployment. I will see if spot the issue, this is very inconvenient

@anxolin
Copy link
Contributor Author

anxolin commented Oct 28, 2021

I will merge to develop this one, to not waterfall changes. @biocom will review why the merge of his PR didn't apply all the changes and address any possible issue in develop directly

If anyone has new concerns, I will address in a new PR to not complicate further this one

@anxolin anxolin merged commit 2218878 into develop Oct 28, 2021
@alfetopito alfetopito deleted the presign/safe-swap-tx-display branch October 28, 2021 21:03
@alfetopito
Copy link
Contributor

@anxolin could @alfetopito 's reported issue be similar to the one I had? In my case it seemed to not update the signatures/execution because of having a backlog of pending orders on my Rinkeby Safe.

@michelbio , by seeing his screenshot, i feel he was using an outdated UI, cause his order is FILLED and still shows the orders signature information, but it should look like my screenshot.

Might be, but I can't reproduce anymore in this PR because of the 24h history limitation 😅

nenadV91 pushed a commit that referenced this pull request Nov 15, 2021
* handle insufficient input amount error in mint/hooks

* use error.isInsufficientInputAmountError

* fixed typo
nenadV91 pushed a commit that referenced this pull request Nov 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants