WalletScanner skips accounts with createdAt until createdAt sequence is reached #5652
Conversation
danield9tqh
changed the title
danield9tqh
changed the title
| private scanningAccounts = new Array<{ account: Account; scanFrom: HeadValue | null }>() | ||
| private scanningAccounts = new Array<{ | ||
| account: Account | ||
| scanFrom: { head: HeadValue | null } | null |
There was a problem hiding this comment.
The logic in this module is tricky sometimes. I haven't read the changes yet, but I know I will have a lot of trouble differentiating between scanFrom === null and scanFrom.head === null. Can we find a way to make the meaning of the values more intuitive? Maybe something like this (if it makes sense):
scanFrom: { head: HeadValue } | 'always-scan' | 'something-else'
There was a problem hiding this comment.
Yea I think that's a good idea. you're right that's the main thing I'm trying to differentiate with the change though
danield9tqh
force-pushed
the
dont-scan-empty-accounts-with-birthday
branch
from
79100c2
to
6c2f87a
Compare
| if (account.createdAt === null && blockHeader.sequence === GENESIS_BLOCK_SEQUENCE) { | ||
| candidate.scanFrom = 'always-scan' | ||
| } | ||
|
|
||
| if (account.createdAt && blockHeader.sequence >= account.createdAt.sequence) { | ||
| candidate.scanFrom = 'always-scan' | ||
| } |
There was a problem hiding this comment.
Nit: I think this can be simplified as:
const createdAtSequence = account.createdAt?.sequence ?? GENESIS_BLOCK_SEQUENCE
if (blockHeader.sequence >= createdAtSequence) {
candidate.scanFrom = 'always-scan'
}
| for (const { scanFrom } of this.scanningAccounts) { | ||
| if (scanFrom === 'always-scan') { | ||
| continue | ||
| } |
There was a problem hiding this comment.
I don't think that this will have any effect at the moment. We only call getEarliestHead right after calling refreshScanningAccounts, which will set scanFrom back to the head for each account.
| const decryptAndConnectAccounts = this.scanningAccounts | ||
| .filter(({ scanFrom }) => scanFrom === 'always-scan') | ||
| .map(({ account }) => account) | ||
|
|
There was a problem hiding this comment.
It seems like we use 'always-scan' as a way to to divide scanningAccounts into two lists: one list that the walletScanner is currently scanning for (the always-scan accounts), and a list of accounts that are waiting for the walletScanner to get to them.
I wonder if it would be easier to keep the accounts separate or if it would make it harder to follow?
There was a problem hiding this comment.
Might be easier but popping and pushing the accounts across arrays would be more verbose. I can look into something like this though
Summary
When an account has the following
createdAt: 10
head: null
the wallet scanner should not take any action on this account until it reaches block 10. Currently the wallet scanner will still connect blocks 1-10 (without decrypting transactions) but it still takes some time and it doesn't provide any value over just connecting block 10 directly
Testing Plan
Unit tests + running local wallet tests
Documentation
Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference)? If yes, link a
related documentation pull request for the website.
Breaking Change
Is this a breaking change? If yes, add notes below on why this is breaking and label it with
breaking-change-rpcorbreaking-change-sdk.