Persist state and sync between tabs

[lukaw3d]

Part of #792

Temporarily deployed to https://lukaw3d.github.io/oasis-wallet-web/ce527a5/#/
Updated deploy https://lukaw3d.github.io/oasis-wallet-web/32fbe2b/#/

Old preview of 22369e0

Preview of ce527a5

TODO

• don't store fatal errors
• syncing multiple tabs and cleanup, e.g. clearAccount
  open account in two tabs http://localhost:3000/account/oasis1qzwl8nn8lt8cv4gxh85skv9rw3h0raudws66q4y5
  navigate to home on one tab (dispatches clearAccount)
  other tab loses data and shows loading
• skip login:
  • refactor to make networkSaga maintainable:
    always show unlock modal and
    if profile: on unlock emit setUnlockedRootState
    if profile: on skip emit event like setUnlockedRootState to use in networkSaga
    if no profile: auto click skip login emit event like setUnlockedRootState to use in networkSaga
• walletId global var
• close wallet = lock
• change password
• choose password and create profile when importing
• test ui
• test persistence

Deferred and moved to issue:

• auto lock timeout
• multiple profiles
• migrate old storage @metamask/browser-passworder
• test syncing tabs

[github-actions]

🦙 MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ EDITORCONFIG	editorconfig-checker	52		0	0.09s
✅ HTML	htmlhint	1		0	0.26s
✅ JSON	eslint-plugin-jsonc	1	0	0	0.93s
✅ JSON	jsonlint	1		0	0.23s
✅ JSON	prettier	1	0	0	0.8s
✅ JSON	v8r	1		0	1.47s
✅ REPOSITORY	checkov	yes		no	14.77s
✅ REPOSITORY	git_diff	yes		no	0.0s
✅ TSX	eslint	11	0	0	6.7s
✅ TYPESCRIPT	eslint	34	0	0	7.75s

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

MegaLinter is graciously provided by

[pro-wh]

reading about how this works

[pro-wh]

on the surface level it's an awkward asymmetry with encryptWithKey, but deeper down, it really would make a lot of sense to expose a decryptWithKey instead. the only callsite other than in tests pairs it up with another deriveKeyFromPassword, because it also needs the key itself (foreshadowed by encryptWithKey to save any updates will the key). the password-to-key derivation being something that's supposed to be expensive, it would be nice not to have to do it twice

[lukaw3d]

I don't understand :D

A subset of the comment: true, we could avoid deriveKeyFromPassword (with a new random salt) in decryptState (on unlock) by returning derivedKeyWithSalt (with old salt) from decryptWithPassword. Seems safe

[pro-wh]

ya I guess, if you prefer to couple the derivation and decryption into one call

[pro-wh]

security review: we'd be fine using the same salt, as the password is still the same. I'm not aware of any problems with changing it every time either though

[lukaw3d]

I temporarily deployed it to https://lukaw3d.github.io/oasis-wallet-web/ce527a5/#/

[nikolaglumac]

Great work @lukaw3d 🎉
I will do a proper review later today or tomorrow - one thing that I want to mention now (and perhaps that is something we can address in a separate PR) is that we should think about password rules. Having users set a single-character password is definitely not what we should allow.

[nikolaglumac]

Hi @lukaw3d - the flow works pretty well! I think it is a good first version - we can later iterate on it and improve it over time (e.g. by adding "remove-account" options etc).

There is this minor thing I found:

It should be an easy fix.

Thanks!

[lukaw3d]

@nikolaglumac
There is this minor thing I found: Once you create a profile, regardless of if you opened it later or not, "Close wallet" button is always visible

That was intentional so users can get back to login screen after clicking "Continue without the profile" (without it: user would need to create a wallet to get that same button)

[lukaw3d]

Fixing 1. is annoying

• I could refactor resetRootState to keep some slices, instead of resetting everything but: it doesn't feel correct to keep state after user clicks Close Wallet; and it probably introduces state combinations that previously weren't reachable and handled.
• Another option: I could redirect home on resetRootState, but only in webapp takeLatest(resetRootState, () => { if (runtimeIs === 'webapp' && selectUnlockedStatus === 'emptyUnpersisted') location.href = '/' }). Luckily extension with saga in background page won't support multiple tabs / popups yet. And that still needs navigate('/'); dispatch(persistActions.lockAsync()) on click.

[buberdds]

Is this redirect needed? when I open /account/oasis1.. I would expect that continue will allow me to see acc details.

[lukaw3d]

Good UX question. I've been considering the skipped profile like incognito mode, not read-only profile.

• Against:
  • We've removed search for foreign accounts, so read-only view is barely supported.
  • I was considering removing remove account ID from URL. To have fewer sources of state. That would also increase privacy - after locking wallet, someone couldn't check browser history (or click skip) to get your address.
• For:
  • Would allow wallet owner to quickly get their account's address. Pre-redirect URL might not be locked account (open a foreign account URL from bookmark and click skip). More complex proposal is to have a wallet pin to unlock read-only, and only ask for password before signing transactions.
  • Would allow quickly viewing foreign account's balance from URL/bookmark. But this is just worse search.

[lukaw3d]

This is failing on CI, but not locally

[lukaw3d]

I haven't found a reason why it matches differently on CI, only fixed it with 77be857.
await expect(tab2.getByRole('button', { name: 'Unlock' })).toBeHidden() on CI matched

<a aria-label="Open wallet">
  <button tabindex="-1">
    <div>
      <svg aria-label="Unlock">..</svg>
      Open wallet
</

[iSQL]

Hello,

Is there any info when will this be published?

[lukaw3d]

@iSQL I'm not sure. We'll want to deploy it after we move wallet to a new domain (so there's no confusion about saved data on current domain)

and perhaps finish more of #792 (comment)

[lukaw3d]

@iSQL it is finally published