Page MenuHomePhabricator
Differential D15917

[Cashtab] Move wallet mgmt to its own screen
ClosedPublic
Actions

Authored by bytesofman on Apr 6 2024, 21:19.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABC0c228103b7f3: [Cashtab] Move wallet mgmt to its own screen
Summary

T2207

Depends on D15884

Move the Collapse-based wallet mgmt settings to their own screen. Rebuild it without antd.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bytesofman created this revision.Apr 6 2024, 21:19
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 6 2024, 21:19
Harbormaster completed remote builds in B28384: Diff 46914.Apr 6 2024, 21:19
teamcity added a comment.Apr 6 2024, 21:22
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Tail of the build log:

Installing mock-chronik-client dependencies...
/work/modules/mock-chronik-client /work/abc-ci-builds/cashtab-tests

added 236 packages, and audited 237 packages in 1s

35 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
/work/cashtab /work/modules/mock-chronik-client /work/abc-ci-builds/cashtab-tests

added 1718 packages, and audited 1719 packages in 29s

262 packages are looking for funding
  run `npm fund` for details

1 moderate severity vulnerability

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> cashtab@2.21.1 build
> node scripts/build.js

Creating an optimized production build...
Failed to compile.

Attempted import error: 'isValidNewWalletNameLength' is not exported from 'validation' (imported as 'isValidNewWalletNameLength').


Build cashtab-tests failed with exit code 1
bytesofman updated this revision to Diff 46915.Apr 6 2024, 21:34

working on tests

Harbormaster completed remote builds in B28385: Diff 46915.Apr 6 2024, 21:34
teamcity added a comment.Apr 6 2024, 21:37
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Tail of the build log:

Installing mock-chronik-client dependencies...
/work/modules/mock-chronik-client /work/abc-ci-builds/cashtab-tests

added 236 packages, and audited 237 packages in 1s

35 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
/work/cashtab /work/modules/mock-chronik-client /work/abc-ci-builds/cashtab-tests

added 1718 packages, and audited 1719 packages in 30s

262 packages are looking for funding
  run `npm fund` for details

1 moderate severity vulnerability

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> cashtab@2.21.1 build
> node scripts/build.js

Creating an optimized production build...
Failed to compile.

Attempted import error: 'isValidNewWalletNameLength' is not exported from 'validation' (imported as 'isValidNewWalletNameLength').


Build cashtab-tests failed with exit code 1
bytesofman updated this revision to Diff 46917.Apr 6 2024, 22:22

tests pass, move config tests from app to config

Harbormaster completed remote builds in B28386: Diff 46917.Apr 6 2024, 22:25
teamcity added a comment.Apr 6 2024, 22:26
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Tail of the build log:

  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
 src/transactions                |     100 |      100 |     100 |     100 |                                                                                                          
  index.js                       |     100 |      100 |     100 |     100 |                                                                                                          
 src/transactions/fixtures       |     100 |      100 |     100 |     100 |                                                                                                          
  mocks.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
 src/utils                       |   89.58 |    88.23 |     100 |   89.58 |                                                                                                          
  cashMethods.js                 |     100 |      100 |     100 |     100 |                                                                                                          
  formatting.js                  |   86.11 |    84.61 |     100 |   86.11 | 18,28,51-53                                                                                              
 src/utils/fixtures              |       0 |        0 |       0 |       0 |                                                                                                          
  vectors.js                     |       0 |        0 |       0 |       0 |                                                                                                          
 src/validation                  |    96.3 |    96.62 |     100 |   96.58 |                                                                                                          
  index.js                       |    96.3 |    96.62 |     100 |   96.58 | 178-179,321,353,373,420,618,714-715,849,854                                                              
 src/validation/fixtures         |     100 |      100 |     100 |     100 |                                                                                                          
  mocks.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
 src/wallet                      |   94.36 |    85.79 |   83.67 |   94.82 |                                                                                                          
  context.js                     |     100 |      100 |     100 |     100 |                                                                                                          
  index.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  useWallet.js                   |   91.69 |    79.33 |   74.19 |   92.36 | 66,150,423-429,468,538,609,636,658-666,706,738,769,775-778,852-853,878                                   
 src/wallet/fixtures             |     100 |      100 |     100 |     100 |                                                                                                          
  mocks.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
---------------------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='3385']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='4229']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='1682']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='2238']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='684']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='964']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='3334']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='4147']
##teamcity[blockClosed name='Code Coverage Summary']

