wallet-connect: fix ui crash when account unspecified by dapp #2712
Conversation
|
pls review @benma @thisconnect |
Tomasvrba
force-pushed
the
wc-unspecified-account
branch
from
6e1d47f
to
1296629
Compare
|
@Tomasvrba testing on https://highlight.xyz , I can't seem to connect my wallet to begin with (although BBApp says it's successful). chainId not found 🤔 |
|
requesting review from @shonsirsha as he has more experience with WC 😇 |
are you on mainnet or testnet? highlight.xyz probably doesn't support testnet |
|
Hm yeah I'm on mainnet. 
|
yeah, getting the same now, strange, worked yesterday, not sure what's going on yet, the error messages from highlight are useless |
|
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 |
|
@thisconnect fixed in: 1aa589d update namespace handling to conform to: Until now we were relying only on requiredNamespace to match the tested with Blur.io, opensea.io and se-sdk-dapp.vercel.app (wallet connect integration test) and all work now |
|
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. |
| const chains: string[] = eipList.flatMap(proposal => | ||
| proposal.chains ? proposal.chains.filter(chain => Object.keys(SUPPORTED_CHAINS).includes(chain)) : [] | ||
| proposal.chains ? proposal.chains.filter(chain => Object.keys(SUPPORTED_CHAINS).includes(chain)) : ['eip155:1'] |
There was a problem hiding this comment.
👍 looks good, maybe one day we need to define the chain id as a constant (e.g eip155:1 to be ETHEREUM_MAINNET) just for clarity. But fine for now as it's only used once this way here.
There was a problem hiding this comment.
LGTM 👍
Thanks for already testing on several platforms
Further tested on:
- https://app.uniswap.org/
- https://react-app.walletconnect.com/ (tested various methods)
- and https://highlight.xyz -> able to connect and sign but failed with a
422error (as described by @Tomasvrba).
frontends/web/src/routes/account/walletconnect/components/incoming-pairing/incoming-pairing.tsx
Outdated
Show resolved
Hide resolved
frontends/web/src/routes/account/walletconnect/components/incoming-pairing/incoming-pairing.tsx
Show resolved
Hide resolved
Tomasvrba
force-pushed
the
wc-unspecified-account
branch
from
1aa589d
to
c859dcf
Compare
|
Nice!! @Tomasvrba can you add a CHANGELOG entry for this? 😇 |
some dapps may not populate Wallet Connect's Session Namespaces with data about which account they are connecting to while this is not recommended behavior, it is up to the wallet software to handle (as per wallet connect namespace documentation) since the user can have multiple accounts in the bitbox app, we rely on the namespaces to show which one of these accounts the WC dapp session is connected to, however, the session should work even in absence of this information so we show "Unspecified account" instead when the dapp doesn't provide this session information
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, requiredNamespace is not actually required, can be empty and the necessary namespace details are instead defined in optionalNamespace
Tomasvrba
force-pushed
the
wc-unspecified-account
branch
from
c859dcf
to
4956846
Compare
done and rebased |
Closes #2689