Details

Reviewers

Fabien
bytesofman

Group Reviewers

Restricted Project

Commits

rABC8e4b8d528fd7: [Cashtab][Alias] Pending Aliases

Summary

Updates Cashtab to handle pending aliases via the /address/ endpoint.

Design considerations:

When the user broadcasts a new alias via Alias.js there is network propagation time to consider before the pending alias is picked up by alias-server and exposured via the /address/ endpoint.
To mitigate this, the newly successfully broadcasted alias is locally appended to the existing pending dropdown list in Cashtab.
This ensures the user will not notice any delay in the UI between the point of registration broadcast and the next alias refresh interval, which is up to 30 seconds, which will then re-sync Cashtab with the updated pending list from alias-server.
If the pending alias was not successfully confirmed for this wallet, the alias refresh interval will reflect this between the Registered Aliases and Pending Aliases dropdowns.
There is a potential edge case where the user broadcasts a registration right before a scheduled interval alias refresh, and if the pending alias was not propagated to alias-server for that particular alias refresh event, then that local pending alias would disappear from the pending dropdown. However this is only temporary because the subsequent alias refresh interval (within 30 seconds) will bring it back in sync using the updated alias-server data as the source of truth. Hence I believe this rare occurance is negligible.
Upon alias-server outage, I purposely did not set a pop up alert for this otherwise an extended downtime with alias-server will result in a constant pop up every 30 seconds courtesy of the refresh interval regardless of which page the user is on.

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.
Use an invalid endpoint for the alias registration process in Alias.js and ensure the registration process is disabled in the UI
Attempt to register an alias that already has a pending tx for this wallet and observe a non-blocking warning underneath the input field. Click Register Alias and ensure it is broadcasted. Once confirmed, both instances are removed from Pending list and a single successful alias is added to Registered Aliases.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

pendingAliasV3

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25294
Build 50173: Build Diff	cashtab-tests
Build 50172: arc lint + arc unit

Event Timeline

emack created this revision.Oct 7 2023, 12:22

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 7 2023, 12:22

emack requested review of this revision.Oct 7 2023, 12:22

emack planned changes to this revision.Oct 7 2023, 12:22

Harbormaster completed remote builds in B25294: Diff 42587.Oct 7 2023, 12:25

emack edited the summary of this revision. (Show Details)Oct 7 2023, 12:26

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

the summary section for this diff is too long. if it's really that complicated, this should be split up into separate diffs.

imo it can be summarized more succinctly.

cashtab/src/hooks/useWallet.js
76	if no interval is set, this is `null`
1416	Looks like this function does not `throw` -- the error is caught and saved in a state variable.
1475	you only want to `setInterval` if `aliasIntervalId` in state is `null`

In D14611#328283, @bytesofman wrote:

the summary section for this diff is too long. if it's really that complicated, this should be split up into separate diffs.

imo it can be summarized more succinctly.

It looks like it because of the bullet points but it is really describing the rationale behind the solution, and the tradeoffs that have been made. There is no problem in describing your diff verbosely, it will also help with git history when needed.

