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

Gnosis safe app #2495

Merged
merged 15 commits into from
Mar 10, 2022
Merged

Gnosis safe app #2495

merged 15 commits into from
Mar 10, 2022

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Feb 24, 2022

Summary

Basic integration with Gnosis Safe Apps

image

I just gave it a small try, and since it was a low hanging fruit because we had most of it implemented I went for it (because of our Gnosis Safe implementation using Wallet Connect)

Basically, it does the following:

  • Tries to connect with Gnosis Safe if embeded in Gnosis Safe Apps
  • Adds a special name and logo for Gnosis Safe Apps
  • Adds Gnosis Safe info to the context of the app, so it channels the workshop within the Gnosis Safe workflow

Not included

#2497 Remove disconnect and change wallet buttons

To Test

TODO: I will add instructions

  1. Go to https://gnosis-safe.io/app
  2. Add the app https://pr2495--gpswapui.review.gnosisdev.com
  3. Test to wrap, approve, trade, etc.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2022

CLA Assistant Lite:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger the CLA Action by commenting recheckcla in this Pull Request

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@anxolin anxolin marked this pull request as ready for review February 24, 2022 20:34
config.headers = {
'Access-Control-Allow-Origin': '*',
'Access-Control-Allow-Methods': 'GET',
'Access-Control-Allow-Headers': 'X-Requested-With, content-type, Authorization',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required to add CORS rules to debug locally

@@ -161,6 +161,7 @@
"scripts": {
"start:default": "craco start",
"start": "REACT_APP_DISABLE_TOKEN_WARNING=true yarn start:default",
"start:ssl": "HTTPS=true yarn start",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SSL is required to debug locally

@anxolin anxolin requested review from nenadV91, alfetopito, W3stside, fairlighteth and a team and removed request for nenadV91 February 24, 2022 20:37
@elena-zh
Copy link

I'm still testing, works great in Rinkeby, still need to run some tests in Mainnet and GC.
The only issue I have faced for now is the issue with the app background when expand the form:
bg issue

@elena-zh
Copy link

Works great!
Besides the issue I have mentioned above, I faced another one that might be related to #1965 issue: when I connected to an account that is not an owner of the safe, the app allows me somehow to press on the Swap button, so the order appears in the Activity modal in 'Signing' state. However, the transaction itself fails.
not an owner

However, I'm not sure whether we can fix this (disable Swap button when the safe in the read-only mode). If yes, please let me know if I need to create a separate issue for this.

Thanks

@anxolin
Copy link
Contributor Author

anxolin commented Feb 25, 2022

Nice catch! I will try to address this one in a different PR (read only issue). I think we can do something

#2499

@fairlighteth
Copy link
Contributor

Tested out the app with a Safe having a single owner, and also with a Safe with 2 owners. It was a very smooth user experience. Much better as compared using a WalletConnect bridge.

One thing I missed from the Safe's end, was for a way to access the transaction queue without leaving the swap page.

One comment I have, is that the background seems blurred at all times:
Screen Shot 2022-02-28 at 10 31 55

Not sure if this was intentional, but I believe we should disable this. In particular it interferes with any modal that adds another blur layer on top.

@anxolin
Copy link
Contributor Author

anxolin commented Mar 4, 2022

@michelbio , regarding

One comment I have, is that the background seems blurred at all times:

please, review #2499 to see if it fixes it.

One thing I missed from the Safe's end, was for a way to access the transaction queue without leaving the swap page.

Yes, I think, that's mostly an issue with the Gnosis Safe, but we can try to ask if theres ways to improve

@anxolin anxolin added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Mar 8, 2022
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.

Working fine. Tested placing an order on rinkeby with a 2/n Safe.

The only caveat is the known issue regarding Safe information for devices other than the one that created the order.

For example, this is what the signer that created the order sees
Screen Shot 2022-03-09 at 08 52 04

And this is the other signer sees
Screen Shot 2022-03-09 at 08 51 52

Copy link
Contributor

@fairlighteth fairlighteth left a comment

Choose a reason for hiding this comment

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

Approving based on my tests/earlier comment.

@elena-zh
Copy link

elena-zh commented Mar 9, 2022

@alfetopito , seems that the issue you have described is related to #1958 . Correct?

@W3stside
Copy link
Contributor

W3stside commented Mar 9, 2022

same issue(s) as @alfetopito otherwise looks good!

@alfetopito
Copy link
Contributor

@elena-zh yes, that's the one.
Just raising it here to be explicit we are aware of it.

export { useActiveWeb3React, useInactiveListener } from '@src/hooks/web3'
export { useInactiveListener } from '@src/hooks/web3'

export function useActiveWeb3React() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this function stayed the same as the original one, is there a reason why we re-declared it here instead of just exporting it from the original like we did before?

@anxolin
Copy link
Contributor Author

anxolin commented Mar 10, 2022

The only caveat is the known issue regarding Safe information for devices other than the one that created the order.

Yep, other signers need to go to gnosis safe and they don't get this. It would be nice to add. I think is doable just by using the safe API and gettting the latest tx for the connected safe, but anyways, not prio. Good one for a community member.

Is there an issue for this one? I would love to add a "help wanted" tag for all that have to do with the Gnosis Safe, i think is nice way to do contributions for the project

@anxolin anxolin merged commit 1f0c6ac into develop Mar 10, 2022
@alfetopito alfetopito deleted the gnosis-safe-app branch March 11, 2022 15:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants