Page MenuHomePhabricator
Differential D17194

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

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

Details

Reviewers
tobias_ruck
Group Reviewers
Restricted Project
Commits
rABCb108a03d2181: [chronik-client] Update GenesisInfo to return hex string for data key of ALP…
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

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
teamcity added a comment.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

bytesofman updated this revision to Diff 51075.Sun, Nov 24, 13:02

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

This is a breaking change so should require a 2.0.0

modules/chronik-client/test/integration/token_alp.ts
212–214
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

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.

bytesofman updated this revision to Diff 51081.Sun, Nov 24, 22:31
bytesofman marked an inline comment as done.

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

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.

This revision now requires changes to proceed.Mon, Nov 25, 01:00
bytesofman updated this revision to Diff 51083.Mon, Nov 25, 01:23

major version bumps and readme/changelog updates

Harbormaster completed remote builds in B31305: Diff 51083.Mon, Nov 25, 01:31
tobias_ruck accepted this revision.Mon, Nov 25, 01:54
This revision is now accepted and ready to land.Mon, Nov 25, 01:54
Closed by commit rABCb108a03d2181: [chronik-client] Update GenesisInfo to return hex string for data key of ALP… (authored by bytesofman). · Explain WhyMon, Nov 25, 02:17
This revision was automatically updated to reflect the committed changes.
bytesofman added a commit: rABCb108a03d2181: [chronik-client] Update GenesisInfo to return hex string for data key of ALP….

Revision Contents
Changeset List

PathSize
modules/
chronik-client/
1 line
4 lines
2 lines
src/
4 lines
test/
integration/
9 lines
ecash-lib/
2 lines
4 lines
2 lines
src/
token/
2 lines
2 lines
tests/
4 lines

Diff 51075

View Options

modules/chronik-client/README.md

Loading...
View Options

modules/chronik-client/package-lock.json

Loading...
View Options

modules/chronik-client/package.json

Loading...
View Options

modules/chronik-client/src/ChronikClient.ts

Loading...
View Options

modules/chronik-client/test/integration/token_alp.ts

Loading...
View Options

modules/ecash-lib/README.md

Loading...
View Options

modules/ecash-lib/package-lock.json

Loading...
View Options

modules/ecash-lib/package.json

Loading...
View Options

modules/ecash-lib/src/token/alp.ts

Loading...
View Options

modules/ecash-lib/src/token/common.ts

Loading...
View Options

modules/ecash-lib/tests/alp.test.ts

Loading...