Details

Reviewers

bytesofman
Fabien

Group Reviewers

Restricted Project

Summary

T3216

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 latestAliases, 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 a new block to be found, verify the alias has now moved from the Pending Aliases to the Registered Aliases dropdown
Register another alias, close Cashtab. When the next block has been found 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 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

pendingAlias

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25143
Build 49874: Build Diff	cashtab-tests
Build 49873: arc lint + arc unit

Event Timeline

emack created this revision.Aug 12 2023, 13:28

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 12 2023, 13:28

emack requested review of this revision.Aug 12 2023, 13:28

Harbormaster completed remote builds in B24750: Diff 41806.Aug 12 2023, 13:31

Hm, should actually go in wallet state, since we are only tracking the pending aliases for the active wallet. If we do it in cashtabCache, we will need to track them for all wallets. It's doable but at this point we would be turning Cashtab into a Rube Goldberg machine, all to implement something that should be a basic part of alias implementation. Looking at this diff and the one preceding it -- I think it's also fair to say we can't really expect potential third party implementers to support this kind of feature.

@Fabien what do you think about modifying the previous /pending/ endpoint of alias-server to /pending/<address>? We don't need to use it to prevent a wallet from registering a "pending" alias -- but it would greatly simplify the problem of "we would like to keep our users from accidentally double registering the same alias"

The original intent of alias-server was to prevent applications from needing to cache and validate on-chain information. Without this endpoint, we need to locally cache pendingAliases (for all user wallets), which, while possible, imo complicates the app in a way that outweights the expected benefit.

When updatePendingAliasArrayInLocalForage is called in useWallet's 30 sec interval checks to update cashtabCache, it doesn't translate to an automatic state refresh for Alias.js even though cashtabCache is a dependency in Alias.js' useEffect() function. I also tried with cashtabCache.pendingAliases as well to no avail.

this is because the useEffect dependency cashtabCache is a state variable from wallet context, but this diff is only updating localforage cashtabCache. Hence the state variable is never changing and will not trigger the useEffect loop, unless the wallet happens to receive an incoming token transaction from a previously unseen tokenId while the diff is being tested (to really round out the Rube Goldberg machine).

emack edited the summary of this revision. (Show Details)Aug 13 2023, 02:41

Fixed cashtabCache refresh trigger dependency by also updating the cashtabCache state variable in useWallet, which now successfully refreshes the Alias.js component on new alias registrations and changes to pending aliases list
Added notification on successful or unsuccessful alias registeration in useWallet's pendingAliasCheck function

should actually go in wallet state

Will wait to see the decision around potentially moving pending logic to alias-server before making this change

Harbormaster completed remote builds in B24755: Diff 41811.Aug 13 2023, 03:22

@Fabien what do you think about modifying the previous /pending/ endpoint of alias-server to /pending/<address>? We don't need to use it to prevent a wallet from registering a "pending" alias -- but it would greatly simplify the problem of "we would like to keep our users from accidentally double registering the same alias"

Are you suggesting to rely on a remote server to prevent the users of your app to do something with your app that you don't want them to do ?

Tracking pending aliases on server side has its can of worms too. One simple example: how would you deal with a registration from cashtab not showing as pending on the server (think mempool is full for example) ?

In D14376#322316, @Fabien wrote:

@Fabien what do you think about modifying the previous /pending/ endpoint of alias-server to /pending/<address>? We don't need to use it to prevent a wallet from registering a "pending" alias -- but it would greatly simplify the problem of "we would like to keep our users from accidentally double registering the same alias"

Are you suggesting to rely on a remote server to prevent the users of your app to do something with your app that you don't want them to do ?

Tracking pending aliases on server side has its can of worms too. One simple example: how would you deal with a registration from cashtab not showing as pending on the server (think mempool is full for example) ?

In this case, we own the whole stack, so it's reasonable to consider where the best implementation point would be. Worth noting the impact of this change within Cashtab --- for all users all of the time, we are going to constantly check something in the background for an issue that will only be relevant to some users some of the time.

The Cashtab implementation has its own issue as well -- we would be caching locally, but the alias is really "pending" or not depending on the state of the blockchain, not the state of the user's Cashtab local storage. A user might register an alias on one device -- say their phone -- then check the same wallet on their computer, which would have no record of this registration either way. If we had a /pending/address endpoint that was checked only on loading the alias page, imo it would be overall more effective at preventing a user from double-registering -- though neither option is fail safe.

