Page MenuHomePhabricator

[Cashtab] contactList PropType error patch
ClosedPublic

Authored by kieran709 on Jul 7 2022, 18:06.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABCe328572e90f4: [Cashtab] contactList PropType error patch
Summary

In useWallet.js at line 41, contactList is initialized as a boolean, instead of an empty array. Related to task T2515.

From Joey:

After reviewing the code -- this looks good and I think it is a good approach. However, please provide an explanation as to why it's okay to not initialize contactList as false.

There is no logic that I can find that depends on contactList being a boolean value. When loadContactList runs, if the contactList is not found in localStorage, it will be set as an empty array containing an empty object.

For example, wallet is initialized as false -- and this is critical to the logic of how Cashtab functions, i.e. this is how it knows whether or not the user has a wallet.

Looking through with grep -r 'contactList' src/, I was not able to find similar logic where contactList is treated as a boolean before it is initialized. However, please confirm this, and please add this discussion to the diff.

Although I also could not find an instance of Cashtab requiring contactList to be initialized as a boolean, an alternative option to preserve this logic would be to use the oneOfType Proptype and include boolean as an acceptable Proptype. With that being said, having tested the various contactList actions and reviewing the code, it seems certain that it is an unnecessary inclusion; so in the spirit of writing less code, I believe it is best to initialize contactList as an empty array containing an empty object.

Test Plan

cd web/cashtab && npm start
Open dev tools
Observe that the contactList error does not appear in the console

Diff Detail

Repository
rABC Bitcoin ABC
Branch
contactList-proptypes-patch
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19595
Build 38909: Build Diffcashtab-tests
Build 38908: arc lint + arc unit

Event Timeline

bytesofman requested changes to this revision.Jul 7 2022, 21:09

After reviewing the code -- this looks good and I think it is a good approach. However, please provide an explanation as to why it's okay to not initialize contactList as false.

For example, wallet is initialized as false -- and this is critical to the logic of how Cashtab functions, i.e. this is how it knows whether or not the user has a wallet.

Looking through with grep -r 'contactList' src/, I was not able to find similar logic where contactList is treated as a boolean before it is initialized. However, please confirm this, and please add this discussion to the diff.

https://en.wikipedia.org/wiki/Wikipedia:Chesterton's_fence

This revision now requires changes to proceed.Jul 7 2022, 21:09
This revision is now accepted and ready to land.Jul 8 2022, 16:54