Page MenuHomePhabricator

[Cashtab] [Chronik] getTokenStats refactor
ClosedPublic

Authored by emack on Sep 21 2022, 16:04.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABCc351e1f31c77: [Cashtab] [Chronik] getTokenStats refactor
Summary

This diff migrates the getTokenStats() function from useBCH.js into the chronik.js hook and refactored it to retrieve token info via the chronik client and deprecates the use of bch-js' BCH.SLP.Utils.tokenStats() api call. sendToken.js is also refactored to directly reference chronik token object params.

Test Plan
  • npm test
  • npm start
  • navigate to a token
  • validate token stats with bch-api's tokenStats

e.g. https://rest.kingbch.com/v4/slp/tokenStats/cc1f4b7feb98a3de8879bc9ceef69510c48868b3f6c11407fe8e2c9e42ec3120

  • validate token genesis info with explorer.e.cash
  • burn some of the token in the wallet's possession
  • refresh the sendToken component and ensure the circulating supply and total burned stats have been updated
  • test a newly created token and ensure the genesis date parsing shows holding text when block.timestamp is null (still in mempool)
  • wait till this newly created token genesis tx is mined and ensure the genesis date is displayed correctly
  • test tokens with various decimal points and ensure values are presented correctly.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
getTokenStats
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 20118
Build 39923: Build Diffcashtab-tests
Build 39922: arc lint + arc unit

Event Timeline

emack requested review of this revision.Sep 21 2022, 16:04

Failed tests logs:

====== CashTab Unit Tests:  getTokenStats successfully returns a Cashtab compatible tokenStat object via chronik ======
Error: expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

@@ -7,11 +7,11 @@
    "documentUri": "https://cashtab.com/",
    "id": "bb8e9f685a06a2071d82f757ce19201b4c8e5e96fbe186960a3d65aec83eab20",
    "initialTokenQty": "21000000",
    "name": "FaucetToken",
    "symbol": "FTN",
-   "timestamp": "11/09/2022, 8:12:12 am",
+   "timestamp": "9/11/2022, 8:12:12 AM",
    "timestampUnix": "1662883932",
    "totalBurned": "20",
    "totalMinted": "21000000",
    "versionType": "FUNGIBLE",
  }
    at Object.<anonymous> (/work/web/cashtab/src/utils/__tests__/chronik.test.js:46:51)
    at processTicksAndRejections (node:internal/process/task_queues:94:5)

Each failure log is accessible here:
CashTab Unit Tests: getTokenStats successfully returns a Cashtab compatible tokenStat object via chronik

Arc lint changed the date format in the chronik token mocks which then causes the build issue, will fix in the next update post feedback.

Good looking diff. A couple of points.

  1. There are lots of spacing changes to comments unrelated to this diff. Could you run arc lint before pushing up the next revision? Mb these files just happened to not be linted in some time, but it's possible your IDE settings are a little different from arc lint
  1. grep -r getTokenStats src/ shows that this function is only used in SendToken.js -- so this is a good opportunity to move away from the format provided by the legacy getTokenStats function. I think it's best to preserve the chronik format and refactor SendToken.js to accept this. I'm open to other perspectives if you think the legacy tokenStats format is specifically useful for other reasons, but seems like a good opportunity to drop this bch-api artifact.
This revision now requires changes to proceed.Sep 21 2022, 22:48

Fixed IDE induced linting changes - more updates to follow

Failed tests logs:

====== CashTab Unit Tests:  getTokenStats successfully returns a Cashtab compatible tokenStat object via chronik ======
Error: expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

@@ -7,11 +7,11 @@
    "documentUri": "https://cashtab.com/",
    "id": "bb8e9f685a06a2071d82f757ce19201b4c8e5e96fbe186960a3d65aec83eab20",
    "initialTokenQty": "21000000",
    "name": "FaucetToken",
    "symbol": "FTN",