It's not that the server isn't without issues. However they are probably less significant than this local storage problem. Because on-chain activity is the real arbiter here, I don't think we should use local storage for anything other than load time optimization. The alias-server indexer is more optimized to provide this information.

In D14376#322339, @bytesofman wrote:

In D14376#322316, @Fabien wrote:

@Fabien what do you think about modifying the previous /pending/ endpoint of alias-server to /pending/<address>? We don't need to use it to prevent a wallet from registering a "pending" alias -- but it would greatly simplify the problem of "we would like to keep our users from accidentally double registering the same alias"

Are you suggesting to rely on a remote server to prevent the users of your app to do something with your app that you don't want them to do ?

Tracking pending aliases on server side has its can of worms too. One simple example: how would you deal with a registration from cashtab not showing as pending on the server (think mempool is full for example) ?

In this case, we own the whole stack, so it's reasonable to consider where the best implementation point would be. Worth noting the impact of this change within Cashtab --- for all users all of the time, we are going to constantly check something in the background for an issue that will only be relevant to some users some of the time.

The Cashtab implementation has its own issue as well -- we would be caching locally, but the alias is really "pending" or not depending on the state of the blockchain, not the state of the user's Cashtab local storage. A user might register an alias on one device -- say their phone -- then check the same wallet on their computer, which would have no record of this registration either way. If we had a /pending/address endpoint that was checked only on loading the alias page, imo it would be overall more effective at preventing a user from double-registering -- though neither option is fail safe.

It's not that the server isn't without issues. However they are probably less significant than this local storage problem. Because on-chain activity is the real arbiter here, I don't think we should use local storage for anything other than load time optimization. The alias-server indexer is more optimized to provide this information.

If you think that's the way to go then give it a try.

In D14376#322388, @Fabien wrote:

In D14376#322339, @bytesofman wrote:

In D14376#322316, @Fabien wrote:

@Fabien what do you think about modifying the previous /pending/ endpoint of alias-server to /pending/<address>? We don't need to use it to prevent a wallet from registering a "pending" alias -- but it would greatly simplify the problem of "we would like to keep our users from accidentally double registering the same alias"

Are you suggesting to rely on a remote server to prevent the users of your app to do something with your app that you don't want them to do ?

Tracking pending aliases on server side has its can of worms too. One simple example: how would you deal with a registration from cashtab not showing as pending on the server (think mempool is full for example) ?

In this case, we own the whole stack, so it's reasonable to consider where the best implementation point would be. Worth noting the impact of this change within Cashtab --- for all users all of the time, we are going to constantly check something in the background for an issue that will only be relevant to some users some of the time.

The Cashtab implementation has its own issue as well -- we would be caching locally, but the alias is really "pending" or not depending on the state of the blockchain, not the state of the user's Cashtab local storage. A user might register an alias on one device -- say their phone -- then check the same wallet on their computer, which would have no record of this registration either way. If we had a /pending/address endpoint that was checked only on loading the alias page, imo it would be overall more effective at preventing a user from double-registering -- though neither option is fail safe.

It's not that the server isn't without issues. However they are probably less significant than this local storage problem. Because on-chain activity is the real arbiter here, I don't think we should use local storage for anything other than load time optimization. The alias-server indexer is more optimized to provide this information.

If you think that's the way to go then give it a try.

D14201

clearing my queue

This revision now requires changes to proceed.Aug 16 2023, 09:26

Another way to handle this, and one we should probably implement for robustness

The transaction history is available in Cashtab's wallet state. We should have some sort of "on block" function for this in general. Use cases:

Render number of confirmations per tx in tx history
If any alias registration txs in tx history, assume they are pending if unconfirmed
If any confirmed alias registration txs in tx history, check the server to see if they are registered or not

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

emack edited the summary of this revision. (Show Details)Sep 22 2023, 13:46

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

Harbormaster completed remote builds in B25132: Diff 42343.Sep 22 2023, 13:48

It's actually more complicated to implement because a Cashtab user can register aliases for an address that is not their own address.

Managing this complexity is probably not worth it...imo the majority of Cashtab users will not be using it this way, and it is much much harder to deal with this, e.g. what if the user is registering 100 aliases all for different addresses all pending ... basically this will never happen, interesting engineering problem but low impact to implement. Really, only advanced users should be using it this way...I can foresee lots of user confusion from accidentally registering aliases for some other address. It's very difficult to recognize an address you've copy pasted and it's quite common to have some other address on your clipboard when you are working with a wallet (this is the problem aliases are trying to solve).

