fixes: eip-1193-provider should only return ethereum wallet accounts #1044
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c0fa69c:
|
andrewkmin
changed the title
andrewkmin
force-pushed
the
andrew/eip-1193-fixes-rebased-2
branch
from
4599aac to
6e9ff45
Compare
andrewkmin
force-pushed
the
andrew/eip-1193-fixes-rebased-2
branch
from
6e9ff45 to
c0fa69c
Compare
| client: turnkeyClient, | ||
| }); | ||
| setConnected(true, { chainId: activeChain.chainId }); | ||
|
|
|
|
||
| addedChains.push({ ...chain, connected: true }); | ||
| // Only add if it hasn't been added already | ||
| if (!addedChains.some((c) => c.chainId === rpcChainId)) { |
There was a problem hiding this comment.
not too familiar with the flow here but normally with eip-1193 providers they prompt the user to approve the action of adding a chain since this could potentially be done silently with malicous intent
wondering why we don't do the same here? Or if someone wanted to use this provider and do that, could they easily?
There was a problem hiding this comment.
I think this piece of code executes once a wallet has emitted a wallet_addEthereumChain action, meaning the end-user has been prompted and has approved this
we were just seeing slightly buggy behavior; namely, someone was seeing the same chain multiple times in their chain list, and that's what we're trying to prevent here
btw: addedChains can probably be a hashmap for speed but that's a micro optimization that we don't have to make rn
| ) => { | ||
| const { rpcUrls, blockExplorerUrls, chainId, nativeCurrency } = chain; | ||
|
|
||
| if (addedChains.some((c) => c.chainId === chainId)) { |
There was a problem hiding this comment.
can't we just remove this?
this is opinionated, but IMO if someone passes a chainId that already exists, but with an invalid RPC, we should throw an error about the RPC being invalid
right now, it'll silently succeeds without any action
There was a problem hiding this comment.
that's a good point; will note down as a followup
Summary & Motivation
$title
How I Tested These Changes
via CSB
Did you add a changeset?
yes!
If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run
pnpm changeset.pnpm changesetwill generate a file where you should write a human friendly message about the changes. Note how this (example) includes the package name (should be auto added by the command) along with the type of semver change (major.minor.patch) (which you should set).These changes will be used at release time to determine what packages to publish and how to bump their version. For more context see this comment.