-   "timestamp": "11/09/2022, 8:12:12 am",
+   "timestamp": "9/11/2022, 8:12:12 AM",
    "timestampUnix": "1662883932",
    "totalBurned": "20",
    "totalMinted": "21000000",
    "versionType": "FUNGIBLE",
  }
    at Object.<anonymous> (/work/web/cashtab/src/utils/__tests__/chronik.test.js:46:51)
    at processTicksAndRejections (node:internal/process/task_queues:94:5)

Each failure log is accessible here:
CashTab Unit Tests: getTokenStats successfully returns a Cashtab compatible tokenStat object via chronik

Fixing build failures caused by date format in chronik mocks

Build issues resolved, more update to follow to refactor sendToken to accept the chronik token format, hold off on review for now.

Good to continue review now.

  • getTokenStats no longer carries out custom mapping to the legacy bch-api structure and passes the chronik token response to sendToken.js, with the exception of totalMinted, totalBurned, initialTokenQuantity and circulatingSupply as they needed to be dynamically adjusted based on the token's decimal points
  • the chronik unit test for getTokenStats now adjusted to specifically test for the four parameters above since the remaining parameters are directly from the API call
  • sendToken.js refactored to directly reference the chronik token parameters rather than the legacy bch-api token structure
  • see additional test cases added to test plan above

Removing redundant chronik mock

bytesofman added inline comments.
web/cashtab/src/utils/__tests__/chronik.test.js
54 โ†—(On Diff #35100)

Shouldn't this have decimal places now? i.e. shouldn't it be new BigNumber(21000000.00)?

web/cashtab/src/utils/chronik.js
14 โ†—(On Diff #35100)

Elsewhere in chronik.js, the app is using .shiftedBy(-1 * thisTokenDecimals) instead of .div(new BigNumber(10).exponentiatedBy(tokenDecimals)

Should be consistent throughout the app, so please match to shiftedBy

Going forward, might make more sense to put this into its own function, since it's not intuitive what is going on here and it's used all over the app.

e.g. something like

tokenResponseObj.tokenStats.totalMinted = adjustTokenQtyForTokenDecimals(tokenDecimals, tokenResponseObj.tokenStats.totalMinted)

Anyway, irrelevant for this diff. I created T2690 for later implementation.

This revision now requires changes to proceed.Sep 22 2022, 11:51
emack marked 2 inline comments as done.
  • aligned .shiftedBy approach with the rest of the chronik hooks
  • ensured minimum decial points based on token decimal point via toFormat()
  • updated unit tests to factor for token decimal points
bytesofman added inline comments.
web/cashtab/src/utils/__tests__/chronik.test.js
47 โ†—(On Diff #35107)

This is tautalogical in the unit test, since you're expecting something to match what is done in the function.

Please use a mock of actual expected results and expect to match that in the unit test area, rather than performing the same operations on a mock to get expected results.

This revision now requires changes to proceed.Sep 22 2022, 19:53
emack marked an inline comment as done.
  • rebased to master
  • updated unit test to compare with a full mock response of getTokenStats
bytesofman added inline comments.
web/cashtab/src/utils/__mocks__/mockChronikTokenStats.js
51 โ†—(On Diff #35128)

Stuff like commas and exact decimals we should be rendering on display. It isn't a big deal at the moment since this is the only place where this is used. However, if we use the return object of getTokenStats anywhere else, it would be weird to parse away the commas if we wanted to do math with it.

Also, with T2690, we want to have a consistent function and approach for dealing with "token quantity numbers"

Please

  • get rid of the toFormat thing in the function
  • store these values in the getTokenStats return object as strings
  • use toFixed(decimals) when displaying the values on SendToken.js
This revision now requires changes to proceed.Sep 23 2022, 05:34

Moved decimcal and locale formatting to sendToken.js and token stats now stored as strings in the return object.
I retained toFormat() rather than toFixed() because toFixed() requires conversion to Number which is not appropriate for our big numbers here. toFormat() works well with BigNumber.

This revision is now accepted and ready to land.Sep 23 2022, 15:12
This revision was automatically updated to reflect the committed changes.