Page MenuHomePhabricator

[chronik-client] Support token data in tx
ClosedPublic

Authored by bytesofman on Feb 14 2024, 19:40.

Details

Reviewers
tobias_ruck
Group Reviewers
Restricted Project
Commits
rABC5da580a8daf3: [chronik-client] Support token data in tx
Summary

Now that the in-node chronik supports token indexing, add supported proto data to chronik-client.

Test Plan

CI or ref D14915

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cc-tokens-in-tx
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 27143
Build 53852: Build Diffchronik-client-tests · chronik-client-integration-tests
Build 53851: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
modules/chronik-client/src/ChronikClientNode.ts
421 ↗(On Diff #45310)

there’s no spoon neo

625 ↗(On Diff #45310)

Yes, hopefully we can remove all the _InNode suffixes soon

634 ↗(On Diff #45310)

Protobuf enums must start at 0; but there’s no SLP type with that number, so I just put NONE. It’s a quirk of protobuf.

However, it actually should be turned into an UNKNOWN, since people could start sending token type 0 txs.

bytesofman added inline comments.
modules/chronik-client/src/ChronikClientNode.ts
407 ↗(On Diff #45310)

ok I think I finally get this now

634 ↗(On Diff #45310)

ok understood

bytesofman marked an inline comment as done.

remove doxygen comments from convertTo function, which is not defining the type

The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
tobias_ruck added inline comments.
modules/chronik-client/src/ChronikClientNode.ts
589 ↗(On Diff #45337)

this can now be guaranteed

modules/chronik-client/test/integration/token_alp.ts
461 ↗(On Diff #45337)

this should allow you to avoid all the // @ts-ignore comments

test/functional/setup_scripts/chronik-client_token_alp.py
136 ↗(On Diff #45337)

be careful here; either use genesis.test to ensure the token stuff in the TokenTx matches, or don't use TokenTx at all and just send txs normally (e.g. using sendrawtransaction or chronik.broadcast_tx).

Otherwise, people could change the tx and the actual token data of the tx won't be checked against the claimed token data in TokenTx.

My suggestion would be to not use TokenTx in the first place which would make these files significantly shorter; and that's ok because the txs are tested in JS already.

This revision now requires changes to proceed.Feb 17 2024, 15:17
bytesofman added inline comments.
modules/chronik-client/test/integration/token_alp.ts
461 ↗(On Diff #45337)

nice, ty

test/functional/setup_scripts/chronik-client_token_alp.py
136 ↗(On Diff #45337)

ok nice. i get what was going on in the file now. bit of a refactor with var names but definitely worth it.

what's with all the tx.rehash() statements? tests here pass without them, are they needed for the chronik compare tests / can they be omitted here?

bytesofman marked an inline comment as done.

no ts-ignore, remove TokenTx from setup scripts, remove tx.rehash() from setup scripts, remove done todo comment

test/functional/setup_scripts/chronik-client_token_alp.py
136 ↗(On Diff #45337)

The rehash was mostly related to GENESIS txs in TokenTx, but since you don't use that anymore you can just remove it.

modules/chronik-client/test/integration/token_slp_nft1.ts
247 ↗(On Diff #45347)

you can do the same thing here too I think

269 ↗(On Diff #45347)

and here

bytesofman added inline comments.
modules/chronik-client/test/integration/token_slp_nft1.ts
247 ↗(On Diff #45347)

nice yeah just missed it in this file.

modules/chronik-client/src/ChronikClientNode.ts
591 ↗(On Diff #45352)

I don’t wanna be that guy but while we’re at it we should turn this into a string literal sum type as well

663 ↗(On Diff #45352)
modules/chronik-client/test/integration/token_alp.ts
117 ↗(On Diff #45352)

What do you think about adding a default object and then use it with the spread operator? That’s what I do in the tests, and it removes a lot of dead LOC

modules/chronik-client/test/integration/token_slp_fungible.ts
151 ↗(On Diff #45352)

I don’t think we have to test that

modules/chronik-client/test/integration/token_slp_mint_vault.ts
151 ↗(On Diff #45352)

Same

modules/chronik-client/test/integration/token_slp_nft1.ts
151 ↗(On Diff #45352)

Aqui tambien

test/functional/setup_scripts/chronik-client_token_slp_nft1.py
89 ↗(On Diff #45352)

As discussed before, remove this

143 ↗(On Diff #45352)

Here too

184 ↗(On Diff #45352)

Here too

229 ↗(On Diff #45352)

Here too

This revision now requires changes to proceed.Feb 18 2024, 11:26
bytesofman marked 11 inline comments as done.

remove missed TokenTx in py script, remove copypasta placeholder test assertion, better type guards for TokenTxType, spread operator to simplify tokenEntries expected shape in tests

modules/chronik-client/src/ChronikClientNode.ts
591 ↗(On Diff #45352)

missed it, added

modules/chronik-client/test/integration/token_alp.ts
117 ↗(On Diff #45352)

nice optimization, makes it easier to see what we are actually testing

tobias_ruck added inline comments.
modules/chronik-client/src/ChronikClientNode.ts
603 ↗(On Diff #45361)

make sure to use it ;)

modules/chronik-client/test/integration/token_slp_nft1.ts
136 ↗(On Diff #45361)

these empty newlines seem unintentional (there's a few others below)?

This revision now requires changes to proceed.Feb 18 2024, 15:23
bytesofman added inline comments.
modules/chronik-client/test/integration/token_slp_nft1.ts
136 ↗(On Diff #45361)

(badly) assumed the linter was picking this up as I went through

bytesofman marked an inline comment as done.

use type, remove accidental newlines

so close!

modules/chronik-client/src/ChronikClientNode.ts
540 ↗(On Diff #45366)
This revision now requires changes to proceed.Feb 18 2024, 23:57
bytesofman marked an inline comment as done.

tokenStatus: TokenStatus not string

The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
tobias_ruck added inline comments.
modules/chronik-client/test/integration/token_alp.ts
248 ↗(On Diff #45372)

and then remove some of the fields below

263 ↗(On Diff #45372)

dito

This revision now requires changes to proceed.Feb 19 2024, 07:42
bytesofman marked 2 inline comments as done.

fix missed BASE_TOKEN_ENTRY in token_alp.ts, rebase

This revision is now accepted and ready to land.Feb 19 2024, 21:41