Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper rejections #356

Closed
tschubotz opened this issue Sep 9, 2020 · 19 comments
Closed

Proper rejections #356

tschubotz opened this issue Sep 9, 2020 · 19 comments
Assignees
Labels

Comments

@tschubotz
Copy link
Member

tschubotz commented Sep 9, 2020

What is this feature about? (1 sentence)

This ensures that rejections can be properly created and confirmed with the migration to the uniform tx list view (#353)

Why is it needed? What is the value? For whom do we build it?

  • Transactions with the same nonce are competing - only 1 can be executed.
  • In order to “reject” a transactions, another transaction needs to be executed with the same nonce, thereby overwriting it.
  • Cancel transactions are technically just empty transactions with the same nonce.
  • On mobile:
    • No way to reject a tx, currently.
    • In the tx list, rejections aren't marked as such, but they are empty contract interactions.
  • On web:
    • Cancel txs not marked explicitly

High-level overview of the feature

  • User can reject transactions on mobile.
  • "rejecting" means creating a cancel tx. We decided to be explicit about what's happening under the hood.
  • Users can confirm such cancel tx.
  • Users can open / view a cancel tx.
  • Web and mobile should behave similar.

Misc

@lukasschor
Copy link
Member

@tschubotz tschubotz removed the needs kickoff 🥾 Needs a proper kickoff label Jan 21, 2021
@posthnikova
Copy link

Changes after kickoff meeting
Web: https://invis.io/6Z100KGTWEFU (clickable)
Mobile: https://invis.io/QF100KZGEMV4 (clickable)

What's been done:
Web:
• Advanced parameters – you can't change safeTxGas while creating a cancellation.
• Rename 'cancelling transaction' to 'on-chain cancellation'.
• Change text on empty queue to 'Transactions waiting for confirmation or execution will appear here' (after user tests).
• Add a link to help article 'Why do I need to pay for cancelling a tx?' (after user tests).

Mobile:
• Same as web.
• Don't show advanced paratemers because it's not possible to change them and there is no execution on mobile anyway.

I don't really like the buttons on mobile in this version, mainly because on iOS there are too much buttons.

image.png

So I made some alternative versions.

image.png

@tschubotz
Copy link
Member Author

tschubotz commented Jan 25, 2021

Thanks for these prototypes and also the different options! That helps me thinking through this a lot!

I see the problem with the many buttons that you mentioned. But considering the alternatives, I think it's still the best options because:

  • Buttons aren't hidden like e.g. behind the 3 dots.
  • Cancel and confirm sufficiently big and far enough away from each other to prevent accidental wrong clicks.

What would happen if for a tx there is already a cancellation created, would we still show the cancel button or not?

@lukasschor
Copy link
Member

Transactions waiting for confirmation or execution will appear here' (after user tests)

I was thinking about this just the other day. So thought this might be a good place to mention this:

For me, this description is a bit confusing, same for the "Collectibles will appear here". Because it is written in a passive way and makes it sound like I (as a user) don't have to do anything and they just "will appear".

So it can be odd that collectibles will appear there even though I do not plan to receive any collectibles. I would expect much more of a factual description such as "This Safe has no collectibles" or "There are no transactions in the queue".

Don't think it's critical at all though, just my subjective feeling. :)

@MouazAlzahabi
Copy link

Regarding action buttons,

How about the first design we had, when we put the actions at the bottom of confirmations section ?

@posthnikova
Copy link

What would happen if for a tx there is already a cancellation created, would we still show the cancel
button or not?

@tschubotz On the web I'm for disabling the action the user just performed and showing it until the tx status changes. So the user sees which actions are available and which they already performed.

image

I think for mobile it also makes sense, you see that you can no longer cancel but you can still confirm.

image

@lukasschor I agree, what you proposed sounds better.

@MouazAlzahabi Not sure which first design you are talking about, do you mean this screen? https://prnt.sc/sesukw

@MouazAlzahabi
Copy link

@posthnikova not that one,

I remember that first we make the confirm/reject buttons in the confirmations sections, almost same style as confirmation row but clickable, I don't know were I can find it

@posthnikova
Copy link

@MouazAlzahabi You are probably referring to the old timeline on the web.

image

@posthnikova
Copy link

Changes after quick user tests

Web: https://invis.io/6Z100KGTWEFU
Mobile: https://invis.io/QF100KZGEMV4

What's been done:

  1. 'On-chain cancellation', 'cancel' renamed to 'on-chain rejection', 'reject'.

  2. After a cancellation has been created on mobile the user is directed to the queue.

  3. Added a red label to the on-chain cancellation in the queue. When the cancellation is successful, the red label disappears because we no longer need to draw attention to this tx.

image.png

I had more versions but they require more layout changes.

