Fix importing multiple accounts from ledger

[buberdds]

Select the first wallet from a selection after all wallets are loaded

Closes #934

[github-actions]

MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ EDITORCONFIG	editorconfig-checker	2		0	0.01s
✅ GIT	git_diff	yes		no	0.01s
✅ TYPESCRIPT	eslint	2	0	0	4.65s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

[codecov]

Codecov Report

Merging #980 (52dde69) into master (6b86f62) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #980      +/-   ##
==========================================
+ Coverage   88.19%   88.20%   +0.01%     
==========================================
  Files          99       99              
  Lines        1711     1713       +2     
  Branches      395      396       +1     
==========================================
+ Hits         1509     1511       +2     
  Misses        202      202              

Flag	Coverage Δ
cypress	57.60% <0.00%> (-0.13%)	⬇️
jest	79.12% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/app/state/wallet/saga.ts	98.55% <100.00%> (+0.04%)	⬆️

[lukaw3d]

Does this need to be here to work around walletId global variable messing everything up?

[buberdds]

It's not related to global variable. Test covers another bug/issue when user imports wallets and the first selected wallet exists in redux store. In this case we will use existing id not a new one stored in global walletId variable.

Steps to repro (selection modal):
A - selected
B - selected
C
D
E - selected

nav to import again
A
B - selected
C - selected
D - selected
E - selected

[lukaw3d]

How come this fixed the bug?

[lukaw3d]

Can this test be extended to validate the bug was fixed?

[buberdds]

Looks like you have already answered this question :D

[lukaw3d]

Update: I can't reproduce the bug with expectSaga on master. Looks like reducer + saga scheduling in app is different than in expectSaga.

const result = await expectSaga(rootWalletSaga)
.provide(providers)
.withReducer(walletReducer)
.dispatch(
  walletActions.openWalletsFromLedger([
    {
      address: 'oasis1qq2vzcvxn0js5unsch5me2xz4kr43vcasv0d5eq4',
    } as LedgerAccount,
    {
      address: 'oasis1qq5t7f2gecsjsdxmp5zxtwgck6pzpjmkvc657z6l',
    } as LedgerAccount,
    {
      address: 'oasis1qrusst9f8ey6kphgp5d02nuf4kx5qsa80vamtxj8',
    } as LedgerAccount,
    {
      address: 'oasis1qpqs54pdwxstpjefw5t6ucy425nt4yud3c4fgr8p',
    } as LedgerAccount,
    {
      address: 'oasis1qrf54xw003wkznm90w0q05ns8nryla0455tlex38',
    } as LedgerAccount,
  ]),
)
.fork(walletSaga)
.put({ type: walletActions.walletSelected.type, payload: 4 })
.silentRun(50)

expect(result.storeState).toMatchObject({
  selectedWallet: 4,
} as WalletState)

put(selectWallet) in step 8 schedules walletSelected at the end:

1. 'wallet/addWallet', payload: { id: 4, address: 'oasis1qq2vzcvxn0js5unsch5me2xz4kr43vcasv0d5eq4' }
2. 'wallet/addWallet', payload: { id: 5, address: 'oasis1qq5t7f2gecsjsdxmp5zxtwgck6pzpjmkvc657z6l' }
3. 'wallet/addWallet', payload: { id: 6, address: 'oasis1qrusst9f8ey6kphgp5d02nuf4kx5qsa80vamtxj8' }
4. 'wallet/addWallet', payload: { id: 7, address: 'oasis1qpqs54pdwxstpjefw5t6ucy425nt4yud3c4fgr8p' }
5. 'wallet/addWallet', payload: { id: 8, address: 'oasis1qrf54xw003wkznm90w0q05ns8nryla0455tlex38' }

6. 'wallet/walletSelected', payload: undefined
7. 'wallet/walletOpened', payload: { id: 4, address: 'oasis1qq2vzcvxn0js5unsch5me2xz4kr43vcasv0d5eq4' }
8. 'wallet/selectWallet', payload: 4

9. 'wallet/walletSelected', payload: undefined
10. 'wallet/walletOpened', payload: { id: 5, address: 'oasis1qq5t7f2gecsjsdxmp5zxtwgck6pzpjmkvc657z6l' }

11. 'wallet/walletSelected', payload: undefined
12. 'wallet/walletOpened', payload: { id: 6, address: 'oasis1qrusst9f8ey6kphgp5d02nuf4kx5qsa80vamtxj8' }

13. 'wallet/walletSelected', payload: undefined
14. 'wallet/walletOpened', payload: { id: 7, address: 'oasis1qpqs54pdwxstpjefw5t6ucy425nt4yud3c4fgr8p' }

15. 'wallet/walletSelected', payload: undefined
16. 'wallet/walletOpened', payload: { id: 8, address: 'oasis1qrf54xw003wkznm90w0q05ns8nryla0455tlex38' }

17. 'wallet/walletSelected', payload: 4

But it looks like can fix the bug by just removing yield* put(walletActions.walletSelected(initialState.selectedWallet)). There is no reason to select undefined when adding every wallet.

[buberdds]

Yeah probably there are many ways to fix it. My preference is to select a wallet when prev actions are done.

[lukaw3d]

I think we should do:

• commit: test for re-importing existing accounts (this test is already passing before any changes)
• commit: fix the issue by removing yield* put(walletActions.walletSelected(initialState.selectedWallet)) (selecting undefined when adding every wallet makes no sense)
• optionally commit: replacing selectImmediately with a select after all wallets are added

[buberdds]

• (selecting undefined when adding every wallet makes no sense)

fyi @lukaw3d this was actually handling a case when you open an active wallet again

[lukaw3d]

• (selecting undefined when adding every wallet makes no sense)

fyi @lukaw3d this was actually handling a case when you open an active wallet again

Ah, I didn't notice when it was added in https://github.com/oasisprotocol/oasis-wallet-web/pull/787/files#diff-f5039a29f4edb72ce140205418b6a20f84c4e5ed87c38bd33c4724aaee5c66fbR130

when changing from

  yield* takeLatest(walletActions.selectWallet, selectWallet)
    
  export function* selectWallet({ payload: index }: PayloadAction<number>) {
    ...
    yield* put(push(`/account/${newWallet?.address}`))
  }

to

  useEffect(() => {
    if (typeof activeWalletIndex !== 'undefined' && address) {
      history.push(`/account/${address}`)
    }
  }, [activeWalletIndex, address, history])

:/