-
Notifications
You must be signed in to change notification settings - Fork 473
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
Conversation
Branch preview✅ Deploy successful! https://off_chain_signing_tweaks--walletweb.review-wallet-web.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
const ongoingMessage = useSafeMessage(safeMessageHash) | ||
const [safeMessage, setSafeMessage] = useState(ongoingMessage) | ||
|
||
// Sync ongoing msg | ||
useEffect(() => { | ||
setSafeMessage(ongoingMessage) | ||
}, [ongoingMessage]) |
There was a problem hiding this comment.
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?
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 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 Also I've noticed that for messages the right side "Tx status" is missplaced. I'll create a new ticket to deal with it |
Here's the issue #2384 |
What it solves
Resolves #2019
How this PR fixes it
How to test it
Screenshots
Checklist