Summary of all failing tests
FAIL src/components/Contacts/__tests__/index.test.js
  ● Test suite failed to run

    Cannot find module 'components/Configure/fixtures/mocks' from 'src/components/Contacts/__tests__/index.test.js'

       5 | import React from 'react';
       6 | import { walletWithXecAndTokens } from 'components/App/fixtures/mocks';
    >  7 | import { populatedContactList } from 'components/Configure/fixtures/mocks';
         | ^
       8 | import { render, screen, waitFor } from '@testing-library/react';
       9 | import userEvent, {
      10 |     PointerEventsCheckLevel,

      at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)
      at Object.require (src/components/Contacts/__tests__/index.test.js:7:1)


Test Suites: 1 failed, 31 passed, 32 total
Tests:       741 passed, 741 total
Snapshots:   0 total
Time:        41.619 s
Ran all test suites.
Build cashtab-tests failed with exit code 1
bytesofman updated this revision to Diff 46918.Apr 6 2024, 22:32

improve styles

Harbormaster completed remote builds in B28387: Diff 46918.Apr 6 2024, 22:36
teamcity added a comment.Apr 6 2024, 22:36
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Tail of the build log:

  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
 src/transactions                |     100 |      100 |     100 |     100 |                                                                                                          
  index.js                       |     100 |      100 |     100 |     100 |                                                                                                          
 src/transactions/fixtures       |     100 |      100 |     100 |     100 |                                                                                                          
  mocks.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
 src/utils                       |   89.58 |    88.23 |     100 |   89.58 |                                                                                                          
  cashMethods.js                 |     100 |      100 |     100 |     100 |                                                                                                          
  formatting.js                  |   86.11 |    84.61 |     100 |   86.11 | 18,28,51-53                                                                                              
 src/utils/fixtures              |       0 |        0 |       0 |       0 |                                                                                                          
  vectors.js                     |       0 |        0 |       0 |       0 |                                                                                                          
 src/validation                  |    96.3 |    96.62 |     100 |   96.58 |                                                                                                          
  index.js                       |    96.3 |    96.62 |     100 |   96.58 | 178-179,321,353,373,420,618,714-715,849,854                                                              
 src/validation/fixtures         |     100 |      100 |     100 |     100 |                                                                                                          
  mocks.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
 src/wallet                      |   94.36 |    85.79 |   83.67 |   94.82 |                                                                                                          
  context.js                     |     100 |      100 |     100 |     100 |                                                                                                          
  index.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  useWallet.js                   |   91.69 |    79.33 |   74.19 |   92.36 | 66,150,423-429,468,538,609,636,658-666,706,738,769,775-778,852-853,878                                   
 src/wallet/fixtures             |     100 |      100 |     100 |     100 |                                                                                                          
  mocks.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
---------------------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='3386']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='4230']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='1682']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='2238']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='684']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='964']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='3335']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='4148']
##teamcity[blockClosed name='Code Coverage Summary']

Summary of all failing tests
FAIL src/components/Contacts/__tests__/index.test.js
  ● Test suite failed to run

    Cannot find module 'components/Configure/fixtures/mocks' from 'src/components/Contacts/__tests__/index.test.js'

       5 | import React from 'react';
       6 | import { walletWithXecAndTokens } from 'components/App/fixtures/mocks';
    >  7 | import { populatedContactList } from 'components/Configure/fixtures/mocks';
         | ^
       8 | import { render, screen, waitFor } from '@testing-library/react';
       9 | import userEvent, {
      10 |     PointerEventsCheckLevel,

      at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)
      at Object.require (src/components/Contacts/__tests__/index.test.js:7:1)


Test Suites: 1 failed, 31 passed, 32 total
Tests:       741 passed, 741 total
Snapshots:   0 total
Time:        40.809 s
Ran all test suites.
Build cashtab-tests failed with exit code 1
bytesofman updated this revision to Diff 46919.Apr 6 2024, 22:40

use a different icon for wallets vs backup wallet

