Skip to content

Fix importing and reselecting the same account and cleanup#984

Merged
lukaw3d merged 7 commits intomasterfrom
lw/cleanup-wallet
Sep 22, 2022
Merged

Fix importing and reselecting the same account and cleanup#984
lukaw3d merged 7 commits intomasterfrom
lw/cleanup-wallet

Conversation

@lukaw3d
Copy link
Copy Markdown
Contributor

@lukaw3d lukaw3d commented Sep 7, 2022

@lukaw3d lukaw3d requested a review from buberdds September 7, 2022 02:01
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 7, 2022

MegaLinter status: ✅ SUCCESS

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

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
Copy link
Copy Markdown

codecov Bot commented Sep 7, 2022

Codecov Report

Merging #984 (2381e00) into master (9e38502) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 2381e00 differs from pull request most recent head ce24369. Consider uploading reports for the commit ce24369 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #984      +/-   ##
==========================================
+ Coverage   88.73%   88.78%   +0.05%     
==========================================
  Files         100      100              
  Lines        1757     1748       -9     
  Branches      407      405       -2     
==========================================
- Hits         1559     1552       -7     
+ Misses        198      196       -2     
Flag Coverage Δ
cypress 60.58% <100.00%> (+0.31%) ⬆️
jest 79.71% <71.42%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
src/app/state/wallet/index.ts 78.94% <100.00%> (+6.22%) ⬆️
src/app/state/wallet/saga.ts 98.38% <100.00%> (-0.15%) ⬇️

@buberdds
Copy link
Copy Markdown
Contributor

buberdds commented Sep 7, 2022

re-importing of a wallet does not work without walletSelected. if we want to get rid of it we should handle that via selectWallet

@lukaw3d
Copy link
Copy Markdown
Contributor Author

lukaw3d commented Sep 7, 2022

I'm not sure what you mean. Yes, re-importing of a wallet does not work, either way. This is not an attempt to fix it tho.

And an ugly way to fix it remains possible:

  if (selectImmediately) {
    yield* put(walletActions.selectWallet(undefined))
    yield* put(walletActions.selectWallet(walletId))
  }

@buberdds
Copy link
Copy Markdown
Contributor

buberdds commented Sep 8, 2022

Yes, please include this action if you want to remove walletSelected.

@lukaw3d lukaw3d changed the title Cleanup wallet actions Cleanup wallet actions and fix importing and reselecting the same account Sep 8, 2022
@lukaw3d
Copy link
Copy Markdown
Contributor Author

lukaw3d commented Sep 8, 2022

Added back selecting undefined as a quickfix.

Larger solutions:

  • when importing: disable checkboxes for existing accounts
  • remove useRouteRedirects and redirect in import components

Comment thread src/app/state/wallet/saga.ts
@lukaw3d lukaw3d changed the title Cleanup wallet actions and fix importing and reselecting the same account Fix importing and reselecting the same account and cleanup Sep 19, 2022
@lukaw3d lukaw3d force-pushed the lw/cleanup-wallet branch 3 times, most recently from 7ac5576 to 864e274 Compare September 20, 2022 01:14
@buberdds
Copy link
Copy Markdown
Contributor

not sure it's wip or ready for review, but reselecting is still failing for me for a mnemonic and ledger, but works for a priv key.

@lukaw3d
Copy link
Copy Markdown
Contributor Author

lukaw3d commented Sep 20, 2022

oh, it's supposed to work :/

@lukaw3d
Copy link
Copy Markdown
Contributor Author

lukaw3d commented Sep 20, 2022

Alright, it's ready now.
I'll simplify into these commits after review:

Remove unused walletLoaded action
Deduplicate walletSelected and selectWallet
Don't keep selectImmediately in wallet state
Deduplicate selecting wallet by using selectImmediately
Test importing multiple accounts
Test reimporting and reselecting the same accounts
Fix reimporting and reselecting the same accounts

Comment thread src/app/state/wallet/saga.ts Outdated
if (newWallet.selectImmediately) {
if (selectImmediately) {
yield* put(walletActions.selectWallet(undefined)) // Workaround so useRouteRedirects detects selecting the same account
yield* delay(1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch, can you add a comment that this is used to avoid React batch updates?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep yep

Tho I'm not sure why this would be batched in react 17 🤷

@lukaw3d lukaw3d merged commit e299b26 into master Sep 22, 2022
@lukaw3d lukaw3d deleted the lw/cleanup-wallet branch September 22, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants