Page MenuHomePhabricator

[Cashtab + doc] Parse XECX rewards
ClosedPublic

Authored by bytesofman on Dec 27 2024, 12:48.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCaac6a42e9df3: [Cashtab + doc] Parse XECX rewards
Summary

Add spec for XECX EMPP txs

Implement spec in Cashtab to parse XECX payout txs

To do this, we implement parsing EMPP app actions in Cashtab with associated helper functions.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
xecx-doc
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 31804
Build 63101: Build Diffcashtab-tests
Build 63100: arc lint + arc unit

Event Timeline

test error cases for getEmppActions

Fabien requested changes to this revision.Dec 28 2024, 20:59
Fabien added a subscriber: Fabien.
Fabien added inline comments.
cashtab/src/chronik/index.ts
493 ↗(On Diff #51770)

concat ? Also why do you do a copy here ?

I would expect a push in a loop intuitively.

cashtab/src/opreturn/index.ts
292 ↗(On Diff #51770)

no consume ? I remember we added a consume function at some point

302 ↗(On Diff #51770)

should it be an error if it is undefined ?

330 ↗(On Diff #51770)

Any plan to move the agora parsing here ?

doc/standards/op_return-prefix-guideline.md
27 ↗(On Diff #51770)

You should create a separated spec file and reference it in the array. It's now big enough that it's not really readable

This revision now requires changes to proceed.Dec 28 2024, 20:59
bytesofman added inline comments.
cashtab/src/chronik/index.ts
493 ↗(On Diff #51770)

stopped using concat

I'm not sure on which approach is "better" here

.push seems more intuitive and it also allows us to keep appActions as a const, since we are not mutating it.

But, if we have multiple actions, seems like "just combine the arrays" is mb faster than iterating? Not like this type of hypothetical efficiency is meaningful for this loop, where there will only be 1 action 99.9% of the time ... but in terms of best practices for JS. I default to using concat.

hm after asking grok looks like concat is actually the more memory intensive operation, TIL. will .push by default in future.

cashtab/src/opreturn/index.ts
292 ↗(On Diff #51770)

there is a consume method available in the ecash-script lib, which is where we get stackArray

Potentially should probably deprecate ecash-script. It's possible to parse OP_RETURN with more granularity using ecash-lib.

However, the stackArray is itself ... just really convenient for OP_RETURN parsing in JS. getStackArray deals with all the various to-spec push options of OP_RETURN and gives you all the pushes as a hex string. This is quite convenient for parsing txs in Cashtab, and it also makes sense to store this kind of object when the input could be literally anything -- well, any on-spec OP_RETURN that the ecash network accepts.

We have a unique situation here with EMPP pushes. We know it is EMPP because the first push is OP_RESERVED. But then every other push is itself its own stack that needs to be parsed to its own spec. We used consume in the ecash-script lib to get the stackArray in the first place; now the OP_RESERVED is a marker that "this stack array contains EMPP pushes, each of which is its own stack."

EMPP pushes are unique because there is no push data internal to the EMPP push. You need to parse it specifically according to its spec. getStackArray is able to parse any arbirary OP_RETURN string to the OP_RETURN spec. We can't really have a "catch-all" function for parsing EMPP ... I mean .... well, we could, but it needs to be updated for every supported app. I don't know if it would make sense to do that at the library level. If so, we should do it in ecash-lib.

That said, consume would be useful for parsing each EMPP push, implemented there.

302 ↗(On Diff #51770)

this is a one-off thing that needs to be handled somehow.

Issue is that -- ALP is EMPP. But ALP is indexed by chronik. So we get all our token info for ALP already preprocessed from chronik-client in tokenEntries. We could parse it here in EMPP, but that is not really useful since we don't have the index available ... we only know it's valid from the indexer, so the indexer should be our reference.

The API of "getEmppAction(push) returns undefined if the EMPP action is ALP" is how I have decided to handle this situation. I've also extended it to include Agora, which has a similar issue.

330 ↗(On Diff #51770)

No. Like ALP txs, the main way we know something is Agora is because the agora.py plugin has indexed it this way. We could have a situation where there is "valid" agora info in an EMPP push but the tx itself is not actually a valid agora anything.

We might want to check out agora EMPP if we already know it is valid agora from the plugin. but currently we do not have a need for this.

bytesofman marked 3 inline comments as done.

add detailed spec, use consume to parse empp push

Fabien added inline comments.
cashtab/src/opreturn/index.ts
374

Can't you inline the consume() ? This would avoid the extra long variables

This revision is now accepted and ready to land.Dec 29 2024, 08:59
This revision was automatically updated to reflect the committed changes.