imo the ability to register aliases for another address should maybe be a gated power-user feature.

For the purposes of this diff, we should:

Show a warning when the user is registering an alias for some other address that Cashtab will not track this pending alias
Make sure to clear all input fields after a registration tx is broadcast.

cashtab/src/components/Alias/Alias.js
115 ↗	(On Diff #42343)	`convertToEcashPrefix` should be deprecated. Only unmigrated very legacy wallets will have `bitcoincash:` prefix at this point.
142 ↗	(On Diff #42343)	this is how they come from the server -- so, no need to do this here.
151 ↗	(On Diff #42343)	the server also does this sort for pending, no need to do it in the app
241–243 ↗	(On Diff #42343)	this should have its own error
cashtab/src/hooks/__tests__/useWallet.test.js
123–130 ↗	(On Diff #42343)	remove
cashtab/src/hooks/useWallet.js
73 ↗	(On Diff #42343)
1415 ↗	(On Diff #42343)	`isValidXecAddress` should also be deprecated. There is an equivalent function in `ecashaddrjs` that will also allow you to specify a prefix. But still -- imo just drop it. I would be curious to see if we get any user feedback of bitcoincash: prefix errors here, since we migrated to ecash: prefixes a long time ago. Easy enough to patch if there is an impact.
1432 ↗	(On Diff #42343)	imo no real benefit to setting a state field in context to either `false` or `string` you can just `setLatestAliases` to `false` if you get an error and work with the one context variable.
1454 ↗	(On Diff #42343)	Overkill for this to just be happening all the time for every Cashtab user. Should happen on opening Cashtab. Should happen if the wallet has any pending alias txs. Otherwise, what are we doing? Aliases don't change, so the result will always be the same. I guess we are checking to see if any other wallet has registered an alias for this wallet -- something that is expected to happen basically never, so we shouldn't be doing this for all Cashtab users all the time.
1456 ↗	(On Diff #42343)
1462 ↗	(On Diff #42343)
1464 ↗	(On Diff #42343)

This revision now requires changes to proceed.Sep 22 2023, 17:06

Added warning about not tracking pending aliases for other wallets as part of the registration confirmation modal if the user is specifying a separate address for the alias
Cleared inputs upon registration broadcast
Fixed a silent bug where refreshAliases() was being constantly triggered in a loop - have moved it into its own useEffect() now with just hasPendingAliases as a dependency
Responding to other feedback as below:

cashtab/src/components/Alias/Alias.js
115 ↗	(On Diff #42343)	Awesome! Didn't realise we got rid of it already. Removed redundant conversion.
142 ↗	(On Diff #42343)	Without this sorting logic here the registered aliases are displayed in unsorted order: I also double checked it at the API call i.e. the `queryAliasServer` call within `refreshAliases` and it came back with an unsorted array: Edit: Also triple checked at the source, the api response is also unsorted. https://alias.etokens.cash/address/ecash:qzvydd4n3lm3xv62cx078nu9rg0e3srmqq0knykfed Hence retaining this sorting logic here.
151 ↗	(On Diff #42343)	As per above.
241–243 ↗	(On Diff #42343)	Updated and also spotted a bug where pendingAliasesDropdown.includes(aliasInput) will always return false because it wasn't comparing the `alias` prop to aliasInput. Using the `some` => `includes` approach which works well now.
cashtab/src/hooks/__tests__/useWallet.test.js
123–130 ↗	(On Diff #42343)	yea true, there shouldn't be a scenario where cashtab wallet object is spitting out invalid ecash addresses. Removed.
cashtab/src/hooks/useWallet.js
73 ↗	(On Diff #42343)	Updated.
1415 ↗	(On Diff #42343)	Ok agreed, removed.
1432 ↗	(On Diff #42343)	The benefit is to have a separate state var for keeping the server error message for rendering in the frontend. Otherwise it becomes messy using the `setAliases` var because I can't set the server error msg to `setAliases` as it would negate the `false` flag, resulting in the need to separately store the error message to be rendered in Alias.js.
1454 ↗	(On Diff #42343)	Consider this use case if the interval is removed: Use broadcasts a registration tx and sits on the Alias.js The tx is confirmed, the pending list length on Alias.js is still > 0 but because there are no interval refreshes, nothing changes The now-confirmed pending alias stays pending forever until the user manually refreshes the page or app, which is not ideal UX So to mitigate this, I've created a `hasPendingAliases` state variable to track the presence of pending aliases in useWallet.js. I'll still retain the interval but it will only fire off the API refresh call if `hasPendingAliases` is true. This pending flag is set to true in `refreshAliases` every time the endpoint returns a pending array with at least 1 entry or in Alias.js right after the registration broadcast.
1456 ↗	(On Diff #42343)	Removed
1462 ↗	(On Diff #42343)	Removed
1464 ↗	(On Diff #42343)	Removed

Harbormaster completed remote builds in B25143: Diff 42354.Sep 24 2023, 02:09

Minor commenting nit

Harbormaster completed remote builds in B25144: Diff 42355.Sep 24 2023, 02:16

bytesofman requested changes to this revision.Sep 25 2023, 20:21

bytesofman added inline comments.

cashtab/src/components/Alias/Alias.js
247	otherwise if you register, e.g. "thisalias", then try to register "thisalia" --> you will get this error even though these are two distinct aliases
284	I'm still not convinced we need this flag. The interval should always exist if `aliases.pending` has length. So -- we could just manually add `aliasInput` to `aliases.pending` here -- which would keep the app checking the server.
cashtab/src/hooks/useWallet.js
76		`hasPendingAliases === aliases.pending.length > 0` ... redundant
1458		This should be set in state so that we can clear it when `aliases.pending` length goes to zero
1454 ↗	(On Diff #42343)	The tx is confirmed, the pending list length on Alias.js is still > 0 but because there are no interval refreshes, nothing changes If `aliases.pending` --- from the API --- has any length, the interval should be live. The server is the source of truth here.

This revision now requires changes to proceed.Sep 25 2023, 20:21

Responding to feedback

cashtab/src/components/Alias/Alias.js
247	Updated per feedback
284	Updated to add `aliasInput` into the alias.pending array and updating it in state. The interval useEffect() in useWallet.js will now trigger based on any changes to `aliases.pending.length`. (note: using aliases or aliases,pending results in an infinite useEffect dependency loop)
cashtab/src/hooks/useWallet.js
76		Removed. All previous references to `hasPendingAliases` will simply check the `aliases.pending` array length.
1458		I replaced the line below `if (hasPendingAliases) {` into `if (aliases.pending.length > 0) {` so it will only execute the API call based on the pending length without needing to go through an extra state var. I also added an extra `await refreshAliases(wallet.Path1899.cashAddress);` call before the interval, so that upon startup it will ensure the pending list is up to date, before the interval starts checking the pending list from API (if there are pending aliases each time). The interval now gets cleared in `refreshAliases` when the pending length from the API is 0.
1454 ↗	(On Diff #42343)	Updated per feedback.

Harbormaster completed remote builds in B25170: Diff 42393.Sep 26 2023, 14:38

some significant improvements available by getting rid of redundant state variables. Might want to think about how you want to implement this. We are getting into a complex review here.

If you think you can get this 100% after another 2 iterations, go for it. Otherwise please abandon and start deprecating redundant state fields to get ready for this implementation.

cashtab/src/components/Alias/Alias.js
99 ↗	(On Diff #42393)	activeWalletAliases === aliases.registered -> sort it where it is rendered, or where it is initially set in wallet context
103 ↗	(On Diff #42393)	pendingAliasesDropdown === aliases.pending sorted -> sort it where it is rendered, or where it is initially set in wallet context
134–149 ↗	(On Diff #42393)	can remove all of this after removing these redundant state variables
150–152 ↗	(On Diff #42393)	looks like a rube goldberg machine here alias server has an error --> set isValidInput flag to false --> disables something, presumably just use aliasServerError where it should be implemented, don't daisy chain these
155 ↗	(On Diff #42393)	can get rid of this whole useEffect block
281–286 ↗	(On Diff #42393)	this doesn't look like the right syntax for modifying a state variable that is an object by adding a new entry to an array stored at one of the keys

This revision now requires changes to proceed.Sep 26 2023, 21:35

emack abandoned this revision.Sep 27 2023, 00:24

emack mentioned this in D14555: [Cashtab][Alias] Pending Aliases.Sep 28 2023, 13:08

[Cashtab][Alias] Pending Aliases
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42354

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 AliasesAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42354

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