fix: import all accounts for wallet

[pete-watters]

This PR contains a working draft of importing multiple accounts from a seed.

It is based on previous extension code but some more work is needed.

When importing the dev wallet account it takes quite a while and the resulting UI is very laggy (maybe as we have no controls in place on the max limits of the accounts)

[linear]

LEA-1579 Load all wallet accounts when restoring accounts

[pete-watters]

Added this for mnemonicToSeedSync in mnemonicToRootNode but maybe we have a different way on mobile?

[kyranjamie]

both of these methods exist in @leather.io/crypto

[codecov]

Codecov Report

Attention: Patch coverage is 97.91667% with 1 line in your changes missing coverage. Please review.

Project coverage is 24.65%. Comparing base (9b342af) to head (eea0493).
Report is 1 commits behind head on dev.

Files with missing lines	Patch %	Lines
packages/crypto/src/index.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #605      +/-   ##
==========================================
+ Coverage   24.07%   24.65%   +0.57%     
==========================================
  Files         164      165       +1     
  Lines        6085     6133      +48     
  Branches      333      346      +13     
==========================================
+ Hits         1465     1512      +47     
- Misses       4620     4621       +1     

Files with missing lines	Coverage Δ
packages/crypto/src/recurse-accounts.ts	100.00% <100.00%> (ø)
packages/crypto/src/index.ts	0.00% <0.00%> (ø)

Components	Coverage Δ
bitcoin	62.04% <ø> (ø)
query	12.94% <ø> (ø)
utils	48.31% <ø> (ø)
crypto	76.27% <97.91%> (+8.05%)	⬆️
stacks	54.71% <ø> (ø)

[pete-watters]

In the extension stx-chain.slice.ts for stxChainSlice.actions.restoreAccountIndex(recursiveActivityIndex) it is using highestAccountIndex which gets incremented in createNewAccount so maybe that's where I am going wrong.

[kyranjamie]

There should not be side effects in this callback.

The idea of this recursive look up is that we give it a callback to determine whether there's activity or not. Then, the return value (which isn't used here because of void recurseAccountsForActivity) tells you the highest account index that has activity.

Later, you use that index to know how many accounts should be created.

[kyranjamie]

I see here you're try to recurse as well, but that isn't needed because all the recursion is handled internally to this function, it's way easier that way

[pete-watters]

I based this code on the extension code commented out above it. That was using dispatch(stxChainSlice.actions.restoreAccountIndex(recursiveActivityIndex)) to restore accounts.

I could be mistaken, but when I debug this, it seems like in .then the recursiveActivityIndex is always 0 so this code is never reached (on extension or copied version). See extension draft .

I think accounts are actually created further on by:

 if (legacyAccountActivityLookup !== 0)
        // dispatch(stxChainSlice.actions.restoreAccountIndex(legacyAccountActivityLookup)); -> extension
        this.createNewAccountOfWallet(fingerprint, legacyAccountActivityLookup);

legacyAccountActivityLookup is determined by checkForLegacyGaiaConfigWithKnownGeneratedAccountIndex which I think is actually what is determining how many accounts to create.

What I think I can get to work is to run this.createNewAccountOfWallet inside of the recursive function if there is a balance

[kyranjamie]

Why does it always return 0? Does the account have activity you're testing with? It's quite slow, so are you waiting until the end?

What I think I can get to work is to run this.createNewAccountOfWallet inside of the recursive function if there is a balance

I really don't think this is the way to do it. The purpose of recurseAccountsForActivity is to handle the recursion of the look up. If you have to do additional recursion outside that fn, then either your implementation misses the point of the function, or the implementation of recurseAccountsForActivity doesn't work and needs fixing. It's been a while since I wrote this, and can likely be improved, it's v slow for example.

Happy to run through in more detail the gist of this function, but really its just a look up function that stops after n false values and returns a number.

Also, let's remove all the gaia stuff so we don't need to worry about legacy account look up code.

