Page MenuHomePhabricator

[Cashtab] [Alias] pt 14 - Frontend bytesize validation
ClosedPublic

Authored by emack on Mar 3 2023, 00:43.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC98375040b2ed: [Cashtab] [Alias] pt 14 - Frontend bytesize validation
Summary

T2551

This is a standalone diff that validates the frontend alias input based on bytesize rather than string chars.

[Cashtab] [Alias] pt 1 - Create scaffold for new Alias component
[Cashtab] [Alias] pt 2 - Upgrade sendXec() to handle alias registration
[Cashtab] [Alias] pt 3 - Implement isAliasAvailable function
[Cashtab] [Alias] pt 4 - Implement isAddressRegistered function
[Cashtab] [Alias] pt 5 - Implement getAddressFromAlias function
[Cashtab] [Alias] pt 6.1 - Get latest alias tx count from payment address
[Cashtab] [Alias] pt 6.1.1 - Apply Promise.All approach for alias history retrieval
[Cashtab] [Alias] pt 6.2 - Implement getAliasesFromLocalForage
[Cashtab] [Alias] pt 6.3 - Implement updateAliases
[Cashtab] [Alias] pt 6.4 - Update getAliases() to extract both alias and address
[Cashtab] [Alias] pt 6.5 - Optimize getAllTxHistory to only make API calls for uncached tx history pages
[Cashtab] [Alias] pt 6.6 - Render list of Aliases owned by active wallet in Alias.js
[Cashtab] [Alias] pt 7 - Mitigate edge cases for registration records
[Cashtab] [Alias] pt 8 - Activation flag in prod
[Cashtab] [Alias] pt 9 - Retain tokenInfoById upon alias validation
--stacked diff cutoff--
[Cashtab] [Alias] pt 10 - Enable alias inputs for one to one Send XEC txs
[Cashtab] [Alias] pt 11 - Enable alias inputs for Send Token txs
[Cashtab] [Alias] pt 12 - Upgrade tx history to recognize alias registration txs
[Cashtab] [Alias] pt 13 -real time alias char length and registration fee display
[Cashtab] [Alias] pt 14 - Frontend bytesize validation
--closed beta--
[Cashtab] [Alias] pt 15 - Pre-prod update (add p2sh parsing, set final registration fees, remove residual dev logs, test in extension mode and enable prod flag)
[Cashtab] [Alias] pt 16 - Port Alias feature to Cashtab extension
--post mvp---
[Cashtab] [Alias] - Enable alias parsing without the .xec extension
[Cashtab] [Alias] - Optimize isAliasAvailable to take cached tx history as input
[Cashtab] [Alias] - Add active wallet's aliases to caching mechanism
[Cashtab] [Alias] - Resolve special characters processing in node app

Test Plan
  • grep -r 'aliasRegistrationFeeInSats' src/ and ensure all references are now to *Bytes rather than *Char
  • npm test
  • set aliasEnabled to true
  • npm start
  • enter an alias of exactly 21 bytes and ensure the 8 byte fee is displayed
  • enter an alias > 21 bytes and ensure the validation error is displayed and the registration button is disabled
  • regression test byte lengths 1-8 to ensure the correct fee is displayed
  • regression test emoji, special characters and non-english characters
  • set aliasEnabled to false and ensure nothing blows up

Diff Detail

Repository
rABC Bitcoin ABC
Branch
bytesizevalidation
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 22242
Build 44119: Build Diffcashtab-tests
Build 44118: arc lint + arc unit

Event Timeline

emack requested review of this revision.Mar 3 2023, 00:43
bytesofman requested changes to this revision.Mar 3 2023, 03:54

implementation looks good, see inline comment for question re: Blob approach

