Page MenuHomePhabricator

[Cashtab][Alias] Pending Aliases
ClosedPublic

Authored by emack on Oct 7 2023, 12:22.

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 Diffcashtab-tests
Build 50172: arc lint + arc unit

Event Timeline

emack requested review of this revision.Oct 7 2023, 12:22
emack planned changes to this revision.Oct 7 2023, 12:22
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

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 marked 3 inline comments as done.

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.

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.

Pls note the state var cleanup is via D14624.

Rebased to master after state var cleanup

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 marked 3 inline comments as done.

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.

image.png (165×669 px, 13 KB)

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.

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 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

image.png (479×563 px, 46 KB)

Registering test11 which has two other pending txs onchain

image.png (475×575 px, 51 KB)

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

image.png (542×571 px, 65 KB)

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.

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

image.png (248×542 px, 24 KB)

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

image.png (252×542 px, 21 KB)

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

image.png (360×581 px, 40 KB)

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 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
emack marked 3 inline comments as done.

Improved pending array filter and variable names.

bytesofman requested changes to this revision.Nov 1 2023, 17:29

image.png (266×568 px, 28 KB)

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

image.png (266×568 px, 26 KB)

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
This revision was automatically updated to reflect the committed changes.