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

[Safe] Handle the case: Enough signatures, but not transaction #1694

Merged
merged 3 commits into from
Oct 27, 2021

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Oct 26, 2021

Summary

This PR tries to tackle the edge case where an user signed the transaction with enough signers, but they didn't execute the transaction.

It will be shown like this:
image

The user will need to go to Gnosis Safe and execute the transaction:
image

To Test

  1. Sign a transaction with all the required signers. The last signer should uncheck the "Execute transaction" checkbox before signing.
  2. Verify message in CowSwap
  3. Sign the transaction
  4. Verify message in CowSwap

@anxolin anxolin changed the title Safe/handle unexecuted tx [Safe] Handle the case: Enough signatures, but not transaction Oct 26, 2021
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@elena-zh
Copy link

Hey @anxolin , I reviewed changes. Great job!

I'd like to add an enhancement proposal: When an order is signed and executed, and is in 'Open' status, it would be nice stop displaying 'Need to execute' message and replace it with 'Executed' one. WDYT?
do not show when it is opened

Some minor UI issues I found, but they might be fixed by @biocom :

  1. It would be nice to change 'Need to execute' message color in the light mode. At least, make it the same with the 'Filled' status. I think
    styles

  2. It would be nice to add a margin between the message and the 'View Gnosis Safe' link
    add a padding

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.

Happy case working good.

Agree with Elena's comments on hiding the signature threshold completely when pre-sign tx executed.

@fairlighteth
Copy link
Contributor

I'd like to add an enhancement proposal: When an order is signed and executed, and is in 'Open' status, it would be nice stop displaying 'Need to execute' message and replace it with 'Executed' one. WDYT?

If all signatures are collected + the pre-sign transaction is executed, the order then can go proceed to status OPEN. In that case we don't need the 'need to execute' message anymore. I do think it's convenient to still show some brief 'history' as in, how many signatures were collected. As a nice to have.

The light mode green color is something I will fix with a waterfall PR on this one.

@anxolin
Copy link
Contributor Author

anxolin commented Oct 27, 2021

I'd like to add an enhancement proposal: When an order is signed and executed, and is in 'Open' status, it would be nice stop displaying 'Need to execute' message and replace it with 'Executed' one. WDYT?

Good idea! this happens for a delay, but your proposal is good! Will do it now

@fairlighteth
Copy link
Contributor

@elena-zh @anxolin Addressed the style issues in #1700

Also an issue I spotted when testing: Been testing a Safe (on Rinkeby) with a threshold of 2 signatures. The second confirmed signature (execution was unchecked) didn't seem to get updated on the CowSwap end. I did verify the Safe endpoint returning 2 confirmations (no execution).

@anxolin
Copy link
Contributor Author

anxolin commented Oct 27, 2021

@elena-zh I implemented your proposal! thanks for the tip

@alfetopito addressed your comments too.

@biocom thanks for the PR, I will review soon

Also an issue I spotted when testing: Been testing a Safe (on Rinkeby) with a threshold of 2 signatures. The second confirmed signature (execution was unchecked) didn't seem to get updated on the CowSwap end. I did verify the Safe endpoint returning 2 confirmations (no execution).

We can see this in a zoom after/before our sync

@anxolin anxolin changed the base branch from develop to presign/safe-swap-tx-display October 27, 2021 17:31
@anxolin
Copy link
Contributor Author

anxolin commented Oct 27, 2021

I'll do a simple merge to #1652 since I have one approval and that other PR is related and @elena-zh has reported the same issue in both places.

This way the fix to one of the issues will be already applied :)

@anxolin anxolin merged commit fa5aca8 into presign/safe-swap-tx-display Oct 27, 2021
@alfetopito alfetopito deleted the safe/handle-unexecuted-tx branch October 27, 2021 19:14
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.

4 participants