Page MenuHomePhabricator
Differential D14555

[Cashtab][Alias] Pending Aliases
AbandonedPublic
Actions

Authored by emack on Sep 28 2023, 13:08.

Details

Reviewers
bytesofman
Fabien
Group Reviewers
Restricted Project
Summary

Improvements since D14376

  • registered and pending alias arrays sorted at the point of retrieval (refreshAliases)
  • simplified registered and pending alias frontend rendering by removing redundant pendingAliasesDropdown and activeWalletAliases state vars
  • fixed the post-registration setAliases call that adds the newly registered alias to the pending list

Original scope from D14376

  • New refreshAliases() function added to useWallet that calls the /address endpoint to retrieve both registered and pending aliases for the active wallet
  • Latest aliases are stored in useWallet's state variable aliases, which are then used by Alias.js to populate the registered and pending alias dropdowns
  • Interval created in useWallet that calls refreshAliases() to refresh registered and pending aliases
  • useEffect() dependencies adjusted in Alias.js so that a refresh is triggered upon alias registration (balance change) and new aliases detected
  • No longer using localStorage to keep track of pending aliases
  • Per earlier feedback, no longer using websocket listener on 'BlockConnected' events as that may not be a one to one correlation to a successful registration
Test Plan
  • Register a new alias and observe it is displayed in the Pending Aliases dropdown
  • Register another alias and observe it is added to the previous pending alias
  • Wait for the registration tx to receive one confirmation, verify the alias has now moved from the Pending Aliases to the Registered Aliases dropdown
  • Register another alias, close Cashtab. When the tx receives one confirmation (monitor via explorer), open Cashtab and ensure this pending alias has moved into the Registered Aliases dropdown.
  • Use an invalid endpoint for the refreshAliases function in useWallet and ensure it disables the Register Alias button and both dropdowns are showing Error: Unable to retrieve aliases. Note: I purposely did not set a frontend alert for this otherwise a downtime with alias-server will result in a constant pop up every 30 seconds regardless of which page they're on.
  • Use an invalid endpoint for the alias registration process in Alias.js and ensure the registration process is disabled in the UI

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pendingAliasV2
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25273
Build 50132: Build Diffcashtab-tests
Build 50131: arc lint + arc unit

Event Timeline

emack created this revision.Sep 28 2023, 13:08
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 28 2023, 13:08
emack requested review of this revision.Sep 28 2023, 13:08
emack planned changes to this revision.Sep 28 2023, 13:09

Awaiting alias-server to come back up before final integration tests on this diff