Harbormaster completed remote builds in B28388: Diff 46919.Apr 6 2024, 22:43
teamcity added a comment.Apr 6 2024, 22:43
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Tail of the build log:

  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
 src/transactions                |     100 |      100 |     100 |     100 |                                                                                                          
  index.js                       |     100 |      100 |     100 |     100 |                                                                                                          
 src/transactions/fixtures       |     100 |      100 |     100 |     100 |                                                                                                          
  mocks.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
 src/utils                       |   89.58 |    88.23 |     100 |   89.58 |                                                                                                          
  cashMethods.js                 |     100 |      100 |     100 |     100 |                                                                                                          
  formatting.js                  |   86.11 |    84.61 |     100 |   86.11 | 18,28,51-53                                                                                              
 src/utils/fixtures              |       0 |        0 |       0 |       0 |                                                                                                          
  vectors.js                     |       0 |        0 |       0 |       0 |                                                                                                          
 src/validation                  |    96.3 |    96.62 |     100 |   96.58 |                                                                                                          
  index.js                       |    96.3 |    96.62 |     100 |   96.58 | 178-179,321,353,373,420,618,714-715,849,854                                                              
 src/validation/fixtures         |     100 |      100 |     100 |     100 |                                                                                                          
  mocks.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
 src/wallet                      |   94.36 |    85.79 |   83.67 |   94.82 |                                                                                                          
  context.js                     |     100 |      100 |     100 |     100 |                                                                                                          
  index.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  useWallet.js                   |   91.69 |    79.33 |   74.19 |   92.36 | 66,150,423-429,468,538,609,636,658-666,706,738,769,775-778,852-853,878                                   
 src/wallet/fixtures             |     100 |      100 |     100 |     100 |                                                                                                          
  mocks.js                       |     100 |      100 |     100 |     100 |                                                                                                          
  vectors.js                     |     100 |      100 |     100 |     100 |                                                                                                          
---------------------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='3389']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='4233']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='1684']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='2240']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='685']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='965']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='3338']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='4151']
##teamcity[blockClosed name='Code Coverage Summary']

Summary of all failing tests
FAIL src/components/Contacts/__tests__/index.test.js
  ● Test suite failed to run

    Cannot find module 'components/Configure/fixtures/mocks' from 'src/components/Contacts/__tests__/index.test.js'

       5 | import React from 'react';
       6 | import { walletWithXecAndTokens } from 'components/App/fixtures/mocks';
    >  7 | import { populatedContactList } from 'components/Configure/fixtures/mocks';
         | ^
       8 | import { render, screen, waitFor } from '@testing-library/react';
       9 | import userEvent, {
      10 |     PointerEventsCheckLevel,

      at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)
      at Object.require (src/components/Contacts/__tests__/index.test.js:7:1)


Test Suites: 1 failed, 31 passed, 32 total
Tests:       741 passed, 741 total
Snapshots:   0 total
Time:        40.808 s
Ran all test suites.
Build cashtab-tests failed with exit code 1
bytesofman updated this revision to Diff 46921.Apr 6 2024, 22:53

update mocks

