T3216
Switching the registration fee lookup to the alias-server prices endpoint.
Details
Details
- Reviewers
bytesofman Fabien - Group Reviewers
Restricted Project - Commits
- rABC1d1f62a9825a: [Cashtab][Alias] Switch alias registration fee lookup to api
Revised test plan (29th July)
- grep -r aliasRegistrationFeeInSats src/ and ensure no output
- npm test
- Set alias flag to true in Ticker.js
- npm start and navigate to the Alias page
- Attempt to register an existing alias and observe the This alias [alias] has already been taken by <address>, please try another alias error notification
- Input an unregistered alias, click Register Alias, verify the price on the preview modal, click Ok and ensure successful broadcast of the registration tx
- In preparePreviewModal, change the queryAliasServer's alias endpoint to an invalid one, attempt to register an existing and unregistered alias and ensure the Error retrieving alias details pop up notification, the Registered aliases list being overwritten by Unable to retrieve alias info, please try again later and the Register Alias button is perma-disabled. It can be reset by reloading or re-navigating to the Alias page.
- In preparePreviewModal, change the 2nd aliasInput param to aliasInput+'_' in the queryAliasServer call to simulate a valid error response. Observe the pop up error and the Registered aliases list being overwritten by Unable to retrieve alias info, please try again later. The Register Alias button is similarly perma-disabled because Cashtab should never let an invalid alias input flow all the way through to the alias-server.
- Revert preparePreviewModal endpoints to its original valid state, repeat this simulation for the queryAliasServer call in useEffect() against the /address endpoint and observe similar errors on page load and perma-disabling of the Register Alias button. Ensure entering fresh inputs does not re-active the Register Alias button
- Set the alias flag to false in Ticker.js
- Sanity check via sending XEC txs
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| cashtab/src/components/Common/Ticker.js | ||
|---|---|---|
| 25 ↗ | (On Diff #41404) | Nice, one less in Ticker.js. The whole aliasSettings.js should also be moved to e.g. config/alias.js |
| cashtab/src/utils/aliasUtils.js | ||
| 44 ↗ | (On Diff #41404) | I might be missing something, but I looked at the queryAliasServer function and it appears that it throws on error. My understanding is that all these checks are unreachable. Is there any tests for these errors ? |
| cashtab/src/components/Alias/Alias.js | ||
|---|---|---|
| 218 ↗ | (On Diff #41404) | We don't want to be calling an async function every time the user types a character in the alias field alias prices are not expected to change often. Get alias prices and save in state on Alias.js screen load We could confirm before registration tx is broadcast that the price is accurate. Imo this is a bit overkill, we can deal with this if and when we change alias prices...would only impact users who had cashtab open on the Alias screen while we updated alias prices. Changing the alias prices would be a big deal -- we would do a review, make sure Cashtab is ready for the migration, etc. So, for now, just get the prices when Alias screen loads and save in state. Lock the UI until the pricelist is loaded. |
| cashtab/src/utils/aliasUtils.js | ||
| 44 ↗ | (On Diff #41404) | I'm not aware of a case where queryAliasServer does not throw an error but we still have !priceResponse ... seems like the Unable to connect to alias-server error is unreachable. |
Comment Actions
- updated to retrieve registration prices once on page load and saved in state
- error notification on price retrieval errors on page load, this can be tested by changing const priceResponse = await queryAliasServer('prices', ''); to const priceResponse = await queryAliasServer('prices', 'aaa'); in Alias.js to trigger an exception. Verify the UI error notification and the permanent disabling of the Register button
- removed redundant getAliasRegistrationFee
| cashtab/src/components/Common/Ticker.js | ||
|---|---|---|
| 25 ↗ | (On Diff #41404) | yup it's part of the tail end clean up diffs planned |
| cashtab/src/components/Alias/Alias.js | ||
|---|---|---|
| 145 ↗ | (On Diff #41431) | Note that, if there is a server error, /prices may return an object like {error: 'some error msg'} So, we need a validation function, isValidPriceResponse. Could maybe do it as a one liner. Need to check that priceResponse |
| 152 ↗ | (On Diff #41431) | I'm not so sure. We would like the user to be able to view their own aliases. But we don't want to allow any registrations. So, probably best to not lock the UI -- but disable the 'register alias' button and show an error message |
Comment Actions
- added isValidAliasPricing in validation.js that checks alias pricing response for keys and value types
- updated error handling for alias price retrieval logic
| cashtab/src/components/Alias/Alias.js | ||
|---|---|---|
| 164 ↗ | (On Diff #41443) | Is that run for each block ? How does it work if the price changes ? |
| cashtab/src/components/Alias/Alias.js | ||
|---|---|---|
| 164 ↗ | (On Diff #41443) | hmm good point, this is run on each page load, so the user can technically just stay on the page forever and the price won't update. I can do a separate diff and use a chronik websocket to listen for new blocks and then trigger a refresh of pricing in addition to this update on page load. |
Comment Actions
Although having said that, changing prices after launch is meant to be extremely rare.
Also the cashtab update modal, that pop up when we deploy an update to the code, refreshes the app, which mitigates stale pricing.
| cashtab/src/components/Alias/Alias.js | ||
|---|---|---|
| 164 ↗ | (On Diff #41443) | The price endpoint needs to be stateful as well. Say you want to start another alias server, you won't be able to validate the aliases if the price is changed, as past registrations might be invalidated. |
| cashtab/src/components/Alias/Alias.js | ||
|---|---|---|
| 164 ↗ | (On Diff #41443) | If alias pricing does change, it will be implemented in a way that does not invalidate already-registered aliases. This is mentioned in the spec but looking at it now it is a bit ambiguous, will update. A price change would be implemented by "aliases registered in block 8xx,xxx or higher are validated by these new prices" Since any price change would need to come from an alias-server change and a spec update, imo it's okay to assume the prices are constant unless and until we change them. D14267 - spec update So, Cashtab will need to confirm that the current blockheight is <= to the validThru key provided by the server |
| cashtab/src/components/Alias/Alias.js | ||
|---|---|---|
| 164 ↗ | (On Diff #41443) |
This brings zero value to make cashtab track the tip height. All you really need is to make sure that you refresh the prices at each block, and that the returned prices are valid for the next block (but that's on the alias server side). |
Comment Actions
Refactored to align with the revised alias endpoint where unregistered aliases returns an object containing the new registrationFeeSats param.
Key changes include:
- Removed full pricing tier retrieval at page load
- Instead of showing updated pricing for each character entered into the registration input field, the final price will be displayed via the preview modal via one single API call. This is to avoid making an API call for every single character input.
- The one single API call being made is at the point of retrieving alias price for the preview modal
Note: refer to revised test plan above
Comment Actions
Removed redundant isValidAliasPricing function and its corresponding unit tests as Cashtab is now exclusively using the /alias endpoint for pricing and no longer has a need for the /pricing endpoint
| cashtab/src/components/Alias/Alias.js | ||
|---|---|---|
| 185 ↗ | (On Diff #41635) | You should print the address here as well. I can imagine a user checking his alias via this |
| 281 ↗ | (On Diff #41635) | Is that still needed ? The server already returns the appropriated price now. Can be removed in another diff if no longer needed |
Comment Actions
Updated alias taken notification to include address
| cashtab/src/components/Alias/Alias.js | ||
|---|---|---|
| 281 ↗ | (On Diff #41635) | This is used to render the length in the frontend in real time as the user is typing out the alias input, which I think still has value. It allows them to test different permutations of their alias input and immediately see the length and cross check it against the final published price per byte.  |
| cashtab/src/components/Alias/Alias.js | ||
|---|---|---|
| 185 ↗ | (On Diff #41644) | Wording nit, leave that up to you |
| 281 ↗ | (On Diff #41635) | OK, I'm not convinced but it's out of scope for this diff anyway. Imo a better UX would be to grey out the register button if the alias isn't available with some message, and change the button caption depending on the alias like "Register for xxx XEC" |
Comment Actions
imo this can be greatly optimized by using throttling of the API calls to still show alias price and availability in near-real time. But, this gets the job done, should get in.