image.png

  1. Cancel popup – add an explanation that creating a useful transaction is possible for advanced users (which results in even more text 🙄). There will be a link in the article 'Why do I need to pay ... ?' explaining how do do this.

  2. Removed 'Imported owner key' on mobile (out of scope of Signing on mobile v2).

@KristinaMayman
Copy link
Member

Thanks for posting these! I think these should help but I'm curious what the users will say. I fear the increased amount of text will get some negative feedback...

Do you think it would also make sense to add the red label to the transaction detail screen on mobile? Since that's one of the screens where users got confused by the amount of green for a rejection transaction.

@jpalvarezl
Copy link

Maybe it is still an open question, but now that we only have executed transaction in the "History" tab, should we think about how we display the "rejection" transaction when they are finally executed? The "rejected" transaction wouldn't show in the list in the current state of the app and I believe the "rejection" transaction will show up just as a "Custom" transaction.

@rmeissner
Copy link
Member

because we no longer need to draw attention to this tx.

Could you say why we need to draw attention to the cancelation transaction?

Question that come to my mind related to that: We have it already in a block with the other transaction, why is this not enough? When you have 2 conflicting transaction without a cancelation transaction, do we need to draw more attention to this too? The users mentioned that it is weird that a cancelation transaction is all "green" in the details (if I recall correctly), should we add more "red" there too?

@posthnikova
Copy link

Do you think it would also make sense to add the red label to the transaction detail screen on mobile?
Since that's one of the screens where users got confused by the amount of green for a rejection
transaction.

@KristinaMayman The comment about tx details referred to the confirmations timeline. This person said referring to the timeline this is not a regular tx so he expected smth yellow or orange. He said he expected cancellation to be more prominent. So I would probably say 'On-chain rejection created' or 'initiated' in the timeline.

image

Could you say why we need to draw attention to the cancelation transaction?

@rmeissner One person said that the cancellation is not a regular tx so he expected smth yellow or orange. He said he expected cancellation to be more prominent (he was referring to the timeline). Another person expected the word cancellation to be red in the queue.
So we need to highlight the cancellation in the details and in the queue to show it's not a regular transaction.

We have it already in a block with the other transaction, why is this not enough?

My impression was that people didn't quite understand the relation between the original tx and cancelling tx. They both look like regular transactions. Red is already associated with rejecting a transaction in the web interface so adding red would indicate that smth is being cancelled.

When you have 2 conflicting transaction without a cancelation transaction, do we need to draw more
attention to this too?

No. We tested this UI and it performed ok.

The users mentioned that it is weird that a cancelation transaction is all "green" in the details (if I recall
correctly), should we add more "red" there too?

Yes, see above.

The "rejected" transaction wouldn't show in the list in the current state of the app and I believe the
"rejection" transaction will show up just as a "Custom" transaction.

@jpalvarezl For past rejections I remember we agreed it's ok to show them as contract interactions. If future rejections will show as contract interactions it would be hard for the user to understand what's happening.

@jpalvarezl
Copy link

@jpalvarezl For past rejections I remember we agreed it's ok to show them as contract interactions. If future rejections will show as contract interactions it would be hard for the user to understand what's happening.

Sounds good! Thanks for clarifying!

@MouazAlzahabi
Copy link

  • I assume that cancellation tx should be at the top of next tx section as it's last created one, not Is there a reason why we put it at the end?

  • if I understand correctly the cancelation tx wouldn't be on chain until it get executed, so the "On-chain" wording is little bit confusing for me

@posthnikova
Copy link

I assume that cancellation tx should be at the top of next tx section as it's last created one, not Is there
a reason why we put it at the end?

@MouazAlzahabi Inside the group transactions are sorted by creation date. Cancellation was created later so it's lower in the list. In the queue the order is ascending.

@posthnikova
Copy link

Changes after quick user tests

Web: https://invis.io/6Z100KGTWEFU
iOS: https://invis.io/QF100KZGEMV4
Android: https://invis.io/4Q104YQ2XG6J

What's been done:

  1. Tx type icon changed to red cross in a circle.
  2. Red label in the queue changed to option 4 from earlier proposal.
  3. Rename 'awaiting' to 'needs' after voting in Slack.

Open question: where does loading happen? I assume after you tap 'Reject transaction' you are redirected to the queue and there's a loading indicator there.

image

@liliya-soroka
Copy link
Member

liliya-soroka commented Mar 1, 2021

Waiting for the web app fix for cancellation txs:

  1. two cancellation txs
  2. problem with gas limit in rejection tx
  3. problem with tx execution of recived confirmation > required confirmation

waiting for back-end task to be implemented:
5afe/safe-client-gateway#310

@liliya-soroka
Copy link
Member

Verified
all majour mobile app, web app and back-end issues are fixed

ios app - 2.13.0 (366)
android - 2.13.0 -2346rc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants