Page MenuHomePhabricator

[Cashtab] Use context instead of mixed approach for managing contacts in Configure
ClosedPublic

Authored by bytesofman on Feb 1 2024, 00:49.

Details

Summary

Right now we have a local state variable and local storage. Instead, we should use the available contactList from wallet context -- so there is only one source of truth here.

Ended up requiring a significant refactor. I added several integration tests to support. However some cases are still not tested (see test plan).

This is the kind of diff that really shows the negative fallout from rushing to add a feature. The contact list is a low impact feature. It was poorly implemented and now must be cleaned up to avoid deprecating it.

Test Plan

npm test

Add a contact from savedWallet
Try to add it again, it won't add bc it exists

Diff Detail

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

Event Timeline

Add integration test for editing contact list in config

no dep changes, remove debug log

add todo for needed unit tests, code cleanup

Update tests for new router, rebase, version bump

get rid of the ridiculous [{}] default

clean up code, test case of adding a saved wallet

bytesofman edited the test plan for this revision. (Show Details)

removing more antipatterns, do not use duplicated flag

remove try...catch from updateContactList calls as this is handled in the actual function

bytesofman published this revision for review.Feb 9 2024, 20:08
PiRK added a subscriber: PiRK.

good cleanup and test coverage

This revision is now accepted and ready to land.Feb 10 2024, 11:58