T3216
Following the removal of caching-driven registration status checks in D14318, this diff implements this check using the alias-server /alias endpoint.
This API call is only made if the contact address input is parsed to be a valid alias format.
Details
Details
- Reviewers
bytesofman Fabien - Group Reviewers
Restricted Project - Commits
- rABC0130850f2a1d: [Cashtab][Alias] Add alias status check to wallet contacts
- Set alias flag to true in Ticker.js
- npm start
- Navigate to Settings and click on Add Contact. Use an invalid alias input for eCash Address and ensure the Invalid eCash address or alias error is displayed
- Use a valid and registered alias input and ensure it is accepted and saved as a new contact
- Click on the send icon for the valid and registered alias contact from above and ensure successful XEC send tx to this alias
- Use a valid but unregistered alias and ensure the Invalid eCash address or alias error is displayed
- Regression test with a valid eCash address and a valid prefix-less eCash address as input and ensure both passes validation
- Regression test with an invalid eCash/alias address and ensure the Invalid eCash address or alias error is displayed
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| cashtab/src/components/Configure/Configure.js | ||
|---|---|---|
| 1233–1241 ↗ | (On Diff #41636) | |
| 1252 ↗ | (On Diff #41636) | Nit: You can make the code flow easier to read by reversing the condition: if (!isValidAlias) {
setManualContactAddressIsValid(false);
return;
}
[...] |
Comment Actions
Code looks good. Now the elephant in the room:
- There is no test
- How is that in Configure.js ?
Comment Actions
- How is that in Configure.js ?
Configure should probably be renamed to Settings to align with the frontend. (I can throw a diff up for this)
The contact list resides there because:
- Logical grouping of saved wallets alongside saved contacts
- Contacts will eventually be applicable to both Send XEC and Send eToken screens, so it wouldn't work putting it exclusively in either of those send components
- Doesn't belong in the Home (Tx History) nor Receive (QR Code) screens
- There is no test
This is similar to SendToken.js where internal functions (i.e. handleManualContactAddressInput in this case) in a React component (classes that render things) can't be accessed directly. You can only test the initial state and props upon initialization but cases where you're looking to simulate a user input (entering a contact address) and then verifying it's reaction to that input is a challenge with Cashtab.
In general, this type of testing can usually be achieved by:
- Tools such as the React Testing Library - which facilitates the simulation of user inputs and verification of state variables afterward, which theoretically can be used to test this type of diff. However the issue when it comes to Cashtab is that the testing tool is looking for specific labels to latch onto in order to simulate the input (e.g. label="address-input"), whereas Cashtab uses re-usable subcomponents (e.g. <AddressInput>) which the tool can not detect. Cashtab components are essentially a blank page from this tool's POV. The key lesson here for the next app we build is to design it to be fully unit-testable from the outset.
- The other (hacky) way of achieving this is by moving the handleManualContactAddressInput function out, to the same level as the Configure class. This will allow it to be declared as an export function and accessed directly however it will cause issues where handleManualContactAddressInput can't access the state variables internal to the Configure component. In order to resolve that it starts looking like a refactor to make unit tests work rather than what makes sense from a component POV.
Hence why these types of diffs are manually tested for the time being.
Comment Actions
I totally agree that it doesn't belong in any of these files either. But it's not a configuration, nor a setting.
If it's for consistency with saved contacts, which makes sense, then maybe contacts.js ?
- There is no test
This is similar to SendToken.js where internal functions (i.e. handleManualContactAddressInput in this case) in a React component (classes that render things) can't be accessed directly. You can only test the initial state and props upon initialization but cases where you're looking to simulate a user input (entering a contact address) and then verifying it's reaction to that input is a challenge with Cashtab.
In general, this type of testing can usually be achieved by:
- Tools such as the React Testing Library - which facilitates the simulation of user inputs and verification of state variables afterward, which theoretically can be used to test this type of diff. However the issue when it comes to Cashtab is that the testing tool is looking for specific labels to latch onto in order to simulate the input (e.g. label="address-input"), whereas Cashtab uses re-usable subcomponents (e.g. <AddressInput>) which the tool can not detect. Cashtab components are essentially a blank page from this tool's POV. The key lesson here for the next app we build is to design it to be fully unit-testable from the outset.
- The other (hacky) way of achieving this is by moving the handleManualContactAddressInput function out, to the same level as the Configure class. This will allow it to be declared as an export function and accessed directly however it will cause issues where handleManualContactAddressInput can't access the state variables internal to the Configure component. In order to resolve that it starts looking like a refactor to make unit tests work rather than what makes sense from a component POV.
Hence why these types of diffs are manually tested for the time being.
Or you can keep it simple, create a function isValidRecipient(value) -> bool that takes either an address or an alias and call it from the handler like setManualContactAddressIsValid(isValidRecipient(value)). This isValidRecipient() function can be unit tested easily as it only requires a mock for the alias server query. Obviously the new function doesn't belong to Configure.js either.
Comment Actions
then maybe contacts.js ?
makes sense, will move it (T3254) in a separate diff as a new component accessed via the hamburger
Or you can keep it simple, create a function isValidRecipient(value) -> bool
damn... that is simpler... will give that a go
Comment Actions
Updated for all feedback except the boolean return statement
| cashtab/src/utils/validation.js | ||
|---|---|---|
| 25–27 ↗ | (On Diff #41662) | This suggestion actually ends up returning the value in aliasDetails.address in the unit tests, hence the original inline conditional statement to enforce a boolean evaluation. The other approach is to use the !! truthy operator in front of aliasDetails.address but that's fairly hard to read.  |
| cashtab/src/utils/validation.js | ||
|---|---|---|
| 25–27 ↗ | (On Diff #41662) | stupid js... then either !! like suggested (imo it's fine) or: return aliasDetails && !aliasDetails.error && aliasDetails.address !== ""; if you feel like it's more readable. |