Details

Reviewers

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 26311
Build 52192: Build Diff	cashtab-tests
Build 52191: 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	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	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

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.

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.

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

cashtab/src/utils/validation.js
81–82	We need two functions because we are validating two different things. On the Alias.js screen, validate that the input characters are all valid for a v0 alias. (`isValidAlias`) 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	This case is caught by `isValidAlias`

bytesofman requested review of this revision.Jan 15 2024, 18:33

bytesofman marked 2 inline comments as done.

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

on second thought, should just simplify this now

isValidAlias --> meetsAliasSpec

need to do integration tests and update test plan

Harbormaster completed remote builds in B26373: Diff 44198.Jan 15 2024, 19:26

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)

fix condition in signverifymsg, disable aliases

snapshot update after re-disabling aliases

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

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

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.

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 44121

cashtab/src/components/Alias/Alias.js

cashtab/src/components/Send/SendToken.js

cashtab/src/components/Send/SendXec.js

cashtab/src/components/SignVerifyMsg/SignVerifyMsg.js

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

cashtab/src/utils/fixtures/vectors.js

cashtab/src/utils/validation.js

[Cashtab] Improve isAliasFormat function and add unit testsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 44121

cashtab/src/components/Alias/Alias.js

cashtab/src/components/Send/SendToken.js

cashtab/src/components/Send/SendXec.js

cashtab/src/components/SignVerifyMsg/SignVerifyMsg.js

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

cashtab/src/utils/fixtures/vectors.js

cashtab/src/utils/validation.js

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

Revision Contents
Changeset List

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