Page MenuHomePhabricator

[Cashtab] Move settings to new cashtabState
ClosedPublic

Authored by bytesofman on Feb 27 2024, 18:59.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABCab7251c786e3: [Cashtab] Move settings to new cashtabState
Summary

T3445

Add settings to the new cashtabState key with its standardized loading and setting methods.

Implement throughout the app. Add a new integration test confirming that toggling a setting on Config screen changes behavior on SendXec screen.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
settings-to-cashtabstate
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 27471
Build 54505: Build Diffcashtab-tests
Build 54504: arc lint + arc unit

Event Timeline

add test to confirm migration of missing settings keys, remove debug logging, fix but in migration of missing keys

Tail of the build log:

/work/cashtab /work/abc-ci-builds/cashtab-tests
npm WARN deprecated rollup-plugin-terser@7.0.2: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-terser
npm WARN deprecated @babel/plugin-proposal-private-methods@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-private-methods instead.
npm WARN deprecated @babel/plugin-proposal-nullish-coalescing-operator@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-nullish-coalescing-operator instead.
npm WARN deprecated @babel/plugin-proposal-numeric-separator@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-numeric-separator instead.
npm WARN deprecated abab@2.0.6: Use your platform's native atob() and btoa() methods instead
npm WARN deprecated @babel/plugin-proposal-class-properties@7.18.6: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-class-properties instead.
npm WARN deprecated @babel/plugin-proposal-private-property-in-object@7.21.11: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-private-property-in-object instead.
npm WARN deprecated sourcemap-codec@1.4.8: Please use @jridgewell/sourcemap-codec instead
npm WARN deprecated domexception@4.0.0: Use your platform's native DOMException instead
npm WARN deprecated stable@0.1.8: Modern JS already guarantees Array#sort() is a stable sort, so this library is deprecated. See the compatibility table on MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#browser_compatibility
npm WARN deprecated @babel/plugin-proposal-optional-chaining@7.21.0: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-optional-chaining instead.
npm WARN deprecated workbox-cacheable-response@6.6.0: workbox-background-sync@6.6.0

added 1722 packages, and audited 1723 packages in 22s

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

found 0 vulnerabilities

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

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

[eslint] 
src/hooks/useWallet.js
  Line 35:29:  'cashtabDefaultConfig' is defined but never used  no-unused-vars

Search for the keywords to learn more about each error.


Build cashtab-tests failed with exit code 1
bytesofman added inline comments.
cashtab/src/components/Receive/fixtures/mocks.js
806

Note: the use of these context objects is being phased out as I work through T3444

cashtab/src/components/Send/fixtures/mocks.js
802 ↗(On Diff #45676)

wil be deprecating these context mocks as continue through T3444

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

We need this for the button clicking in the newly added tests. But, it should also be applied everywhere, since it's needed to keep some antd getters from flakiness.

cashtab/src/hooks/__tests__/useWallet.test.js
24

This is already covered in useWalletStorage.test.js (test 'Cashtab loads wallet, settings, cache, and contactlist from localforage to context if they are present')

So, remove this rather than update it for the new format.

cashtab/src/hooks/useWallet.js
1407 ↗(On Diff #45676)

useEffect is the right way to do this.

Not by manually shutting it down and re-initializing it every time settings change, IF that change happened to be the part that should change the fiat price checker

this expected behavior is covered by existing integration tests

cashtab/src/validation/__tests__/index.test.js
927 ↗(On Diff #45676)

better unit test organization for improved function

cashtab/src/validation/index.js
263 ↗(On Diff #45676)

function is confusingly named, undocumented.

give it a better name and document it.

emack requested changes to this revision.Feb 27 2024, 21:48
emack added a subscriber: emack.
emack added inline comments.
cashtab/src/hooks/useWallet.js
289 ↗(On Diff #45677)

for consistency with all the other instances of localforage interactions, wrap this in a try/catch block. Will make it easier to debug too.

This revision now requires changes to proceed.Feb 27 2024, 21:48
bytesofman marked an inline comment as done.
bytesofman added inline comments.
cashtab/src/hooks/useWallet.js
289 ↗(On Diff #45677)

this was removed in the last diff, which took out params from loadCashtabState

D15499

rationale there:

have always wrapped this in try...catch in Cashtab and console.log err msg. However, have never actually seen this error, and it is always untested.

So, stop doing this. If there is an error it will throw.

The new standard will be to never wrap in try...catch -- have never observed this error. Now with extensive testing including localforage interaction with components, lots of opportunities to see if it's ever getting thrown.

This revision is now accepted and ready to land.Feb 27 2024, 21:58
This revision was automatically updated to reflect the committed changes.
bytesofman marked an inline comment as done.