feat: add support for setSelectedAccounts
#543
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| const selectedAccounts = allAccounts.filter((account) => | ||
| accountIdSet.has(account.id), | ||
| ); | ||
|
|
There was a problem hiding this comment.
BTC snap has 2 methods for updating accounts. One is a full scan which previously was used when an account was created/discovered and the other one is a lighter synchronize which identified updates.
Since now we don't do any scanning on discovery we don't really know when it is the first time we see an account (without storing anything on snaps state).
Thus, on this PR we are going to run a full scan everytime the keyring method gets called and a synchronize on the cron job running every 30 seconds.
Feel free to add your thoughts. We can always store "seen" accounts on the snap state but I am not sure how efficient is going to be if a user creates loads of them.
orestiseth
changed the title
setSelectedAccountssetSelectedAccounts
| ]; | ||
| const request = { method: 'synchronizeAccounts' } as JsonRpcRequest; | ||
|
|
||
| it('synchronizes all accounts', async () => { |
There was a problem hiding this comment.
| it('synchronizes all accounts', async () => { | |
| it('synchronizes selected accounts', async () => { |
| }); | ||
|
|
||
| it('filters out accounts that have no transaction history', async () => { | ||
| it.skip('filters out accounts that have no transaction history', async () => { |
There was a problem hiding this comment.
So the filtering was happening when we were discovering accounts which included a full scan. Since now we don't scan the chain for activity we can't know if there were txs or not.
| } | ||
| case `${KeyringRpcMethod.SetSelectedAccounts}`: { | ||
| assert(request, SetSelectedAccountsRequestStruct); | ||
| await this.setSelectedAccounts((request as any).params.accountIds); |
There was a problem hiding this comment.
I don't think the as any is needed here.
On line 149, assert narrows the type of request, so that on line 150 request is of type inferred from the struct.
There was a problem hiding this comment.
Actually it should be await this.setSelectedAccounts(request.params.accounts);. I have the change locally but haven't pushed it yet. Will do in a bit.
| const isSubset = (first: Set<string>, second: Set<string>): boolean => { | ||
| return Array.from(first).every((element) => second.has(element)); | ||
| }; |
| await this.#accountsUseCases.fullScan(account); | ||
| }); | ||
|
|
||
| await Promise.all(scanPromises); |
There was a problem hiding this comment.
What about this?
| await Promise.all(scanPromises); | |
| await Promise.allSettled(scanPromises); |
Ensures that a failing fullScan call doesn't prevent the others to complete
| const scanPromises = selectedAccounts.map(async (account) => { | ||
| await this.#accountsUseCases.fullScan(account); | ||
| }); |
There was a problem hiding this comment.
Can simplify to:
| const scanPromises = selectedAccounts.map(async (account) => { | |
| await this.#accountsUseCases.fullScan(account); | |
| }); | |
| const scanPromises = selectedAccounts.map(async (account) => this.#accountsUseCases.fullScan(account)); |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3 participants
Note
Adds
setSelectedAccountsRPC (with validation and full scans), updates cron to synchronize only selected accounts, and bumps keyring dependencies.KeyringRpcMethod.SetSelectedAccountsinKeyringHandlerto validate IDs andfullScanselected accounts.validateSelectedAccountsinhandlers/validation.ts.synchronizetofalseincreateAccount.discoverAccountsnow returns all discovered accounts (no history filter).CronHandlernow syncs only accounts returned bygetSelectedAccounts(requiresSnapsProvider); constructor and usage updated.SnapsProviderintoCronHandlerinsrc/index.ts.fullScanduringdiscoverinAccountUseCases.discover.@metamask/keyring-apito^21.1.0and@metamask/keyring-snap-sdkto^7.1.0.snap.manifest.jsonversion to1.3.0and shasum.synchronize=false, and skip history-filter test.Written by Cursor Bugbot for commit 741cddf. This will update automatically on new commits. Configure here.