[pete-watters]

Thanks Kyran. I will do more testing and then implement the best solution using your recommendations.

I'm not sure if it's working as we think in the extension right now. I am testing with my personal wallet and the leather dev wallet and in both cases accounts are created only if if (legacyAccountActivityLookup !== 0).

We have two calls to add accounts:

• in recurseAccountsForActivity.then
dispatch(stxChainSlice.actions.restoreAccountIndex(recursiveActivityIndex));

• after a check for if (legacyAccountActivityLookup !== 0):
  dispatch(stxChainSlice.actions.restoreAccountIndex(legacyAccountActivityLookup));

• If I remove the dispatch after the legacyAccountActivityLookup check, no accounts are created apart from the first one.

• If I add a debugger in recurseAccountsForActivity.then it never gets hit.

Here's the full code.

 try {
      void recurseAccountsForActivity({
        async doesAddressHaveActivityFn(index) {
          ....
          return hasStxBalance || hasNames || hasBtcBalance;
        },
      }).then(recursiveActivityIndex => {
        if (recursiveActivityIndex <= legacyAccountActivityLookup) return;
        logger.info('Found account activity at higher index', { recursiveActivityIndex });
        dispatch(stxChainSlice.actions.restoreAccountIndex(recursiveActivityIndex));
      });
    } catch (e) {
      // Errors during account restore are non-critical and can fail silently
    }

 ....
    if (legacyAccountActivityLookup !== 0)
      dispatch(stxChainSlice.actions.restoreAccountIndex(legacyAccountActivityLookup));
  };

If I change this code to be:

const accountsWithBalances = recurseAccountsForActivity({
        async doesAddressHaveActivityFn(index) {
          ....
          return hasStxBalance || hasNames || hasBtcBalance;
        },
      })

accountsWithBalances is always 0. If I add logs inside of there I can see it finding accounts with balances but I think it's not working as we expect.

[pete-watters]

Upon further investigation into this, it seems like the recursion we are using does not generate any accounts. They are in fact all generated initially based on the count returned from checkForLegacyGaiaConfigWithKnownGeneratedAccountIndex

Here is a demo PR from the extension showing that: leather-io/extension#5945

[kyranjamie]

both of these methods exist in @leather.io/crypto

[kyranjamie]

unintended I imagine

[pete-watters]

I saw some warnings about it being out of date so updated it. I should have removed from here

[kyranjamie]

this shouldn't be added directly to mobile, instead though of as a distinct module that I think should like in /crypto pkg, the refactored out to extension

[pete-watters]

I will do this once I can get it to work properly

[kyranjamie]

suggest we don't add any of the gaia checks here, resolving the import issue, check stacks/bitcoin balances only

[kyranjamie]

There should not be side effects in this callback.

The idea of this recursive look up is that we give it a callback to determine whether there's activity or not. Then, the return value (which isn't used here because of void recurseAccountsForActivity) tells you the highest account index that has activity.

Later, you use that index to know how many accounts should be created.

[kyranjamie]

pseudo code we need here

// user restores mnemonic
const mnemonic = 'abandon …';

async function doesStacksAddressHaveBalance (index) {
  // derive address
  // look up api for balances, return true if found
}
async  function doesNativeSegwitAddressHaveBalance (index) {
  // derive address
  // look up api for balances, return true if found
}

const highestAccountIndex = await recurseAccountsForActivity({
  async doesAddressHaveActivityFn(index) {
    const hasStacksBal = await doesStacksAddressHaveBalance(index);
    const hasNsBal = await doesNativeSegwitAddressHaveBalance(index)
    // this tells the inner lookup function to keep looking, or not
    // using callbacks like this means that the inner fn isn't aware of any specific chain related 
    // details, all it is told is whether it needs to keep looking or not
    return hasStacksBal ?? hasNsBal;
  }
})

// now we know that `highestAccountIndex` is the account with the farthest known balance

