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

Add Gnosis Safe connector #94

Merged
merged 4 commits into from
Nov 5, 2022
Merged

Add Gnosis Safe connector #94

merged 4 commits into from
Nov 5, 2022

Conversation

@netlify
Copy link

netlify bot commented Oct 27, 2022

Deploy Preview for vue-dapp-docs ready!

Name Link
🔨 Latest commit 59d5e26
🔍 Latest deploy log https://app.netlify.com/sites/vue-dapp-docs/deploys/6361dddc5e108800083f4e30
😎 Deploy Preview https://deploy-preview-94--vue-dapp-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@johnson86tw johnson86tw temporarily deployed to release October 27, 2022 13:12 Inactive
@netlify
Copy link

netlify bot commented Oct 27, 2022

Deploy Preview for vue-dapp ready!

Name Link
🔨 Latest commit 59d5e26
🔍 Latest deploy log https://app.netlify.com/sites/vue-dapp/deploys/6361dddccb8527000a9debd2
😎 Deploy Preview https://deploy-preview-94--vue-dapp.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@johnson86tw johnson86tw temporarily deployed to release October 29, 2022 08:43 Inactive
@johnson86tw johnson86tw temporarily deployed to release October 29, 2022 08:43 Inactive
@johnson86tw
Copy link
Member Author

johnson86tw commented Oct 29, 2022

@re2005 Please help me review the code and feel free to give me any advice to improve it, thanks

} from './errors'

const __IS_SERVER__ = typeof window === 'undefined'
const __IS_IFRAME__ = !__IS_SERVER__ && window?.parent !== window
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice to export these flags, so users can use it on their apps to conditionally include the connector or NOT.

Like inside GnosisSafe, the only connector you should use if the Safe, the other won't work.

For example in my dapo I'm doing this:

const isServer = typeof window === 'undefined';
const isIframe = !isServer && window?.parent !== window;

const connectors: any = [
    new MetaMaskConnector({
        appUrl: window.location.href,
    }),
    new WalletConnectConnector({
        qrcode: true,
        rpc: Object.values(supportedChains).reduce((acc, cur) => ({ ...acc, [cur.chainId]: cur.rpcUrls[0] }), {}),
    }),
];

if (isIframe) {
    connectors.push(new SafeConnector({}));
}

Maybe is nice if I can import from 'vue-dapp`

import {isGnosisSafe} from 'vue-dapp'

I believe users shouldn't show other connectors in here, only GnosisSafe

Screenshot 2022-10-29 at 12 27 51

@re2005
Copy link
Collaborator

re2005 commented Oct 29, 2022

Somehow the Gnosis is missing the manifest. Perhaps it should be linked on the index.html of the demo.

missing-manifest

Could try adding it ?

@re2005
Copy link
Collaborator

re2005 commented Oct 29, 2022

Overall it's looking good. Very nice feature to have.

Later this weekend I'll test within my own dapps and post here more feedbacks.

Nice work @chnejohnson!

@johnson86tw johnson86tw temporarily deployed to release November 2, 2022 03:02 Inactive
@johnson86tw johnson86tw temporarily deployed to release November 2, 2022 03:02 Inactive
@johnson86tw
Copy link
Member Author

Thanks @re2005 for your feedback!

In the commit 59d5e26, isNotSafeApp only check whether it's in the iframe to detect Safe app. So it's possible that the app is in the iframe but not inside Gnosis Safe app.

if (!isNotSafeApp()) {
  const safe = new SafeConnector()
  connectors = [safe, connectors[0]]
}

A strict way is to use safe.isSafeApp() to detect the Safe app.
But to check whether it's exactly a safe app, it has to call a Promise, and it will lead to some bugs happening in the Board.vue's props.connectors

const safe = new SafeConnector()
const isSafeApp = await safe.isSafeApp()
if (isSafeApp) {
  connectors = [safe, connectors[0]]
}

At the moment, I haven't found a good solution to solve this. So just implement the first one for the feature of conditional connector inclusion.

@johnson86tw
Copy link
Member Author

Besides, I added the manifest link but it seems that it still cannot be detected on Netlify preview site.
Maybe the formal site can be detected.

截圖 2022-11-02 上午11 24 12

截圖 2022-11-02 上午11 24 29

@re2005
Copy link
Collaborator

re2005 commented Nov 4, 2022

Hi @chnejohnson thanks for the update. I've tested not only the demo, but also on my application. It all looks good, including the metadata.

I believe you can move on and merge it.

Great job!

@johnson86tw johnson86tw merged commit 9bc3172 into main Nov 5, 2022
@github-actions
Copy link

github-actions bot commented Nov 5, 2022

🎉 This PR is included in version 0.6.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@johnson86tw
Copy link
Member Author

@re2005 glad to hear that, thanks for your review!

@johnson86tw
Copy link
Member Author

johnson86tw commented Nov 14, 2022

In the commit 59d5e26, isNotSafeApp only check whether it's in the iframe to detect Safe app. So it's possible that the app is in the iframe but not inside Gnosis Safe app.

if (!isNotSafeApp()) {
  const safe = new SafeConnector()
  connectors = [safe, connectors[0]]
}

A strict way is to use safe.isSafeApp() to detect the Safe app. But to check whether it's exactly a safe app, it has to call a Promise, and it will lead to some bugs happening in the Board.vue's props.connectors

const safe = new SafeConnector()
const isSafeApp = await safe.isSafeApp()
if (isSafeApp) {
  connectors = [safe, connectors[0]]
}

At the moment, I haven't found a good solution to solve this. So just implement the first one for the feature of conditional connector inclusion.

I found a solution to detect the Safe app in a strict way.

const connectorsCreated = ref(false)

onMounted(async () => {
  const safe = new SafeConnector()
  if (await safe.isSafeApp()) {
    connectors = [safe]
  }
  connectorsCreated.value = true
})
 <vd-board v-if="connectorsCreated" :connectors="connectors" dark>

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

Successfully merging this pull request may close these issues.

None yet

2 participants