Harbormaster completed remote builds in B28389: Diff 46921.Apr 6 2024, 23:04
bytesofman published this revision for review.Apr 6 2024, 23:10
bytesofman added inline comments.
cashtab/src/components/App/__tests__/App.test.js
501 ↗(On Diff #46921)

Move these tests to Configure as that is what they are testing

cashtab/src/components/App/fixtures/mocks.js
2386 ↗(On Diff #46921)

this is now used by Contacts and Wallets, but not Configure, so keep it in App where it is common

cashtab/src/components/Common/CustomIcons.js
11 ↗(On Diff #46921)

gotta stop with the antd icons but will be looking at this separately

cashtab/src/components/Configure/__tests__/Configure.test.js
1 ↗(On Diff #46921)

In this file

  • wallet mgmt tests moved to new Wallets component
  • Settings tests from App.test.js moved here
cashtab/src/validation/index.js
448 ↗(On Diff #46921)

same as in Contacts improvement

instead of trying to add a wallet that exists, failing, and throwing an error

we validate before and disable the "ok" button if the name exists

emack requested changes to this revision.Apr 7 2024, 10:03
emack added a subscriber: emack.
emack added inline comments.
cashtab/src/components/Wallets/index.js
210 ↗(On Diff #46921)

It's not technically an issue arising out of this diff but we should either have a check here for unsupported 24 word seed mnemonics or just update the placeholder text on the modal to say "12 word mnemonic seed phrase"

image.png (183×322 px, 21 KB)

Particularly as validateMnemonic only checks whether there are no words or contains invalid words.

357 ↗(On Diff #46921)

image.png (189×451 px, 10 KB)

A few things here:

  • The copy icon is not aligned with the other adjacent icons in size (edit: just noticed this is also an issue in prod)
  • The Activate button look-wise feels out of place with rest of the UI with its contrasting greyness. A bit windows 95-ish. I'd suggest using something like below so it mimics the existing 'Activate word inside a rectangle with rounded edges and transparent bg" styling.
#activateButton{
  border-radius: 15px;
  border: 2px solid #73AD21;
  padding: 10px; 
  width: 55px;
  height: 20px;  
}
  • It's been a while since I looked at this component closely but what happened to the ol' hover mouse over the very long balance or long name and it expands to show the full text? It would look better than wrapping it in this diff. Especially when you consider how it would look with clippings for larger balances:

image.png (315×505 px, 26 KB)

This revision now requires changes to proceed.Apr 7 2024, 10:03
bytesofman updated this revision to Diff 46935.Apr 7 2024, 21:09
bytesofman marked 2 inline comments as done.

rebase, better function for formatting wallet balances, better Activate button

cashtab/src/components/Wallets/index.js
210 ↗(On Diff #46921)

validateMnemonic is using bip39's validation method and ensuring we only support the englsh word list. Not sure on the > 12 word seed. We should add validation for this separately (maybe? I guess it's not the end of the world if someone really wants to import this).

Good to note but imo not directly relevant to this diff.

357 ↗(On Diff #46921)

The copy icon is not aligned with the other adjacent icons in size (edit: just noticed this is also an issue in prod)

Will need to take care of the icons in general soon. imo this needs its own diff, as it impacts this and Contacts, and there is some scope to it.

  • We want icons to not come from antd
  • We want icons to line up properly
  • We want icons to be the correct size
  • We want to access these buttons not by data-testid

Needs to be done, but for now I think it is ok to keep to current behavior as much as is practical.

The Activate button look-wise feels out of place with rest of the UI with its contrasting greyness.

yup this is ugly, updated

It's been a while since I looked at this component closely but what happened to the ol' hover mouse over the very long balance or long name and it expands to show the full text? It would look better than wrapping it in this diff. Especially when you consider how it would look with clippings for larger balances:

I added a better function to format wallet balance (and shorten the string as much as practical). As for the mouse hover over, I removed the tooltip and I think we should leave it that way. Mobile users cannot see this anyway, and imo we should make sure the UX supports mobile best first. Something that is desktop only should not be seen as a solution to a mobile ux problem.

Harbormaster completed remote builds in B28399: Diff 46935.Apr 7 2024, 21:13
emack accepted this revision.Apr 8 2024, 04:54
This revision is now accepted and ready to land.Apr 8 2024, 04:54
Closed by commit rABC0c228103b7f3: [Cashtab] Move wallet mgmt to its own screen (authored by bytesofman). · Explain WhyApr 8 2024, 12:18
This revision was automatically updated to reflect the committed changes.
bytesofman added a commit: rABC0c228103b7f3: [Cashtab] Move wallet mgmt to its own screen.

Revision Contents
Changeset List

PathSize
cashtab/
extension/
public/
3 lines
4 lines
2 lines
src/
components/
App/
22 lines
__tests__/
261 lines
fixtures/
15 lines
Common/
7 lines
Configure/
741 lines
__tests__/
509 lines
fixtures/
Contacts/
__tests__/
6 lines
Wallets/
__tests__/
Configure/
__tests__/
index.test.js
Configure.test.js
158 lines
Wallets/
455 lines
82 lines
utils/
__tests__/
12 lines
fixtures/
70 lines
30 lines
validation/
__tests__/
27 lines
fixtures/
38 lines
29 lines

Diff 46944

View Options

cashtab/extension/public/manifest.json

Loading...
View Options

cashtab/package-lock.json

Loading...
View Options

cashtab/package.json

Loading...
View Options

cashtab/src/components/App/App.js

Loading...
View Options

cashtab/src/components/App/__tests__/App.test.js

Loading...
View Options

cashtab/src/components/App/fixtures/mocks.js

Loading...
View Options

cashtab/src/components/Common/CustomIcons.js

Loading...
View Options

cashtab/src/components/Configure/Configure.js

Loading...
View Options

cashtab/src/components/Configure/__tests__/Configure.test.js

Loading...
View Options

cashtab/src/components/Configure/fixtures/mocks.js

Loading...
View Options

cashtab/src/components/Contacts/__tests__/index.test.js

Loading...
View Options

cashtab/src/components/Wallets/__tests__/index.test.js

Loading...
View Options

cashtab/src/components/Wallets/index.js

Loading...
View Options

cashtab/src/components/Wallets/styles.js

Loading...
View Options

cashtab/src/utils/__tests__/formatting.test.js

Loading...
View Options

cashtab/src/utils/fixtures/vectors.js

Loading...
View Options

cashtab/src/utils/formatting.js

Loading...
View Options

cashtab/src/validation/__tests__/index.test.js

Loading...
View Options

cashtab/src/validation/fixtures/vectors.js

Loading...
View Options

cashtab/src/validation/index.js

Loading...