const newAccountsToAdd = createNullArrayOf(highestAccountIndex).map(index => {
  return generateTheAccountOfThisIndex();
})

// redux store method to add all of these to the store
// should be single action with many, not many actions with a single item
dispatch(etc)

This look up can take some time though, so ideally we want some way of handling this in the background. I would first get the slow version implemented before tackling that though.

[pete-watters]

If I try restore one of my personal wallets on the extension the accounts also get generated here.

That account is from 2023 at the earliest so I don't think it should be considered legacy?

[pete-watters]

pseudo code we need here

// user restores mnemonic
const mnemonic = 'abandon …';

async function doesStacksAddressHaveBalance (index) {
  // derive address
  // look up api for balances, return true if found
}
async  function doesNativeSegwitAddressHaveBalance (index) {
  // derive address
  // look up api for balances, return true if found
}

const highestAccountIndex = await recurseAccountsForActivity({
  async doesAddressHaveActivityFn(index) {
    const hasStacksBal = await doesStacksAddressHaveBalance(index);
    const hasNsBal = await doesNativeSegwitAddressHaveBalance(index)
    // this tells the inner lookup function to keep looking, or not
    // using callbacks like this means that the inner fn isn't aware of any specific chain related 
    // details, all it is told is whether it needs to keep looking or not
    return hasStacksBal ?? hasNsBal;
  }
})

// now we know that `highestAccountIndex` is the account with the farthest known balance

const newAccountsToAdd = createNullArrayOf(highestAccountIndex).map(index => {
  return generateTheAccountOfThisIndex();
})

// redux store method to add all of these to the store
// should be single action with many, not many actions with a single item
dispatch(etc)

This look up can take some time though, so ideally we want some way of handling this in the background. I would first get the slow version implemented before tackling that though.

Thanks!

[kyranjamie]

the keychain is different for every account

[pete-watters]

Thanks @kyranjamie 👍

I realised I have more work to do as I was hitting issues in bitcoin-keychains.read.ts with undefined nativeSegwit taproot.

[kyranjamie]

Curious about this comment. This adapter already uses addMany, not addOne

[pete-watters]

I added a new action userAddsAccounts to add many accounts at once so I think I need to adapt this to also handle their keychains as that's not working fully and the accounts seem incomplete.

I am hitting issues with undefined keyOrigin so I think I missed some things during account creation

[pete-watters]

Solved this. I'm passing an array of account creation data so needed a flatMap here

[kyranjamie]

odd reassignments here

Suggested change

	const mostRecentAccount = await recurseUntilGeneratorDone(checkForMostRecentAccount());
	const accountsToRestore = mostRecentAccount;
	return accountsToRestore;
	const accountsToRestore = await recurseUntilGeneratorDone(checkForMostRecentAccount());
	return accountsToRestore;

[kyranjamie]

this is in crypto pkg

[pete-watters]

I'll be removing this whole file in favour of the newer balance queries we have

[kyranjamie]

I wonder how we're going to manage this functionality, because in the case of lots of accounts, it's going to take ages

[pete-watters]

I wonder how we're going to manage this functionality, because in the case of lots of accounts, it's going to take ages

I will think about this a bit more. The latest version of this is still messy but it's working at least.

If I try and restore the Leather dev wallet it takes a while (I'll get stats) and although it creates the accounts, the resulting UI is very laggy. Probably as we have no controls to limit how many things to show etc.

I had a call with @alexp3y about this yesterday in relation to the balances service in case he has some ideas there too.

The flow for determining active keychains and creating accounts now is to:

• iterate over them recursively and check the balance
• repeat until we have no more balances
• batch create accounts

I will be looking into activity soon so I wonder if there is a better way of determining active addresses using activity instead of balances?

Or if on first load when we are trying to calculate the totalBalance, we could also check for the highest index active keychain in a worker?

I'll keep thinking about this. Also wondering if we should add the keychain to state for each account index