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.
Details
Details
- Reviewers
bytesofman - Group Reviewers
Restricted Project - Commits
- rABCc351e1f31c77: [Cashtab] [Chronik] getTokenStats refactor
- npm test
- npm start
- navigate to a token
- validate token stats with bch-api's tokenStats
- 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
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- getTokenStats
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 20119 Build 39925: Build Diff cashtab-tests Build 39924: arc lint + arc unit
Event Timeline
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.
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
Comment Actions
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.
Comment Actions
Good looking diff. A couple of points.
- 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
- 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.
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.
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
Comment Actions
Build issues resolved, more update to follow to refactor sendToken to accept the chronik token format, hold off on review for now.
Comment Actions
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
| web/cashtab/src/utils/__tests__/chronik.test.js | ||
|---|---|---|
| 54 | Shouldn't this have decimal places now? i.e. shouldn't it be new BigNumber(21000000.00)? | |
| web/cashtab/src/utils/chronik.js | ||
| 14 | 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. | |
Comment Actions
- 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
| 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. |
Comment Actions
- rebased to master
- updated unit test to compare with a full mock response of getTokenStats
| 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
|
Comment Actions
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.