Page MenuHomePhabricator
Differential D15155

[Cashtab] Improve isAliasFormat function and add unit tests
ClosedPublic
Actions

Authored by bytesofman on Jan 12 2024, 21:12.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABCace3cd483a4e: [Cashtab] Improve isAliasFormat function and add unit tests
Summary

Alias validation evolved incrementally with the alias spec. This has left overcomplicated validation functions and some missed validation cases.

Fix this.

Function one: meetsAliasSpec

Does a given string meet the spec for registration as an alias?

Function two: isValidAliasSendInput -- same as function one, except it must also end with a .xec suffix.

Add unit tests for both cases. Since these functions are already used in the app, implement the new functions.

Test Plan

npm test

grep -r isAliasFormat src/ and no output

Edit config/alias.js to set aliasEnabled to true

npm start

Navigate to alias screen and test

  • valid alias input
  • non-alphanumeric alias input
  • valid characters but too many bytes alias input

and verify correct error msgs are displayed

Verify that attempting to enter a valid alias with '.xec' suffix does not pass validation

Navigate to Send, SendToken screens and test

  • valid alias input with .xec suffix
  • invalid address gives error 'invalid address'
  • invalid alias gives error 'invalid address' (note: we will add better error msgs here in the parseAddressForParams upgrade

Navigate to SignVerifyMsg screen

  • valid alias input gives error aliases not supported for signature verifications
  • Invalid input (alias or addr) gives 'invalid xec address' error

Diff Detail

Repository
rABC Bitcoin ABC
Branch
refactor-isaliasformat
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26379
Build 52328: Build Diffcashtab-tests
Build 52327: arc lint + arc unit

Event Timeline

bytesofman created this revision.Jan 12 2024, 21:12
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 12 2024, 21:12
bytesofman published this revision for review.Jan 12 2024, 21:14
Harbormaster completed remote builds in B26311: Diff 44121.Jan 12 2024, 21:15
emack requested changes to this revision.Jan 14 2024, 11:07
emack added a subscriber: emack.
emack added inline comments.
cashtab/src/utils/validation.js
81–82 ↗(On Diff #44121)

For consistency these two conditions should be in isValidAlias so it's all in the one place. In which case, why do we need this additional function? Can't you just move the splitting logic into isValidAlias as well?

82 ↗(On Diff #44121)

Needs alias.trim() !== '' to cover the scenario of empty spaces followed by .xec

This revision now requires changes to proceed.Jan 14 2024, 11:07
emack added a comment.EditedJan 14 2024, 11:26

On a semi related note, not entering any address input, with a valid send amount, clicking send spits out this error msg that sounds like it's passed directly from a library, which could use a bit of a non-technical tailoring via a separate diff.

image.png (409×1 px, 81 KB)

bytesofman marked an inline comment as done.Jan 15 2024, 18:33
In D15155#340481, @emack wrote:

On a semi related note, not entering any address input, with a valid send amount, clicking send spits out this error msg that sounds like it's passed directly from a library, which could use a bit of a non-technical tailoring via a separate diff.

image.png (409×1 px, 81 KB)

yeah this can be improved but is not related to the current diff.

cashtab/src/utils/validation.js
81–82 ↗(On Diff #44121)

We need two functions because we are validating two different things.

  1. On the Alias.js screen, validate that the input characters are all valid for a v0 alias. (isValidAlias)
  2. On the SendXec and SendToken screens, validate that the user input is a valid alias and ends with .xec (isValidAliasSendInput)

I'll get a diff in that renames isValidAlias, since it's not really doing that. It is validating whether the input characters are valid. It should be called containsAliasCharacters.

The length validation for case (1) above is handled by Alias.js. Arguably, we could have the function handle both -- but if we go that way, we need the function to return string or throw new Error() depending on the error, so that the screen knows which validation error to throw. It's an available optimization.

Can't you just move the splitting logic into isValidAlias as well?

For the alias create input, we specifically do not want the user to include .xec

There is room for optimization here. But I don't think we are introducing technical debt. Just cleaning up the existing situation and fixing the unit tests.

With these diffs, I'm not looking for optimization. I just need the functions cleaned up enough so that I can use them in the refactor of parseAddressForParams. While I'm at it, I'm upgrading the unit tests to use the improved vectors approach.

In this case -- this diff fixes a bug, since Cashtab currently only catches "too long" .xec alias inputs when the request is rejected by the server.

82 ↗(On Diff #44121)

This case is caught by isValidAlias

bytesofman requested review of this revision.Jan 15 2024, 18:33
bytesofman marked 2 inline comments as done.
bytesofman updated this revision to Diff 44197.Jan 15 2024, 18:34

rebase

Harbormaster completed remote builds in B26372: Diff 44197.Jan 15 2024, 18:36
bytesofman planned changes to this revision.Jan 15 2024, 18:37
bytesofman added a comment.Jan 15 2024, 18:40

on second thought, should just simplify this now

bytesofman updated this revision to Diff 44198.Jan 15 2024, 19:24

isValidAlias --> meetsAliasSpec

bytesofman planned changes to this revision.Jan 15 2024, 19:24

need to do integration tests and update test plan

Harbormaster completed remote builds in B26373: Diff 44198.Jan 15 2024, 19:26
bytesofman updated this revision to Diff 44204.Jan 15 2024, 21:48

pass error msgs to isvalidaliassendinput

bytesofman edited the test plan for this revision. (Show Details)Jan 15 2024, 21:52
bytesofman edited the summary of this revision. (Show Details)
bytesofman updated this revision to Diff 44205.Jan 15 2024, 21:55

fix condition in signverifymsg, disable aliases

bytesofman updated this revision to Diff 44206.Jan 15 2024, 21:56

snapshot update after re-disabling aliases

bytesofman updated this revision to Diff 44207.Jan 15 2024, 21:58

back out initial refactor of isValidRecipient now that you have refactored isValidAliasSendInput

bytesofman updated this revision to Diff 44208.Jan 15 2024, 22:01

update type in params, standardize screen implementation

Harbormaster completed remote builds in B26377: Diff 44204.Jan 15 2024, 22:05
Harbormaster completed remote builds in B26378: Diff 44205.Jan 15 2024, 22:07
teamcity added a comment.Jan 15 2024, 22:07
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Failed tests logs:

====== CashTab Unit Tests:  Wallet with XEC balances and tokens and state field ======
Error: expect(received).toMatchSnapshot()

Snapshot name: `Wallet with XEC balances and tokens and state field 1`

- Snapshot  - 1
+ Received  + 1

@@ -94,11 +94,11 @@
                              name="address"
                              onBlur={[Function]}
                              onChange={[Function]}
                              onFocus={[Function]}
                              onKeyDown={[Function]}
-                             placeholder="Address or Alias"
+                             placeholder="Address"
                              required={true}
                              style={null}
                              type="text"
                              value=""
                            />
    at Object.toMatchSnapshot (/work/cashtab/src/components/Send/__tests__/SendToken.test.js:67:18)
    at Promise.then.completed (/work/cashtab/node_modules/jest-circus/build/utils.js:298:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/work/cashtab/node_modules/jest-circus/build/utils.js:231:10)
    at _callCircusTest (/work/cashtab/node_modules/jest-circus/build/run.js:316:40)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at _runTest (/work/cashtab/node_modules/jest-circus/build/run.js:252:3)
    at _runTestsForDescribeBlock (/work/cashtab/node_modules/jest-circus/build/run.js:126:9)
    at run (/work/cashtab/node_modules/jest-circus/build/run.js:71:3)
    at runAndTransformResultsToJestFormat (/work/cashtab/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
    at jestAdapter (/work/cashtab/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
    at runTestInternal (/work/cashtab/node_modules/jest-runner/build/runTest.js:367:16)
    at runTest (/work/cashtab/node_modules/jest-runner/build/runTest.js:444:34)
    at Object.worker (/work/cashtab/node_modules/jest-runner/build/testWorker.js:106:12)

Each failure log is accessible here:
CashTab Unit Tests: Wallet with XEC balances and tokens and state field

Harbormaster completed remote builds in B26379: Diff 44206.Jan 15 2024, 22:08
bytesofman added inline comments.Jan 15 2024, 22:08
cashtab/src/components/Alias/Alias.js
284 ↗(On Diff #44208)

this code is unreachable after later iterations on the Alias.js screen, because the validation will prevent registerAlias from being called if the user has entered with .xec

Harbormaster completed remote builds in B26380: Diff 44207.Jan 15 2024, 22:10
Harbormaster completed remote builds in B26381: Diff 44208.
emack accepted this revision.Jan 16 2024, 00:53
This revision is now accepted and ready to land.Jan 16 2024, 00:53
Closed by commit rABCace3cd483a4e: [Cashtab] Improve isAliasFormat function and add unit tests (authored by bytesofman). · Explain WhyJan 16 2024, 14:08
This revision was automatically updated to reflect the committed changes.
bytesofman added a commit: rABCace3cd483a4e: [Cashtab] Improve isAliasFormat function and add unit tests.

Revision Contents
Changeset List

PathSize
cashtab/
src/
components/
Alias/
31 lines
Send/
6 lines
4 lines
SignVerifyMsg/
4 lines
opreturn/
fixtures/
3 lines
7 lines
utils/
__tests__/
25 lines
fixtures/
116 lines
54 lines

Diff 44206

View Options

cashtab/src/components/Alias/Alias.js

Loading...
View Options

cashtab/src/components/Send/SendToken.js

Loading...
View Options

cashtab/src/components/Send/SendXec.js

Loading...
View Options

cashtab/src/components/SignVerifyMsg/SignVerifyMsg.js

Loading...
View Options

cashtab/src/opreturn/fixtures/vectors.js

Loading...
View Options

cashtab/src/opreturn/index.js

Loading...
View Options

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

Loading...
View Options

cashtab/src/utils/fixtures/vectors.js

Loading...
View Options

cashtab/src/utils/validation.js

Loading...