Refactor: `wallet` screen to functional component #488

tuliomir · 2024-01-13T00:02:13Z

Acceptance Criteria

The wallet screen should be refactored to a functional component

Security Checklist

Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

codecov · 2024-01-13T00:04:38Z

Codecov Report

Attention: 969 lines in your changes are missing coverage. Please review.

Comparison is base (c0553fa) 9.35% compared to head (628700f) 9.21%.
Report is 7 commits behind head on dev.

❗ Current head 628700f differs from pull request most recent head 8098579. Consider uploading reports for the commit 8098579 to get more accurate results

Files	Patch %	Lines
src/screens/SendTokens.js	0.00%	115 Missing and 9 partials ⚠️
src/screens/Wallet.js	0.00%	100 Missing and 14 partials ⚠️
src/screens/Server.js	0.00%	85 Missing and 21 partials ⚠️
src/screens/CreateNFT.js	0.00%	66 Missing and 5 partials ⚠️
src/screens/UnknownTokens.js	0.00%	65 Missing and 6 partials ⚠️
src/screens/Settings.js	0.00%	57 Missing and 4 partials ⚠️
src/screens/AddressList.js	0.00%	54 Missing and 5 partials ⚠️
src/screens/CreateToken.js	0.00%	52 Missing and 4 partials ⚠️
src/screens/StartHardwareWallet.js	0.00%	46 Missing and 7 partials ⚠️
src/screens/NewWallet.js	0.00%	49 Missing and 3 partials ⚠️
... and 9 more

Additional details and impacted files

@@           Coverage Diff            @@
##             dev    #488      +/-   ##
========================================
- Coverage   9.35%   9.21%   -0.15%     
========================================
  Files        112     112              
  Lines       5161    5242      +81     
  Branches     698     696       -2     
========================================
  Hits         483     483              
- Misses      4015    4098      +83     
+ Partials     663     661       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alexruzenhack · 2024-01-30T16:18:53Z

src/screens/Wallet.js

+  const [backupDone, setBackupDone] = useState(LOCAL_STORE.isBackupDone());
+  /** successMessage {string} Message to be shown on alert success */
+  const [successMessage, setSuccessMessage] = useState('');
+  /* shouldShowAdministrativeTab {boolean} If we should display the Administrative Tools tab */
+  const [shouldShowAdministrativeTab, setShouldShowAdministrativeTab] = useState(false);
+  const [errorMessage, setErrorMessage] = useState(''); // TODO: Metadata token error are being suppressed as of now
+  const [totalSupply, setTotalSupply] = useState(null);
+  const [canMint, setCanMint] = useState(false);
+  const [canMelt, setCanMelt] = useState(false);
+  const [transactionsCount, setTransactionsCount] = useState(null);
+  const [mintCount, setMintCount] = useState(null);
+  const [meltCount, setMeltCount] = useState(null);


[suggestion] Why not aggregate all the states in an object and use a single useState?

I initially implemented it like that because it's the most common pattern for functional components: a getter/setter for each property.

Upon further investigation, I've seen that by having a state for each property we allow React to optimize when to re-render each element in the screen. If we had a single state object, whenever we updated any of its properties the whole screen would be re-rendered.

In practice, this seems to have almost no performance impact, so it's left as a style decision. I even thought about having walletInfo: { mintCount, meltCount } and tokenInfo: { canMint, canMelt, txCount } , but it didn't feel like it would improve the legibility of the code.

What do you think?

I don't have much to say. If it is better to use this way lets keep it.

It's usually preferable to use each property as a different state so we can pass them as dependencies on useCallback and useEffect and other hooks, allowing react to memoize those hooks

andreabadesso · 2024-01-26T14:18:26Z

src/screens/Wallet.js

 import hathorLib from '@hathor/wallet-lib';
 import { t } from 'ttag';
 import { get } from 'lodash';
-import { connect } from "react-redux";
+import { useDispatch, useSelector } from "react-redux";


Suggested change

import { useDispatch, useSelector } from "react-redux";

import { useDispatch, useSelector } from 'react-redux';

Fixed on f85bf7c

andreabadesso · 2024-01-26T14:18:47Z

src/screens/Wallet.js

-    useWalletService: state.useWalletService,
-  };
-};
+import { useHistory } from "react-router-dom";


Is the linter passing?

Suggested change

