Page MenuHomePhabricator

[Cashtab][Alias] Refactor Alias.js to use new alias-server endpoints
ClosedPublic

Authored by emack on Jul 5 2023, 01:44.

Details

Summary

T3216

This diff refactors Alias.js to connect to the new alias-server endpoints to check:

  • alias availability
  • aliases registered to the active wallet

These were not pushed as two separate diffs because they are dependent on each other e.g. once an alias is confirmed to be available and is then registered, the reviewer needs the 'aliases registered to address' component to be hooked up to alias-server at the same time to verify the registration on the Alias.js page.

Test Plan
  • npm test
  • change alias flag in Ticker.js to true
  • npm start
  • navigate to the Alias screen and ensure existing registered aliases for this address are displayed
  • register a new alias that is available and ensure successful broadcast
  • wait for one confirmation and then refresh the page and verify registered alias is now displayed on Alias.js
  • verify registration status matches alias-server's alias endpoint
  • verify newly registered alias is recognized via alias-server's address endpoint
  • attempt to register an alias that is already confirmed and verify registration error

Diff Detail

Repository
rABC Bitcoin ABC
Branch
refactorAliasJs
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 24522
Build 48643: Build Diffcashtab-tests
Build 48642: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

note: we will not be launching with pendingAlias feature in the server

bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
84 ↗(On Diff #41341)

we need to rethink how we are using this state param. in this diff, we have no way of setting it back to true

for now -- this param should just be dropped. We get the goal of having registrations disabled if server is down by failing to authenticate alias availability.

imo action here is

  1. In a separate diff, remove isAliasServerOnline from everywhere in the app
  2. Rebase this diff onto that one, not including any new use of isAliasServerOnline
This revision now requires changes to proceed.Jul 10 2023, 21:20

To be rebased once D14246 lands

cashtab/src/components/Common/Ticker.js
12 ↗(On Diff #41255)

still in use for alias registration transactions

emack edited the summary of this revision. (Show Details)
emack marked an inline comment as done.

Rebased to D14246

emack planned changes to this revision.Jul 12 2023, 03:20

Updated pre-registration to check for aliasDetails.error

Fabien added inline comments.
cashtab/src/components/Alias/Alias.js
132 ↗(On Diff #41403)

You don't need the else

191 ↗(On Diff #41403)

dito

emack marked 2 inline comments as done.

Updated conditional statements

bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
131 ↗(On Diff #41430)

We don't really want to throw errors in this loop ... this just prints errors to the dev console.

It's probably easiest to simply lock the UI of the Alias.js screen until we get a good response from the server, similar to the way we lock the UI of Send.js during a chronik error.

We can't expect the user to see errors in the dev console, but we don't want them to be able to interact with the Alias screen if the server is down.

This revision now requires changes to proceed.Jul 13 2023, 17:02
cashtab/src/components/Alias/Alias.js
131 ↗(On Diff #41430)

loop useEffect block

emack marked 2 inline comments as done.
  • updated error handling to lock UI with UI notification
  • removed redoondant exception check for server unavailability
  • aligned error handling in registerAlias
bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
144 ↗(On Diff #41442)

We don't want to lock the UI if there is no way for it to become unlocked

Looking at this functionality, imo the best option for handling errors is

  1. Error notification
  2. Disable ability to register aliases
  3. Instead of displaying registered aliases of address, display some kind of warning that server contact failed

As long as alias registration txs are disabled when the server is unavailable, keeping the UI unlocked is okay.

This revision now requires changes to proceed.Jul 14 2023, 17:08
emack marked an inline comment as done.

Updated error handling per feedback

bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
148 ↗(On Diff #41453)

won't this just become true again if the input changes? Need to make the alias registration button also dependent on server errors, not just valid input.

This revision now requires changes to proceed.Jul 17 2023, 17:01
emack marked an inline comment as done.

Perma-disables Register Alias button upon server outage, even after new inputs

bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
98

We don't need this state field. disabled prop of the button should depend on aliasServerError state, disabled if aliasServerError is not ''

Going forward -- we should probably run this useEffect block every time a new block is found and not just on page load. That way we would get confirmed alias registrations in real time. But, we don't need that for launching.

Also an option to implement a websocket in alias-server and have Cashtab subscribe directly to the websocket.

This revision now requires changes to proceed.Jul 18 2023, 23:32
emack marked an inline comment as done.
  • Removed registerAliasButton state var and updated the register alias button's disabled prop to depend on aliasServerError
  • Default value of aliasServerError changed from '' to false to simplify usage

Will implement websocket in separate diff

  • rename aliasServerBaseUrl in ticker.js to something that isn't correct
  • navigate to alias page
  • error throws
  • however if you type in an alias, the button is not disabled (registering won't work...but we want the button to be greyed out)

image.png (555×541 px, 44 KB)

This revision now requires changes to proceed.Jul 21 2023, 17:47

Disabled register alias button upon server error regardless of new inputs

bytesofman added inline comments.
cashtab/src/components/Alias/Alias.js
253–256 ↗(On Diff #41554)

this is not the correct approach here. the button should be disabled or not based on aliasServerError state field

506 ↗(On Diff #41554)

This works when aliasServerError is false. However this condition is not picking up when aliasServerError is a string.

To resolve issue from my last comment (if no response from alias server, registration button is still not disabled if alias input is valid) -- need to make this check more specific

This revision now requires changes to proceed.Jul 24 2023, 17:31
emack marked 2 inline comments as done.Jul 25 2023, 04:09
emack added inline comments.
cashtab/src/components/Alias/Alias.js
253–256 ↗(On Diff #41554)

Removed, as it's not needed after applying more specific checks on the button

506 ↗(On Diff #41554)

Updated

emack marked 2 inline comments as done.

Responding to feedback

This revision is now accepted and ready to land.Jul 25 2023, 18:17