Harbormaster completed remote builds in B25197: Diff 42432.Sep 28 2023, 13:11
bytesofman added inline comments.Sep 28 2023, 22:24
cashtab/src/components/Alias/Alias.js
260 ↗(On Diff #42432)

i haven't seen this syntax before -- where are you seeing this? we don't want to be using the current state, as a state variable, to rewrite the current state completely. We just want to be modifying what is being modified. In this case, adding one entry at the pending key.

here's what we have in setFormData in Send.js:

setFormData(p => ({
                    ...p,
                    [name]: value,
                }));

Normal JS stuff "works" in setting state field but always with weird app-is-haunted errors. Need to do this the right way.

Answer here is a bit dated but still applicable: https://stackoverflow.com/questions/43638938/updating-an-object-with-setstate-in-react

how to handle adding items to arrays: https://stackoverflow.com/questions/26253351/correct-modification-of-state-arrays-in-react-js

I'm just free handing this so it's probably incorrect, but you'll end up with something like this:

setAliases(previousAliases => {...previousAliases, pending: [...previousAliases.pending, newPendingAlias})
emack updated this revision to Diff 42466.Sep 30 2023, 13:36
emack marked an inline comment as done.

Updated the post-registration syntax that appends the new alias to the pending array. This can be validated by adding a console log to the top of useWallet's useEffect() function so it displays the aliases state variable value upon startup but not yet triggering the API refresh calls.

Harbormaster completed remote builds in B25215: Diff 42466.Sep 30 2023, 13:38
bytesofman requested changes to this revision.Oct 2 2023, 13:51
bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
106 ↗(On Diff #42466)

Is this always picking up pending aliases? that is, when the server websocket is connected?

It would all be happening instantly with websockets. Possible that even if the server is working correctly, the pending alias hasn't made it to the db yet.

Prudent to wait a second or two before refreshAliases is called

429 ↗(On Diff #42466)
cashtab/src/components/Alias/__tests__/Alias.test.js
99 ↗(On Diff #42466)

What unit test is failing without this? Shouldn't it be a function?

Other functions that are part of wallet context are not needed here -- why refreshAliases ?

cashtab/src/hooks/useWallet.js
1412 ↗(On Diff #42466)

all new functions need standard syntax documentation, i.e.

//**
/*
/* parms
/* throws
/* returns

etc

This revision now requires changes to proceed.Oct 2 2023, 13:51
emack updated this revision to Diff 42501.Oct 3 2023, 07:45
emack marked 4 inline comments as done.

Responding to feedback

cashtab/src/components/Alias/Alias.js
106 ↗(On Diff #42466)

While I haven't encountered any missed pending aliases, you do raise a good point. The issue would likely surface once we head into UAT.
Added 1 second delay prior to refreshAliases for the time being. Will increase if needed during UAT.

429 ↗(On Diff #42466)

Updated alert message.

cashtab/src/components/Alias/__tests__/Alias.test.js
99 ↗(On Diff #42466)

The 'Without wallet defined' unit test fails with null pointer exceptions without this because refreshAliases gets called from WalletContext as part of useEffect() every time Alias.js is rendered. But you're right about it needing to be a function rather than an empty object. Updated to be a mock function returning a default alias-server response.

cashtab/src/hooks/useWallet.js
1412 ↗(On Diff #42466)

Improved comments

Harbormaster completed remote builds in B25239: Diff 42501.Oct 3 2023, 07:48
bytesofman requested changes to this revision.Oct 3 2023, 18:24
bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
109 ↗(On Diff #42501)
135–137 ↗(On Diff #42501)

seems like this should be in the same useEffect block that is handling refreshAliases no? i.e., the one above

I'm pretty sure changing the wallet will also change balances.totalBalance, even if the wallets both have the same balance. Can check two empty wallets to confirm.

This revision now requires changes to proceed.Oct 3 2023, 18:24
emack updated this revision to Diff 42522.Oct 4 2023, 00:33
emack marked 2 inline comments as done.

Responding to feedback

cashtab/src/components/Alias/Alias.js
109 ↗(On Diff #42501)

Fixed typo

135–137 ↗(On Diff #42501)

The reason it is where it's currently at is because of the default flow.
e.g.

  1. On app load scenario, useWallet executes refreshAliases, and then when Alias.js loads, the wallet nor balance has changed so it comes through the 2nd useEffect. Hence the current location to pick up alias server errors.
  2. On interval alias refreshes, it goes from useWallet to Alias.js, and at that point it still hasn't changed the wallet nor balance so it's still the 2nd useEffect.
Harbormaster completed remote builds in B25253: Diff 42522.Oct 4 2023, 00:36
emack planned changes to this revision.Oct 4 2023, 00:40

Upon further testing, both useEffects are being triggered upon Alias.js load. Going to have a think about the most efficient approach to this.

emack updated this revision to Diff 42526.Oct 4 2023, 10:20

Following further testing, moving the alias server error logic to the first useEffect that uses balances.totalBalance as a dependency is the right approach. Have verified this useEffect block is triggered upon app load, Alias.js component load and post-registration broadcast. Additional verification can be done by malforming the alias-server endpoint used in useWallet's refreshAliases and in Alias.js' preparePreviewModal.

Harbormaster completed remote builds in B25256: Diff 42526.Oct 4 2023, 10:23
bytesofman requested changes to this revision.Oct 4 2023, 18:53

the use of aliasServerError context var which sets isValidAliasInput flag isn't quite a reundant daisy chain because there are other instances that would turn isValidAliasInput into false, such as the user inputting non-alphanumeric alias for registration which is unrelated to server error

right -- but these other instances are handled separately, as they should be. They get formatting errors (not the server error alert), and the button is also disabled.

cashtab/src/components/Alias/Alias.js
113–115 ↗(On Diff #42526)

this is rube goldberging the screen. remove -- update the diff description as well. see comments on where isValidAliasInput is used and why we don't need this.

480 ↗(On Diff #42526)

1 of the 2 uses of this state variable -- nothing to do with aliasServerError

491 ↗(On Diff #42526)

seems like this is just a bugfix, unrelated to the pending implementation?

541 ↗(On Diff #42526)

2 of 2 uses of isValidAliasInput state variable -- aliasServerError is already used here to disable the button, we don't also need to flip isValidAliasInput

This revision now requires changes to proceed.Oct 4 2023, 18:53
emack updated this revision to Diff 42559.Oct 5 2023, 04:42
emack marked 4 inline comments as done.

Responding to feedback

cashtab/src/components/Alias/Alias.js
113–115 ↗(On Diff #42526)

Removed

491 ↗(On Diff #42526)

This is related to the earlier feedback on this diff to clear the input field upon registration broadcast

emack edited the summary of this revision. (Show Details)Oct 5 2023, 04:43
Harbormaster completed remote builds in B25273: Diff 42559.Oct 5 2023, 04:45
Fabien requested changes to this revision.Oct 5 2023, 08:19

This diff has gone through 5 rounds of review and the general design is still trivially flawed. This is NOT OK.
It is incredible that none of you is realizing that using a delay to deal with network propagation time is a genuine bad idea.

I'm blocking this diff now and will not review it again, it can be abandoned.
I will review a fresh new diff that has been well thought before submitting the code.

Note for the future: there is no point in listing what is implemented in the diff summary, you can expect your reviewers to figure out by reading the code. If not, your code is just bad.
You should put there the WHY you did it that way so I can tell by reading the summary that time has been spent thinking about the problem being solved.

cashtab/src/components/Alias/Alias.js
110

I'm not sure what this is (assuming it's to trigger on tx broadcast since it happens on balance change ?), but this is never going to work reliably and is the clear symptom of a bad design.
You'll end up increasing the timeout over and over and never manage to cover all the cases, that's 100% guaranteed.

This revision now requires changes to proceed.Oct 5 2023, 08:19
emack abandoned this revision.Oct 5 2023, 09:50

Revision Contents
Changeset List

PathSize
cashtab/
src/
components/
Alias/
147 lines
__tests__/
3 lines
Home/
__mocks__/
hooks/
__tests__/
64 lines
76 lines

Diff 42559

View Options

cashtab/src/components/Alias/Alias.js

Loading...
View Options

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

Loading...
View Options

cashtab/src/components/Home/__mocks__/walletAndBalancesMock.js

Loading...
View Options

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

Loading...
View Options

cashtab/src/hooks/useWallet.js

Loading...