-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: handle transactions through Safe app #1041
Conversation
I think it would make sense to have support for |
4475bdb
to
1d95a6d
Compare
How is the flow with Snapshot offchain? Would be great to have similar modals |
3288a45
to
d6ec0fa
Compare
Should be ready for review. |
Looks like this setting (onchain signatures) are old way of signing and Safe migrated to offchain signature (https://help.safe.global/en/articles/40783-what-are-signed-messages#h_ae27e5b14f). That's the reason our old implementation looked like this (we didn't wait for confirmation because the promise we get after asking for signing never resolved). I'm not sure how this worked with v1 before - I enabled this setting on v1 Snapshot and when I try to edit space settings or create proposal via my Safe it just gets stuck: Can you try it on v1 @ChaituVR ? Enable this setting as in here and see if you are able to propose. |
Both offchain and onchain signatures already work on v2 (offchain spaces) and also v1, something changed recently? i will try to test again |
hmm it works for offchain spaces if I use snapshot.box as a safe app, doesn't work with walletconnect, this one https://github.com/snapshot-labs/workflow/issues/300 |
Safe migrated to offchain sig as default but onchain sig is still possible using the option in their UI. Offchain sig require the initiator of the message to keep his Safe UI open until signers of the Safe confirm the message, this is not ideal for many Safe, I dont think onchain sig will be removed anytime soon. |
The issue is that with this option the promise never returns https://github.com/snapshot-labs/workflow/issues/300 |
@bonustrack after testing it it looks like there is nothing we can do about onchain signatures with WalletConnect on Safe, it just seems to hang for a while and eventually fail with This will also reproduce on https://react-app.walletconnect.com/ as @ChaituVR reported. This issue is also present on master (and on v1 UI), so there will be no negative result of this PR being merged (this PR will improve other UX aspects of Safe usage though). We should wait then to see what Safe responds about this issue. |
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.
tACK, let's merge this for now, if Safe answer we would implement onchain tx in another PR
573ce6e
to
6b870e9
Compare
Summary
This PR changes three things when executing actions through Safe.
Closes: https://github.com/snapshot-labs/workflow/issues/292
How to test
Screenshots
Wasn't sure about design on this screen or text to use there, so I'm open for suggestions.