import { useHistory } from "react-router-dom";

import { useHistory } from 'react-router-dom';

We don't have a linter on this repo yet. 😞
Opened issue #500 for this.

Fixed on f85bf7c.

andreabadesso · 2024-01-26T14:23:07Z

src/screens/Wallet.js

+  const [successMessage, setSuccessMessage] = useState('');
+  /* shouldShowAdministrativeTab {boolean} If we should display the Administrative Tools tab */
+  const [shouldShowAdministrativeTab, setShouldShowAdministrativeTab] = useState(false);
+  const [errorMessage, setErrorMessage] = useState(''); // TODO: Metadata token error are being suppressed as of now


So why should we leave this here?

Even though issue #487 is already open to fix this I found it best to leave it as a warning in the code itself, because this is a bug.

Do you agree with leaving it as a comment?

I mean, setErrorMessage is not being used anywhere in this file, and it's a generic error message

So why keep it there?

Maybe remove the const [errorMessage, setErrorMessage] = useState(''); part and update the comment to explain that metadata fetch errors are being ignored

Done on 8098579

tuliomir added the refactor label Jan 13, 2024

tuliomir self-assigned this Jan 13, 2024

tuliomir mentioned this pull request Jan 13, 2024

Token metadata http requests rejected on development #487

Open

tuliomir added 7 commits January 24, 2024 12:54

refactor: wallet screen to functional component

9e35f0d

chore: suppress missing error message

a74cace

refactor: moves auxiliary functions to closures

debf58f

refactor: retryDownloadClicked

3f45d02

fix: data flow on reload

b28c6cd

refactor: rearrange functions

bc6232b

docs: improves docstrings

bb5e3ad

tuliomir force-pushed the refactor/functional-wallet branch from 878f69e to bb5e3ad Compare January 24, 2024 22:06

tuliomir requested review from andreabadesso and alexruzenhack January 25, 2024 16:29

tuliomir marked this pull request as ready for review January 25, 2024 16:29

tuliomir requested a review from pedroferreira1 as a code owner January 25, 2024 16:29

tuliomir removed the request for review from pedroferreira1 January 25, 2024 16:29

alexruzenhack reviewed Jan 30, 2024

View reviewed changes

alexruzenhack approved these changes Jan 30, 2024

View reviewed changes

andreabadesso requested changes Jan 30, 2024

View reviewed changes

style: linter

f85bf7c

tuliomir requested a review from andreabadesso January 30, 2024 19:11

refactor: removes unused code with a comment

8098579

tuliomir force-pushed the refactor/functional-wallet branch from 628700f to 8098579 Compare February 1, 2024 15:42

andreabadesso approved these changes Feb 1, 2024

View reviewed changes

tuliomir merged commit 0c6771b into dev Feb 1, 2024
4 checks passed

tuliomir deleted the refactor/functional-wallet branch February 1, 2024 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: `wallet` screen to functional component #488

Refactor: `wallet` screen to functional component #488

tuliomir commented Jan 13, 2024

codecov bot commented Jan 13, 2024 •

edited

Loading

alexruzenhack Jan 30, 2024

tuliomir Jan 30, 2024

alexruzenhack Jan 30, 2024 •

edited

Loading

andreabadesso Feb 1, 2024

andreabadesso Jan 26, 2024

tuliomir Jan 30, 2024

andreabadesso Jan 26, 2024

tuliomir Jan 30, 2024

andreabadesso Jan 26, 2024

tuliomir Jan 30, 2024

andreabadesso Feb 1, 2024 •

edited

Loading

tuliomir Feb 1, 2024

	import { useDispatch, useSelector } from "react-redux";
	import { useDispatch, useSelector } from 'react-redux';

	import { useHistory } from "react-router-dom";
	import { useHistory } from 'react-router-dom';

Refactor: wallet screen to functional component #488

Refactor: wallet screen to functional component #488

Conversation

tuliomir commented Jan 13, 2024

Acceptance Criteria

Security Checklist

codecov bot commented Jan 13, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexruzenhack Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreabadesso Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor: `wallet` screen to functional component #488

Refactor: `wallet` screen to functional component #488

codecov bot commented Jan 13, 2024 •

edited

Loading

alexruzenhack Jan 30, 2024 •

edited

Loading

andreabadesso Feb 1, 2024 •

edited

Loading