Paths

Table of Contentst

Differential D13301

[Cashtab] [Alias] Prevent unconfirmed registrations from resolving and rendering
AbandonedPublic
Actions

Authored by emack on Mar 13 2023, 06:23.

Details

Reviewers

bytesofman

Group Reviewers

Restricted Project

Summary

T2992

This diff updates the getAliasAndAddresses function to skip over unconfirmed alias transactions and updates processChronikWsMsg to trigger a full alias tx history retrieval whenever a new block is found. This ensures whichever pending registration that was previously skipped will be committed to cache if they have received 1 confirmation via the newly found block.

A new blockheight param is added into aliasCache.aliases and it migrates wallets without this. Reserved aliases are set to blockheight of 1 whilst unconfirmed txs blockheight are set to 100000000 as per the arbitrary figure used in alias-server.

Registration broadcast notification message updated to clarify the need for aliases to receive 1 conf prior to usage.

Note: with this approach the user can technically keep re-registering the same alias after the first registration broadcast whilst it's still unconfirmed. I can push a subsequent diff that disables this with a lookup of pending alias transactions.

Test Plan

enable alias in ticker.js
npm start
wait for a new block to be found and observe the New block found, resetting aliasCache with a full alias tx history retrieval message in console log and the subsequent full tx history retrieval
register a new alias, refresh a few times and ensure this new alias is not displayed in Registered Aliases and does not resolve to an address in send/sendToken screens
wait for another new block to be found, then verify this alias is now displayed in Registered Aliases and resolves to the correct address in send/sendToken screens
send XEC to this alias via send.js
send etoken to this alias via sendToken.js

Diff Detail

Repository

rABC Bitcoin ABC

Branch

sameBlockRego

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 22423
Build 44472: Build Diff	cashtab-tests
Build 44471: arc lint + arc unit

Event Timeline

emack created this revision.Mar 13 2023, 06:23

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 13 2023, 06:23

emack requested review of this revision.Mar 13 2023, 06:23

Harbormaster completed remote builds in B22390: Diff 38474.Mar 13 2023, 06:26

Needs some cleaning up but this is the right approach.

We might as well implement the pendingAliases array in cache before going to prod (not in this diff). We are already handling the logic of recognizing these aliases. Might as well put them in their own array instead of simply excluding them.

web/cashtab/src/hooks/useWallet.js
280 ↗	(On Diff #38474)	add a check to make sure `cashtabCache.aliasCache.aliases` is iterable before doing this.
912 ↗	(On Diff #38474)	Shouldn't this be an or? i.e. `(type !== ('AddedToMempool' \|\| 'BlockConnected'))` For clarity, please define the cases separately. i.e. if (type !== 'AddedToMempool` \|\| type !== 'BlockConnected') {}
927 ↗	(On Diff #38474)	This triggers an error. It still 'works' but should be handled cleanly.
928 ↗	(On Diff #38474)	if `return` is here, it probably should also be in the `catch` block. Do you still want the rest of the function to execute on an error?

This revision now requires changes to proceed.Mar 13 2023, 17:01

emack marked 4 inline comments as done.Mar 14 2023, 03:36

emack added inline comments.

web/cashtab/src/hooks/useWallet.js
912 ↗	(On Diff #38474)	Shouldn't this be an or? `&&` is the right operator here otherwise a BlockConnected message will hit this and it will technically match the first part (!== 'AddedToMempool`) and since it's an OR operator it is enough to return out of there. The `&&` is need to only catch messages that are not both of these events.

added check to ensure cashtabCache.aliasCache.aliases is iterable
getAliasFromLocalCache no longer failing successfully in console log due to the above check
adjusted positioning of return statement in processChronikWsMsg

Harbormaster completed remote builds in B22423: Diff 38529.Mar 14 2023, 04:11

bytesofman requested changes to this revision.Mar 14 2023, 04:22

bytesofman added inline comments.

web/cashtab/src/hooks/useWallet.js
295	what's actually being returned here if `invalidAliasFound = false` and `!Array.isArray(cashtabCache.aliasCache.aliases)? Is the issue that `cashtabCache.aliasCache.aliases` may be undefined? If that's the case, should be specifically handled. It's a hard mental hurdle for a new dev looking at this code to understand why `invalidAliasFound = false` here, but we still want to keep the input.

This revision now requires changes to proceed.Mar 14 2023, 04:22

emack requested review of this revision.Mar 14 2023, 05:40

emack added inline comments.

web/cashtab/src/hooks/useWallet.js
295	if `Array.isArray(cashtabCache.aliasCache.aliases` is not an array or is undefined, then setting `invalidAliasFound = false` means it will trigger a full tx history retrieval in the following line: cachedAliases = invalidAliasFound ? [] : cashtabCache.aliasCache; i.e. if cache is malformed, nuke it and return an empty aliasCache array so a full tx history will be executed upon loading of the Home/Alias screens.

bytesofman requested changes to this revision.Mar 14 2023, 11:51

bytesofman added inline comments.

web/cashtab/src/hooks/useWallet.js
295	isn't the opposite happening? Right now, `if (!Array.isArray(cashtabCache.aliasCache.aliases))` -- i.e. if you hit the `else` loop of if (Array.isArray(cashtabCache.aliasCache.aliases)) --> you get `invalidAliasFound = false;` If `invalidAliasFound = false`, then `cachedAliases` is being set to `cashtabCache.aliasCache` by line 297 (this only returns an empty array `[]` if `invalidAliasFound ?` i.e. if `invalidAliasFound === true` So, if `cashtabCache.aliasCache.aliases` is malformed, the function ends up `return`ing `cashtabCache.aliasCache` instead of `[]` as intended. This check should be moved to the existing `isValidCashtabCache` function in `validation.js`, then also modify `parseInvalidCashtabCacheForMigration` so that, in the event of a non alphanumeric alias in the cache, it resets just that cache element.

This revision now requires changes to proceed.Mar 14 2023, 11:51

array check moved out of useWallet and since it was already in isValidCashtabCache no changes needed there
parseInvalidCashtabCacheForMigration updated to parse for non-alphanumeric and unconfirmed alias transactions and if found, resets the aliasCache to an empty array and trigger a downstream full tx history retrieval

Harbormaster completed remote builds in B22476: Diff 38611.Mar 16 2023, 06:05

Seems like this is obsoleted if we're just going to get validAliases and pendingAliases from alias-server. Is there anything in this diff we want to keep before that implementation? Mb the notification?

Either way, probably cleanest to abandon this and start working on server implementation.

This revision now requires changes to proceed.Mar 16 2023, 11:46

emack abandoned this revision.Mar 16 2023, 12:14

emack mentioned this in D13367: [Cashtab] [Alias-server] - pt 1 - Alias server api integration.Mar 17 2023, 02:39

emack mentioned this in rABC7072dcbe66dc: [Cashtab] [Alias-server] - pt 1 - Alias server api integration.Mar 27 2023, 00:59