(Note: I didn't look at the code)

Does a page refresh trigger an alias server call ? Because as a user if I see my pending alias disappear that would be my reflex: refresh the page.

Upon alias-server outage, I purposely did not set a pop up alert for this otherwise an extended downtime with alias-server will result in a constant pop up every 30 seconds courtesy of the refresh interval regardless of which page the user is on.

What do you do then ?

emack mentioned this in D14624: [Cashtab][Ailas] Remove aliasToRegister and aliasLength state vars.Oct 11 2023, 00:02

Updated interval default value and initialization logic.

Does a page refresh trigger an alias server call ?

Yes it does. The useEffect() within Alias.js that has a dependency on [wallet.name] triggers it upon refresh of the page. So this serves as another, more common mitigation path.

In D14611#328310, @Fabien wrote:

Upon alias-server outage, I purposely did not set a pop up alert for this otherwise an extended downtime with alias-server will result in a constant pop up every 30 seconds courtesy of the refresh interval regardless of which page the user is on.

What do you do then ?

It's similar to how when chronik goes down Cashtab renders a static Unable to connect to API error message on the component. When the alias-server goes down, a static "Error: Unable to retrieve aliases" message is rendered in the Registered and Pending Aliases dropdown, along with the Register Alias button perma-disabled. Since the interval is in the background polling every 30 seconds, once a successful connection has been re-established, these errors disappear and the list is correctly refreshed.

Harbormaster completed remote builds in B25353: Diff 42676.Oct 15 2023, 01:47

Pls note the state var cleanup is via D14624.

emack mentioned this in rABC8ea867c5bdca: [Cashtab][Ailas] Remove aliasToRegister and aliasLength state vars.Oct 16 2023, 10:39

Rebased to master after state var cleanup

Harbormaster completed remote builds in B25363: Diff 42690.Oct 16 2023, 11:25

Fabien requested changes to this revision.Oct 17 2023, 10:15

Fabien added inline comments.

cashtab/src/hooks/useWallet.js
1470 ↗	(On Diff #42690)	I'm not sure this makes sense: It defeats the purpose of having the pending aliases available from the server, as the wallet on another device might not see the registered aliases So once my aliases are registered or lost, they don't show up anymore ?

This revision now requires changes to proceed.Oct 17 2023, 10:15

emack requested review of this revision.Oct 17 2023, 11:18

emack added inline comments.

cashtab/src/hooks/useWallet.js
1470 ↗	(On Diff #42690)	This only refers to whether to initiate the interval upon app load. If the user uses a wallet on another device and navigates to the Alias.js page, the useEffect() block in Alias.js will trigger the `refreshAliases()` function which will refresh both registered and pending aliases. Once an alias is registered, they disappear from the pending list and appear in the registered list. If the alias was lost (confirmed for someone else in the block) then yes they will simply disappear with no additional dropdown displaying the failed alias registrations as that could balloon into a huge list very quickly. If this is a showstopper UX issue then potential mitigations include: a) checking for latest alias registrations on the ecash herald bot b) typing in the alias into the Send.js component and check whether it's registered to their address or not c) we reinstate what we had previously which was a websocket listener on the registration tx and upon 1 conf it notifies the user to check whether it was successful (doesn't work if they switch to another device on the same wallet) d) we add another section to Alias.js which allows the user to look up info for an alias, however the UI on that page is already looking very busy. Or perhaps a combination of the above.

Fabien requested changes to this revision.Oct 23 2023, 09:39

Fabien added inline comments.

cashtab/src/components/Alias/Alias.js
218 ↗	(On Diff #42690)	This is a problem imo. I would rather add a notice (like an obvious red bold one) but not prevent from continuing. The alias system has been designed to pick the lowest txid over all the valid txs, so a user might want to register several times to increase the chance of success when registering an alias.
cashtab/src/hooks/useWallet.js
1470 ↗	(On Diff #42690)	OK that makes more sense, thanks for the explanation. I think it's OK as is, maybe we'll need a way to tell the user the alias doesn't belong to its address but we can figure that out later as it can be a purely additive change (aka if anything this can be another diff).
1482 ↗	(On Diff #42690)	We can discuss the strategy here to make it a bit more responsive, but let's leave that to another diff.

This revision now requires changes to proceed.Oct 23 2023, 09:39

emack edited the test plan for this revision. (Show Details)Oct 23 2023, 12:12

The alias registration input field now detects whether an input (without needing to click Register Alias) already has a pending tx for this wallet and renders a non-blocking warning.

User can still proceed with the registration broadcast and it will result in two of the same pending alias entries in the Pending Aliases dropdown. (Note: this is in alignment with alias-server which also returns duplicate aliases in the pending array, albeit with unique txid)

However the above change introduced an issue where the duplicate pending alias persists in the Pending list even after one of them is confirmed and refresh has already taken place. Eventually worked out the root cause being a conflict of the array map key, which confuses React's frontend rendering. This previously wasn't an issue as it didn't allow duplicate aliases from being registered. Applied fix by setting array map index to each key, which probably should have been the approach to begin with.

Harbormaster completed remote builds in B25408: Diff 42743.Oct 24 2023, 11:35

Fabien added inline comments.Oct 24 2023, 18:51

cashtab/src/components/Alias/Alias.js
291 ↗	(On Diff #42743)	I don't understand, you don't show pending alias from other addresses as well ? As a user that's what I'm interested in

emack mentioned this in D14683: [alias-server] Return pending registrations for a specified alias.Oct 25 2023, 01:53

Pending D14683

emack mentioned this in D14666: [Cashtab][Alias] Alias lookup facility.Oct 27 2023, 09:58

emack requested review of this revision.Oct 27 2023, 11:39

emack added inline comments.

cashtab/src/components/Alias/Alias.js
291 ↗	(On Diff #42743)	I don't understand, you don't show pending alias from other addresses as well ? As a user that's what I'm interested in This `aliases` state var is basically the registered and pending array from the `/address/` endpoint from alias-server. Hence it only has pending aliases specific to this wallet. The recently landed D14683 adds the "all pending txs for this alias" array to the `/alias/` address, however to access that info is an additional API call. So there is a performance tradeoff for extending this check to include all pending alises from other addresses as well, particularly if you add it here which will trigger an API call per keystroke, which will slow down and lag Cashtab. The only other area that would make sense for this API call would be at the point of clicking the registration button, so the confirmation modal that currently says "are you sure you want to register this alias for xxx XEC" can just be extended to also note that there are x pending registration txs for this alias. If this is feasible then I would be removing the pending check per keystroke for the sake of consistency otherwise you'll have one area of the component checking for pending specific to this wallet, and the other checking across chain.

otherwise you'll have one area of the component checking for pending specific to this wallet, and the other checking across chain.

There are very different use cases: the per address is how you fill your pending and registered aliases lists, the the "full" alias check is to instruct the user of a conflict

As per checking every keystroke, if it's only checking for the user's address it's low value imo. I agree it's too expensive to do an API call at each time, so once after the button is clicked seems right to me

emack planned changes to this revision.Oct 27 2023, 21:37

Full onchain pending alias check is now implemented in the post-registration-button-click logic (preparePreviewModal), which renders an additional sentence in the registration confirmation modal, depending on whether pending.length is > 0. The local pending check per keystroke is now removed due to low value.

Screenshots below:

Registering test234234 which has no pending txs onchain

Registering test11 which has two other pending txs onchain

Registering test11 on behalf of another wallet, with this alias having two other pending txs onchain whilst

Harbormaster completed remote builds in B25466: Diff 42845.Oct 28 2023, 05:14

Can you tell if the pending aliases are from this wallet ? Like message in screenshot 2 would tell "there are 6 pending registrations for this alias, including 2 for this wallet address"

Added conditional substring to the pending alias warning which indicates number of pending registrations for this wallet address.

Harbormaster completed remote builds in B25487: Diff 42875.Oct 31 2023, 13:03

emack planned changes to this revision.Oct 31 2023, 13:05

Added pendingAliasCounter to tally count of certain aliases within the pending array for the current address.

When the current wallet does not have a pending registration for this alias but other wallets do

When the current wallet does have pending registration for this alias amongst the onchain pending txs

When the current wallet does have pending registration for this alias amongst the onchain pending txs + registering the alias on behalf of another address

Harbormaster completed remote builds in B25488: Diff 42876.Oct 31 2023, 13:24

Fabien accepted this revision.Oct 31 2023, 14:18

Fabien added inline comments.

cashtab/src/components/Alias/Alias.js
155 ↗	(On Diff #42876)	bug fix sneaked in :)

This revision is now accepted and ready to land.Oct 31 2023, 14:18

bytesofman requested changes to this revision.Oct 31 2023, 19:52

bytesofman added inline comments.

cashtab/src/components/Alias/Alias.js
158–164 ↗	(On Diff #42876)	confusing that `pendingAliasCounter` is defined as a function here with params `value, index` ...but then, below, `value` is acually an array, and `index` is a string? If we are going to have this function, please give it the usual 'this is a function' documentation about params, expected inputs and outputs. In this case, I don't think we need to define it as a function. Just use it directly. This also removes ambiguity about why the params are named things that they aren't. const thisAliasPendingCount = aliases.pending.filter(pendingAliasObj => pendingAliasObj.alias === formData.aliasName).length also -- why `x.alias == index` and not `x.alias === index` ?
167 ↗	(On Diff #42876)	imo `thisWalletThisAliasPendingCount` is less ambiguous, although a mouthful whatever you choose -- it is a little ambiguous to have `aliasDetailsResp.pending.length` which is really the pending count for this alias and `thisAliasPendingCount` which is not actually the pending count for this alias, but the pending count for this alias at this particular wallet

This revision now requires changes to proceed.Oct 31 2023, 19:52

Improved pending array filter and variable names.

Harbormaster completed remote builds in B25501: Diff 42892.Oct 31 2023, 23:49

let's make the fact that there are pending aliases here a little scarier

it's not that we don't want people to try multiple registrations -- but we do not want them to try it by accident. should have

mb 'warning' as a title
an alert icon
the fact that their are pending aliases out there should be bold, at least on its own line

this 'are you sure' (case where alias is available, no pending) should be friendlier and inviting

say it's available
something should be green

This revision now requires changes to proceed.Nov 1 2023, 17:29

actually -- seeing how these are implemented, I don't think my previous feedback needs to be handled in this diff. Should have separate diffs to get better modal appearance.

This revision is now accepted and ready to land.Nov 1 2023, 17:31

Closed by commit rABC8e4b8d528fd7: [Cashtab][Alias] Pending Aliases (authored by emack). · Explain WhyNov 1 2023, 22:49

This revision was automatically updated to reflect the committed changes.

emack added a commit: rABC8e4b8d528fd7: [Cashtab][Alias] Pending Aliases.

[Cashtab][Alias] Pending Aliases
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42587

cashtab/src/components/Alias/Alias.js

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

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

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

cashtab/src/hooks/useWallet.js

[Cashtab][Alias] Pending AliasesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42587

cashtab/src/components/Alias/Alias.js

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

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

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

cashtab/src/hooks/useWallet.js

[Cashtab][Alias] Pending Aliases
ClosedPublic
Actions

Revision Contents
Changeset List

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

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

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