Skip to content
This repository has been archived by the owner on Sep 6, 2023. It is now read-only.

fix: Update WalletConnectConnector to handle individual namespaces like eip155:* #391

Closed
wants to merge 4 commits into from

Conversation

xzilja
Copy link
Contributor

@xzilja xzilja commented Jun 30, 2023

Description

Up until now WalletConnectConnector was set up to handle namespaces that included all data under global eip155 key i.e.

{
  "namespaces": {
    "eip155": {
      "chains": ["eip155:14", "eip155:19"],
      "methods": ["eth_sendTransaction", "eth_signTransaction", "personal_sign"],
      "events": ["accountsChanged", "chainChanged"],
      "accounts": ["eip155:14:0xab16a96d359ec26a11e2c2b3d8f8b8942d5bfcdb", "eip155:19:0xab16a96d359ec26a11e2c2b3d8f8b8942d5bfcdb"]
    },
  }
}

This worked as that's how majority of wallets implement the spec, however with v1 sunset we saw few wallets that return namespaces in another format (also valid)

{
  "namespaces": {
    "eip155:14": {
      "methods": ["eth_sendTransaction", "eth_signTransaction", "personal_sign"],
      "events": ["accountsChanged", "chainChanged"],
      "accounts": ["eip155:14:0xab16a96d359ec26a11e2c2b3d8f8b8942d5bfcdb"]
    },
    "eip155:19": {
      "methods": ["eth_sendTransaction", "eth_signTransaction", "personal_sign"],
      "events": ["accountsChanged", "chainChanged"],
      "accounts": ["eip155:19:0xab16a96d359ec26a11e2c2b3d8f8b8942d5bfcdb"]
    },
  }
}

Plus, there can be a combination of both formats returned. I updated connector to handle both when checking namespace chainId's and methods for validation.

Additional Information

Your ENS/address: asimetriq.eth

@changeset-bot
Copy link

changeset-bot bot commented Jun 30, 2023

🦋 Changeset detected

Latest commit: 0714209

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@wagmi/connectors Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@xzilja xzilja requested review from jxom and tmm June 30, 2023 10:04
@github-actions
Copy link
Contributor

github-actions bot commented Jun 30, 2023

Size Change: +46 B (0%)

Total Size: 52.1 kB

Filename Size Change
packages/connectors/dist/safe.js 2.72 kB +46 B (+2%)
ℹ️ View Unchanged
Filename Size Change
packages/chains/dist 8.94 kB 0 B
packages/chains/dist/index.d.ts 9.3 kB 0 B
packages/chains/dist/index.js 8.9 kB 0 B
packages/chains/dist/index.mjs 1.34 kB 0 B
packages/connectors/dist 198 B 0 B
packages/connectors/dist/base-2dd7facf.d.ts 2.69 kB 0 B
packages/connectors/dist/chunk-OQILYQDO.js 784 B 0 B
packages/connectors/dist/chunk-QRUHVNWK.js 292 B 0 B
packages/connectors/dist/chunk-QYMCVNHT.js 727 B 0 B
packages/connectors/dist/chunk-ZCAPXGBX.js 1.8 kB 0 B
packages/connectors/dist/coinbaseWallet.d.ts 311 B 0 B
packages/connectors/dist/coinbaseWallet.js 165 B 0 B
packages/connectors/dist/index.d.ts 968 B 0 B
packages/connectors/dist/index.js 119 B 0 B
packages/connectors/dist/injected.d.ts 758 B 0 B
packages/connectors/dist/injected.js 1.99 kB 0 B
packages/connectors/dist/ledger.d.ts 516 B 0 B
packages/connectors/dist/ledger.js 1.28 kB 0 B
packages/connectors/dist/metaMask.d.ts 0 B 0 B 🆕
packages/connectors/dist/metaMask.js 806 B 0 B
packages/connectors/dist/mock 1.62 kB 0 B
packages/connectors/dist/mock/index.d.ts 737 B 0 B
packages/connectors/dist/mock/index.js 1.17 kB 0 B
packages/connectors/dist/safe.d.ts 1.65 kB 0 B
packages/connectors/dist/walletConnect.d.ts 506 B 0 B
packages/connectors/dist/walletConnect.js 1.76 kB 0 B
packages/connectors/dist/walletConnectLegacy.d.ts 0 B 0 B 🆕
packages/connectors/dist/walletConnectLegacy.js 0 B 0 B 🆕

compressed-size-action

@@ -26,6 +26,7 @@
"@safe-global/safe-apps-provider": "^0.17.1",
"@safe-global/safe-apps-sdk": "^8.0.0",
"@walletconnect/ethereum-provider": "2.8.6",
"@walletconnect/utils": "2.8.6",
Copy link
Member

Choose a reason for hiding this comment

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

Can we add sideEffects: false to @walletconnect/utils#package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created pr for this, but not sure if we will land it this week
WalletConnect/walletconnect-monorepo#2910

Copy link
Collaborator

Choose a reason for hiding this comment

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

@0xasimetriq aren't the utils pre-bundled with ethereum-provider? with yarn I'm sure it works without specifying the package, not sure about pnpm tho

@midotrinh1211
Copy link

This PR seems to be fixing for this bug report:
#392

Pls review/merge it asap

Thanks

@xzilja
Copy link
Contributor Author

xzilja commented Jul 5, 2023

Pr is out of sync with actual branch will close and reopen to try and get latest updates. Looks like gh bug

@xzilja xzilja closed this Jul 5, 2023
@midotrinh1211
Copy link

@0xasimetriq
Your demo site still does not work with message-signing on Bifrost. But it works fine on mobile Metamask.
https://wagmi.sh/examples/sign-message
image

@midotrinh1211
Copy link

@0xasimetriq
For the wagmi versions after the version 1.3.4, for example version 1.3.5 or 1.3.7, we here even encounter the below error when starting the dApp:
error - Error: useConfigmust be used withinWagmiConfig``

The error is related to the usage of the hook useNetwork

Pls notice that this error does not occur for wagmi version 1.3.4

@xzilja
Copy link
Contributor Author

xzilja commented Jul 5, 2023

@midotrinh1211

  1. Maybe website wasn't updated yet or there is an issue with Bifrost
  2. Make sure you use wagmi hooks inside <WagmiConfig /> component with correct setup

@midotrinh1211
Copy link

@midotrinh1211

  1. Maybe website wasn't updated yet or there is an issue with Bifrost
  2. Make sure you use wagmi hooks inside <WagmiConfig /> component with correct setup

@0xasimetriq
Thanks a lot for your reply.

Regarding point #1, if possible, pls discuss directly with Bifrost as it is the main mobile wallet on Songbird/Flare blockchain networks.

Regarding point #2, that error does not occur for wagmi version 1.3.4. The error occurs only for versions 1.3.5 and 1.3.7
Looking forward to your further reply on this.

@midotrinh1211
Copy link

midotrinh1211 commented Jul 10, 2023

@0xasimetriq
With the latest version of 1.3.8, still same issue as reported above.
I'd suggest to re-open this PR

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.

4 participants