[chronik-client] Update GenesisInfo to return hex string for data key of ALP tokens
Needs ReviewPublic
Actions

Authored by bytesofman on Sun, Nov 24, 12:42.

Details

Reviewers

tobias_ruck

Group Reviewers

Restricted Project

Summary

This matches existing patterns in chronik-client. IMO we should stick to hex strings for API returns. API returns for tokens -- esp GenesisInfo -- needs to be cached as it never changes. Caching Uint8Array is complicated because it is not JSON. But hex strings are easy.

ecash-lib has available toHex and fromHex methods that make conversion easy depending on dev needs.

auth as a Uint8Array would require special handling in Cashtab's existing caching mechanism if we do not make this change.

Test Plan

CI tests

Diff Detail

Repository

rABC Bitcoin ABC

Branch

cc-genesisinfo-data

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 31303
Build 62104: Build Diff	chronik-client-integration-tests · ecash-agora-integration-tests · ecash-lib-integration-tests · ecash-agora-tests · ecash-lib-tests · chronik-client-tests · token-server-tests · ecash-herald-tests · cashtab-tests
Build 62103: arc lint + arc unit

Event Timeline

bytesofman created this revision.Sun, Nov 24, 12:42

Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Nov 24, 12:42

Harbormaster completed remote builds in B31297: Diff 51074.Sun, Nov 24, 12:48

Build Bitcoin ABC Diffs / Diff Testing (ecash-lib-integration-tests) failed.

Failed tests logs:

====== ALP: TxBuilder P2PKH ALP.ALP TxBuilder P2PKH ALP ======
AssertionError: expected { …(4) } to deeply equal { …(4) }
    at Context.<anonymous> (tests/alp.test.ts:121:58)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

      + expected - actual

       {
         "genesisInfo": {
           "authPubkey": "03040506"
      -    "data": "01020304"
      +    "data": "1,2,3,4"
           "decimals": 4
           "tokenName": "ALP Token Name"
           "tokenTicker": "ALP TOKEN"
           "url": "https://example.com"

Each failure log is accessible here:
ALP: TxBuilder P2PKH ALP.ALP TxBuilder P2PKH ALP

match change in ecash-lib

Harbormaster completed remote builds in B31298: Diff 51075.Sun, Nov 24, 13:11

bytesofman published this revision for review.Sun, Nov 24, 13:30

tobias_ruck requested changes to this revision.Sun, Nov 24, 22:25

tobias_ruck added a subscriber: tobias_ruck.

tobias_ruck added inline comments.

modules/chronik-client/README.md
117 ↗	(On Diff #51075)	This is a breaking change so should require a 2.0.0
modules/chronik-client/test/integration/token_alp.ts
212–214 ↗	(On Diff #51075)

This revision now requires changes to proceed.Sun, Nov 24, 22:25

bytesofman marked 2 inline comments as done.Sun, Nov 24, 22:31

bytesofman added inline comments.

modules/chronik-client/README.md
117 ↗	(On Diff #51075)	technically true (and also for ecash-lib, right?) I think it's early enough days for ALP to keep this at the patch level for now. I'm ok with major-version bumping ecash-lib and chronik-client but it's more letter-of-the-law than spirit.

update test

Harbormaster completed remote builds in B31303: Diff 51081.Sun, Nov 24, 23:18

tobias_ruck requested changes to this revision.Mon, Nov 25, 01:00

tobias_ruck added inline comments.

modules/chronik-client/README.md
117 ↗	(On Diff #51075)	But if there's anyone using this field already (on 1.4.x), a new release on npm would automatically break their software. I don't really see the problem with bumping the version to the "correct" semver value? It's not like we'll run out of numbers or so. If there's another breaking change later, we can bump the version to 3. Not trying to be pedantic, just don't really see what problem is being solved by just bumping it as a patch version instead of major version.