web/cashtab/src/components/Alias/Alias.js
190 ↗(On Diff #38255)

Are we 100% sure that new Blob([value]).size will always be the same as the length of the generated op_return script?

I let this slide in the last diff but questioning it now. Seems like we should just use the exact same function for both just to be sure.

I'm open to the Blob but we need more certainty than just a few unit tests.

From this, seems like Blob.text() could be more appropriate: https://developer.mozilla.org/en-US/docs/Web/API/Blob

Still, seems risky using two different approaches. We might as well calculate the backend length of the alias and parse that, rather than create a front-end construct that should be the same.

Please either

  • Provide some info / justification of why the blob approach is ok (for example, is this the method being used in the library that generatees op_return scripts?)
  • Instead of blob, calculate the bytesize by creating the op_return script and reading the length
This revision now requires changes to proceed.Mar 3 2023, 03:54
emack marked an inline comment as done.

Adjusted byte size calculation via the OP_RETURN script rather than the new Block([value]) approach

bytesofman requested changes to this revision.Mar 3 2023, 13:09
bytesofman added inline comments.
web/cashtab/src/utils/__tests__/cashMethods.test.js
118 ↗(On Diff #38260)

Let's delete all references to new Blob

No need to have this in the unit test now that this approach is not used in the app.

Please refactor these unit tests so that they are checking for an expected hardcoded length instead of a Blob-based length.

140 ↗(On Diff #38260)

Let's delete all references to new Blob

No need to have this in the unit test now that this approach is not used in the app.

Please refactor these unit tests so that they are checking for an expected hardcoded length instead of a Blob-based length.

162 ↗(On Diff #38260)

Let's delete all references to new Blob

No need to have this in the unit test now that this approach is not used in the app.

Please refactor these unit tests so that they are checking for an expected hardcoded length instead of a Blob-based length.

184 ↗(On Diff #38260)

Let's delete all references to new Blob

No need to have this in the unit test now that this approach is not used in the app.

Please refactor these unit tests so that they are checking for an expected hardcoded length instead of a Blob-based length.

206 ↗(On Diff #38260)

Let's delete all references to new Blob

No need to have this in the unit test now that this approach is not used in the app.

Please refactor these unit tests so that they are checking for an expected hardcoded length instead of a Blob-based length.

228 ↗(On Diff #38260)

Let's delete all references to new Blob

No need to have this in the unit test now that this approach is not used in the app.

Please refactor these unit tests so that they are checking for an expected hardcoded length instead of a Blob-based length.

250 ↗(On Diff #38260)

Let's delete all references to new Blob

No need to have this in the unit test now that this approach is not used in the app.

Please refactor these unit tests so that they are checking for an expected hardcoded length instead of a Blob-based length.

This revision now requires changes to proceed.Mar 3 2023, 13:09

Updated unit tests expected hardcoded byte length

bytesofman requested changes to this revision.Mar 3 2023, 14:30
bytesofman added inline comments.
web/cashtab/src/utils/cashMethods.js
18

sorry, missed this one in the last review.

  • input param for this function should be aliasHexBytes

then

const aliasByteCount = aliasHexBytes.length / 2
switch (aliasByteCount) ...
This revision now requires changes to proceed.Mar 3 2023, 14:30

Refactored getAliasRegistrationFee to take in aliasHexBytes, including all upstream functions that interacts with it.

bytesofman requested changes to this revision.Mar 3 2023, 23:37
bytesofman added inline comments.
web/cashtab/src/components/Alias/Alias.js
214 ↗(On Diff #38285)

typo registratioFee

doesn't need to be handled here but at least needs to be tracked

web/cashtab/src/utils/cashMethods.js
15 ↗(On Diff #38285)

let's keep alias as the input, and do the conversion to bytes and then to bytecount in this function

For all cases getAliasRegistrationFee is called, the alias input is available. It makes sense to keep it this way rather than modify the input in 3 of the 4 times the function is called.

This revision now requires changes to proceed.Mar 3 2023, 23:37
emack marked 2 inline comments as done.

Adjusted getAliasRegistrationFee to take in the alias input string and carries out the conversion to bytes and bytecount within the function. A new getAliasByteSize() helper function is also added, along with updated unit tests that calls this function.

This revision is now accepted and ready to land.Mar 4 2023, 00:56