Connect in bulk to accounts being restored from local storage #568
+102
−18
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related: #393
When you connect to chains, Cosmos Kit saves the accounts in local storage (in
cosmos-kit@2:core//current-wallet
). When visiting the site again and the wallet connection needs re-approval, it currently spams separate connection popups for each of the accounts stored in local storage, instead of doing a bulk connection like most wallets support. This PR aims to fix that to prevent the wallet spam.The problem
Relevant code:
cosmos-kit/packages/core/src/manager.ts
Lines 367 to 413 in 6072379
and:
cosmos-kit/packages/core/src/manager.ts
Lines 349 to 365 in 6072379
The problem with this code is that
_restoreAccounts
does the following:_reconnect
_reconnect
then:connect
on the walletconnect(sync = true)
on a single chain wallet, broadcasting to all the other chain wallets that were activated in_restoreAccounts
to connectEach chain wallet individually receives this connection sync and fires off a connect call, triggering a bunch of individual pop ups for every single account in storage.
How
useChains
bulk connect worksThe
useChains
hook knows how to connect to chains in bulk by settingbeforeConnect
andconnectChains
callbacks that intercept typical wallet connection:cosmos-kit/packages/react-lite/src/hooks/useChains.ts
Lines 31 to 55 in 6072379
That hook:
enable
all chains at onceaddChain
for every chain in case a missing chain was the reason for theenable
failureenable
againThis works in the best case scenario that enable succeeds or is missing a chain and then succeeds, but it also leads to double prompting if the user rejects the initial attempt.
The solution
I took the logic from
useChains
, made it a bit more fail-safe (since account reconnection should be a seamless background procedure that doesn't cause the user any issues), and added it to the manager's account reconnection logic.Now the manager:
activate
does but it seems necessary)enable
all stored account chains at once with a valid chain wallet / wallet repo, BAILING IMMEDIATELY if the user rejects the attemptaddChain
if theenable
failed due to any error other than a user rejectionenable
again if all theaddChain
calls succeeded, BAILING IMMEDIATELY if the user rejects the attempt_reconnect
, ONLY if the user did not reject either of theenable
attempts above, and ONLY with the chains loaded from the account storageThere is no way to predict/detect the error message when
enable
fails due to a missing chain, since wallet clients can throw whatever they want, so I think it's best case to just try toaddChain
as long as the user did not reject theenable
(which we can predict/detect).It definitely shouldn't call
_reconnect
ifenable
fails or else a user rejection will lead to the individual spam calls of each chain trying to connect again. And Ifenable
succeeds,_reconnect
will seamlessly set up the clients and sync connection state to the appropriate wallet clients.I added the
_reconnect
logic that only connects to the specific chain wallets WITHOUTsync
because I noticed a bug where if components on the page that use theuseChain
hook load first (before the manager's account restoration function runs), they activate these chain wallets, which then spams individual connection attempts for each of those other activated chains when restore runs, since they weren't in the restored accounts and thus were not part of the bulkenable
call.Questions
Am I missing anything in the chain and/or wallet and/or chain wallet client activation/setup? I'm not exactly sure what's required when it comes to all the
activate
,setData
,setState
, andconnect
calls. It seems to work as-is, so I assume the answer is no.Is the final_reconnect
still necessary? I assume yes becauseenable
just prepares the wallet itself, and then_reconnect
will callconnect
on the necessary internal clients which should seamlessly succeed since the wallet is already enabled.I have confirmed that
_reconnect
is a necessary final step.Like the
_reconnect
call at the end, should we only be running this autoconnection logic if themode
isn'twallet-connect
?