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

fix: better UX for off-chain message signing #2372

Merged
merged 5 commits into from
Aug 14, 2023
Merged

Conversation

schmanu
Copy link
Member

@schmanu schmanu commented Aug 8, 2023

What it solves

Resolves #2019

How this PR fixes it

  • Changes text
  • Adds a Success Card
  • Shows Success Card for 1s when message is fully signed
  • immediately re-fetches message after signing

How to test it

  • Open a 2/3 Safe
  • Create a message signing request through Wallet connect
  • Add the first signature
    • Observe that it gets added immediately
  • Change the owner
  • Add a second signature
    • Observe new success card with Continue button
    • It should disappear after 1s

Screenshots

Screenshot 2023-08-08 at 16 48 51

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

Branch preview

✅ Deploy successful!

https://off_chain_signing_tweaks--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

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

Report generated by eslint-plus-action

@schmanu schmanu requested a review from iamacook August 8, 2023 15:48
Comment on lines 174 to 180
const ongoingMessage = useSafeMessage(safeMessageHash)
const [safeMessage, setSafeMessage] = useState(ongoingMessage)

// Sync ongoing msg
useEffect(() => {
setSafeMessage(ongoingMessage)
}, [ongoingMessage])
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth condensing this into useSafeMessage as ongoingMessage is only used in the useEffect now. What do you think?

@francovenica
Copy link
Contributor

I've used this app to test this https://github.com/5afe/eip-1271-dapp

Have a safe with 2 owners at least and created a message. I open both owners in different browsers
Made sure that after the 1st owner signs in the form the confirmation shows shortly after (2 or 3 seconds takes for the confirmation to show up)
Signing with the other person in the other broswer takes a bit longer but it is expected
The form shows the success message for about a second and then redirects to the WC app again
The message of "Collect sigantures" was changed with the wording suggested in the main ticket and it's not visible for safes 1/x
The message was put into an accordeon block

With this the ticket is fine to be closed but Some suggestion

I think 1 second is too short, especially if other owners are signing in different browsers, if you blink you will miss it. Make it 3 sconds
Since we remove the message of "Collect signatures" for 1/x safes, I think this message should also change from
"Please keep this modal open until all signers confirm this message" to "Please keep this modal open until the message is signed"
image

Also I've noticed that for messages the right side "Tx status" is missplaced. I'll create a new ticket to deal with it
08-10-2023_1247x578(6338)

@francovenica
Copy link
Contributor

Here's the issue #2384

@katspaugh katspaugh merged commit a9a8836 into dev Aug 14, 2023
7 checks passed
@katspaugh katspaugh deleted the off-chain-signing-tweaks branch August 14, 2023 09:26
@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2023
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.

[EIP1271] Offchain signing UI tweaks
4 participants