Details

Reviewers

bytesofman
Fabien

Group Reviewers

Restricted Project

Summary

T3216

Following the deprecation of the pending alias concept, when the user broadcasts an alias registration tx the subsequent UX is a bit janky in that they won't know whether they were successful in registering the alias without refreshing their cashtab. This is particularly pronounced when the next block takes > 10 mins to be found, forcing users to frequently check the explorer and flood the telegram for status checks.

Hence this diff adds a dedicated ws listener to the end of registerNewAlias() and then once 1 conf is received on the registration tx the user is notified via the UI to check the Alias page.

This diff isn't using the websocket in useWallet.js because at the point of app startup it doesn't have the necessary registration info (txid, address...etc) for this tx subscription. Thus implementing this websocket at the registerNewAlias() callsite.

Edge Case:
There'll be a subsequent diff which will be triggered upon this websocket confirmation event and will check whether the registration was successful or not. This is to improve the UX in the case where multiple users are registering the same alias in the same block and there can only be one successful user. This may be an edge case in the long term but during the initial days post-launch there is a high chance of users all trying to register valuable aliases e.g. apple.xec.

Test Plan

npm test
enable alias flag in Ticker.js
npm start
register a new alias
wait for a new block to be found and ensure the 1 conf pop up notification is displayed in cashtab
check the Alias page and verify new alias is listed under the registered aliases section

Diff Detail

Repository

rABC Bitcoin ABC

Branch

1confAlias

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 24652
Build 48900: Build Diff	cashtab-tests
Build 48899: arc lint + arc unit

Event Timeline

emack created this revision.Jul 30 2023, 03:42

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 30 2023, 03:42

emack requested review of this revision.Jul 30 2023, 03:42

emack edited the summary of this revision. (Show Details)Jul 30 2023, 03:44

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

Harbormaster completed remote builds in B24624: Diff 41637.Jul 30 2023, 03:47

You should split the mock refactor out of this diff, this makes it very noisy.

Is there a status list somewhere in addition to the popup ? Like when registering my alias it's pending, and after it's validated it turns valid ?
Otherwise it's not helpful for users that don't stay connected.

This revision now requires changes to proceed.Jul 31 2023, 09:13

In D14325#320825, @Fabien wrote:

You should split the mock refactor out of this diff, this makes it very noisy.

If you're referring to the changes to transactions.test.js, those are necessary in this same diff now that txConfListener will sit there reconnecting to check for a confirmed ws event. Without the mock update, after all tests have completed it will spiral into an infinite async loop waiting for a confirmed event that would never come.

Is there a status list somewhere in addition to the popup ? Like when registering my alias it's pending, and after it's validated it turns valid ?
Otherwise it's not helpful for users that don't stay connected.

That was the intention of the pending alias feature. User sends off a register alias tx, and the Alias.js component calls upon alias-server for a list of registered and pending aliases which will be rendered accordingly in the frontend. Having revisited the post-broadcast UX I think it's still worthwhile to have this pending concept. @bytesofman already had the API running on the old dev server so it should be straightforward to port over to the current alias-server if this is the approach we're taking.

In D14325#320829, @emack wrote:

In D14325#320825, @Fabien wrote:

You should split the mock refactor out of this diff, this makes it very noisy.

If you're referring to the changes to transactions.test.js, those are necessary in this same diff now that txConfListener will sit there reconnecting to check for a confirmed ws event. Without the mock update, after all tests have completed it will spiral into an infinite async loop waiting for a confirmed event that would never come.

I'm talking about the MockChronikClient class

Is there a status list somewhere in addition to the popup ? Like when registering my alias it's pending, and after it's validated it turns valid ?
Otherwise it's not helpful for users that don't stay connected.

That was the intention of the pending alias feature. User sends off a register alias tx, and the Alias.js component calls upon alias-server for a list of registered and pending aliases which will be rendered accordingly in the frontend. Having revisited the post-broadcast UX I think it's still worthwhile to have this pending concept. @bytesofman already had the API running on the old dev server so it should be straightforward to port over to the current alias-server if this is the approach we're taking.

You can have a pending list from local storage without the need for the pending alias support from the mempool. This is just the way you show the data, no need for server-side pending alias management.

I'm talking about the MockChronikClient class

Without the MockChronikClient class you won't be able to trigger a mock Confirmed ws event, which leads to the infinite async loop of txConfListener() reconnecting yearning for a confirmation after npm test has concluded.

pending list from local storage without the need for the pending alias support from the mempool.

Ok that might work, will do some thinking around this in a separate diff.

In D14325#320857, @emack wrote:

I'm talking about the MockChronikClient class

Without the MockChronikClient class you won't be able to trigger a mock Confirmed ws event, which leads to the infinite async loop of txConfListener() reconnecting yearning for a confirmation after npm test has concluded.

Sure, so just do it prior to this diff

