BitBoxSwiss / bitbox-wallet-app

The BitBoxApp for desktop and mobile.
https://bitbox.swiss/app
Apache License 2.0
251 stars 82 forks source link

wallet-connect: fix ui crash when account unspecified by dapp #2712

Closed Tomasvrba closed 3 months ago

Tomasvrba commented 4 months ago

Closes https://github.com/BitBoxSwiss/bitbox-wallet-app/issues/2689

Tomasvrba commented 4 months ago

pls review @benma @thisconnect

shonsirsha commented 4 months ago

@Tomasvrba testing on https://highlight.xyz , I can't seem to connect my wallet to begin with (although BBApp says it's successful).

Screenshot 2024-05-14 at 09 12 43

chainId not found 🤔

thisconnect commented 4 months ago

requesting review from @shonsirsha as he has more experience with WC 😇

Tomasvrba commented 4 months ago

@Tomasvrba testing on https://highlight.xyz , I can't seem to connect my wallet to begin with (although BBApp says it's successful). Screenshot 2024-05-14 at 09 12 43

chainId not found 🤔

are you on mainnet or testnet? highlight.xyz probably doesn't support testnet make sure to use make servewallet-mainnet

shonsirsha commented 4 months ago

Hm yeah I'm on mainnet.

xsaasxsax
Tomasvrba commented 4 months ago

Hm yeah I'm on mainnet.

xsaasxsax

yeah, getting the same now, strange, worked yesterday, not sure what's going on yet, the error messages from highlight are useless

thisconnect commented 4 months ago

seems like a different problem, could you add a 2nd commit to catch and some the error in the BBApp?

Tomasvrba commented 4 months ago

seems like a different problem, could you add a 2nd commit to catch and some the error in the BBApp?

I'll look into it, should have time tomorrow

Tomasvrba commented 3 months ago

@thisconnect fixed in: https://github.com/BitBoxSwiss/bitbox-wallet-app/pull/2712/commits/1aa589da7b4984a2823eaa187a92fc8306144dde

update namespace handling to conform to: https://docs.walletconnect.com/web3wallet/namespaces#case-no-1---recommended-default-for-all-apps

Until now we were relying only on requiredNamespace to match the pairing request to the correct chain and account, however, for some reason (don't ask me why, no idea), requiredNamespace is not actually required, can be empty and the necessary namespace details are instead defined in optionalNamespace FML

tested with Blur.io, opensea.io and se-sdk-dapp.vercel.app (wallet connect integration test) and all work now also tested with highlight.xyz and there's improvement, I can log in and sign the log in message, but get a 422 error "Unprocessable content" from their privy.io authentication provider, no clue at the moment what this is about, for anyone wanting to access highlight.xyz I would recommend to connect via Rabby instead

thisconnect commented 3 months ago

untested lgtm, I asked @shonsirsha to test 😇

I wonder if there could be any other WC related errors that are currently not shown to the users.. other than that looks good.

Beerosagos commented 3 months ago

Nice!! @Tomasvrba can you add a CHANGELOG entry for this? :innocent:

Tomasvrba commented 3 months ago

Nice!! @Tomasvrba can you add a CHANGELOG entry for this? 😇

done and rebased