bytesofman requested changes to this revision.Jul 31 2023, 16:48

bytesofman added inline comments.

cashtab/src/utils/chronik.js
27 ↗	(On Diff #41637)	the address should be accepted as a param then call `ecashaddr.decode(address, true)` to get `type, hash` , which you can use instead of `p2pkh, hash160` in this function also means you don't call the we-need-to-deprecate-it `toHash160` function
cashtab/src/utils/transactions.js
418 ↗	(On Diff #41637)	we want to use the updated `ecashaddrjs` method inside the function, (`.decode, true` to get type and hash ready for chronik) not the `toHash160` function, which should be deprecated from Cashtab

This revision now requires changes to proceed.Jul 31 2023, 16:48

emack mentioned this in D14330: [Cashtab][Alias] Add chronik mock.Jul 31 2023, 23:07

In D14325#320858, @Fabien wrote:

In D14325#320857, @emack wrote:

I'm talking about the MockChronikClient class

Without the MockChronikClient class you won't be able to trigger a mock Confirmed ws event, which leads to the infinite async loop of txConfListener() reconnecting yearning for a confirmation after npm test has concluded.

Sure, so just do it prior to this diff

D14330

emack mentioned this in rABC144e5d653d92: [Cashtab][Alias] Add chronik mock.Aug 1 2023, 23:14

rebased to D14330 chronik mock
updated to use the latest ecashaddrjs in txConfListener
added ws/script mock in chronikMock.js
updated unit tests to align with the address input
local storage-based pending alias tracking will be implemented in a separate diff

Harbormaster completed remote builds in B24651: Diff 41671.Aug 2 2023, 00:55

Updated mockResponses

Harbormaster completed remote builds in B24652: Diff 41672.Aug 2 2023, 01:03

bytesofman requested changes to this revision.Aug 2 2023, 16:57

bytesofman added inline comments.

cashtab/src/utils/__mocks__/chronikMock.js
17–28	doesn't seem like these are being used in this diff? can remove any changes here unrelated, goal was to include everything we needed in the separate diff
36	should have a `close` method here, with flag to show it is called in unit tests
cashtab/src/utils/__tests__/chronik.test.js
79	why construct with `address` ? should just take the default fake url no?

This revision now requires changes to proceed.Aug 2 2023, 16:57

Responding to feedback

cashtab/src/utils/__mocks__/chronikMock.js
17–28	self.mockResponses is used by self.setMock down the bottom, which is needed to set the ws mock event. I've removed self.mockedMethods and self.script.
36	Added
cashtab/src/utils/__tests__/chronik.test.js
79	Corrected

Harbormaster completed remote builds in B24670: Diff 41696.Aug 3 2023, 13:45

An issue I run into with testing this

Wait for the notification
Get distracted bc blocks come at irregular intervals
Notification happens but I never see it

This happens most of the time, and defeats the purpose of the notification in the first place. I don't think we can expect users to constantly stare at Cashtab for ~10 minutes. Makes this diff slightly more complicated, but should

Modify generalNotification function to accept duration as a param (default to notificationDurationShort)
Pass 0 for this param in the call from txConfListener, so that this particular notification must be closed manually.

This revision now requires changes to proceed.Aug 3 2023, 17:01

Updated generalNotification to take in a duration param set to notificationDurationShort by default, but can be overriden to 0 to indicate permanent notification until closed by the user.

Harbormaster completed remote builds in B24679: Diff 41710.Aug 4 2023, 02:00

bytesofman accepted this revision.Aug 8 2023, 22:55

See comment in D14356, I don't think this is useful. Notification upon block for an event that isn't block related is a non sense as well, you should notify when the alias state changed.

This revision now requires changes to proceed.Aug 9 2023, 08:48

Superseded by updates to pending logic in D14356

[Cashtab][Alias] Add websocket listener for 1 conf on alias registration txs
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 41672

cashtab/package-lock.json

cashtab/package.json

cashtab/src/utils/mocks/chronikMock.js

cashtab/src/utils/tests/chronik.test.js

cashtab/src/utils/tests/transactions.test.js

cashtab/src/utils/chronik.js

cashtab/src/utils/transactions.js

[Cashtab][Alias] Add websocket listener for 1 conf on alias registration txsAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 41672

cashtab/package-lock.json

cashtab/package.json

cashtab/src/utils/__mocks__/chronikMock.js

cashtab/src/utils/__tests__/chronik.test.js

cashtab/src/utils/__tests__/transactions.test.js

cashtab/src/utils/chronik.js

cashtab/src/utils/transactions.js

[Cashtab][Alias] Add websocket listener for 1 conf on alias registration txs
AbandonedPublic
Actions

Revision Contents
Changeset List

cashtab/src/utils/mocks/chronikMock.js

cashtab/src/utils/tests/chronik.test.js

cashtab/src/utils/tests/transactions.test.js