Page MenuHomePhabricator

[ecash-lib] Add SLP support
ClosedPublic

Authored by tobias_ruck on Apr 30 2024, 01:18.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC2c1f37ee6d76: [ecash-lib] Add SLP support
Summary

Adds SLP support to ecash-lib for all the token types supported by Chronik (Fungible, Mint Vault, NFT1 Group, NFT1 Child).

Mint Vault tokens require us to mine in tests, so we add the 'generate' cmd to SetupFramework.

Test Plan

npm run integration-tests

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Very powerful diff. Deprecates slp-mdm (and its associated bignumber.js requirement) --- and adds more features at the same time.

A few questions and checks in the in-line comments.

Shouldn't tx version be 2 by default, instead of 1? This is what Cashtab and ElectrumABC create currently (though CashFusion txs are version 1).

modules/ecash-lib/src/token/slp.ts
26 ↗(On Diff #47502)

we should include validation for tokenType so that an error is thrown if it is not a supported SLP token type. goal is to have new-ish devs using this lib. want to avoid as much as possible on-chain txs from implementation errors.

125 ↗(On Diff #47502)

Note that Cashtab still uses the equivalent of slpSend for burn txs, as the explicit burn function in SLP will only burn whole utxos (at least, this was how I interpreted the spec the last time I looked into it).

Not sure if any action is required in this diff. Could comment linking to the slp1 spec

https://github.com/badger-cash/slp-self-mint-protocol/blob/master/token-type1-burn.md

modules/ecash-lib/tests/slp.test.ts
135 ↗(On Diff #47502)

why not the conventional 546?

268 ↗(On Diff #47502)

is ecash-lib by default creating version 1 txs?

They should be version 2, no?

372 ↗(On Diff #47502)

if we're going to test through broadcasting, we should also test that the tx is as expected

853 ↗(On Diff #47502)

same comment as above re: version

939 ↗(On Diff #47502)

intentionally testing an NFT child mint with an input token utxo of > 1?

1005 ↗(On Diff #47502)

is this the usual API for calling slpSend, i.e. you must include a zero amount for an output that is not receiving the qty? Or is this testing some specific case?

Could you call slpSend(childTokenId, SLP_NFT1_CHILD, [1]) for a no-token-change tx sending 1 NFT?

This revision now requires changes to proceed.Apr 30 2024, 16:34

Shouldn't tx version be 2 by default, instead of 1? This is what Cashtab and ElectrumABC create currently (though CashFusion txs are version 1).

I can change it to 2; I made it 1 because it felt more default. However, the "default" in the C++ code is indeed 2.

I think it should be in a separate diff though.

modules/ecash-lib/src/token/slp.ts
26 ↗(On Diff #47502)

Agreed; I can add a function that verifies it.
Should we add an enum here, so TypeScript will catch it?

125 ↗(On Diff #47502)

Afaik CashTab will have to disable the token checks done by Chronik to accomplish this though, no?
Vin added it because he wanted users to be very explicitly opting into the burn, can add the spec.
ALP fixed it by allowing partial burns that are also explicit.

modules/ecash-lib/tests/slp.test.ts
135 ↗(On Diff #47502)

we need some fuel for txs spending these, and just adding sats to the token UTXO makes that easier

268 ↗(On Diff #47502)

Yes default is version 1, but I can change it to 2, I don't think it should be in this diff though.

939 ↗(On Diff #47502)

Yeah, should I add a comment? Either way it's not too important IMO, whether 1 or 1000 here

1005 ↗(On Diff #47502)

You have to add a 0 qty if you want to move the recipient output to outIdx = 2. No big reason I did this, most because I copy pasted the previous SEND tx which had two outputs.

And yes—slpSend(childTokenId, SLP_NFT1_CHILD, [1]) would be the normal usage, where the NFT goes to outIdx = 1.

verify inputs to slpGenesis, slpMint etc.

modules/ecash-lib/src/token/slp.ts
163 ↗(On Diff #47531)

not sure there's really a case where it matters for this particular comparison. should use !== (strict) vs != unless there is some specific case you want to allow, e.g. '' == 0

163 ↗(On Diff #47531)
bytesofman requested changes to this revision.EditedApr 30 2024, 21:28

changes

  • comment on the nft child test

enum to verify token type if you think it's a good idea no need to add complexity to solve an already-solved problem

modules/ecash-lib/src/token/slp.ts
26 ↗(On Diff #47502)

up to you best way to implement. i'm less familiar with typescript but enum sounds like a good idea, since there are only a few possible values.

125 ↗(On Diff #47502)

Yes, Cashtab has to pass a flag to chronik in order to successfully burn SLP tokens. I have it as a 'todo' to implement explicit burning (the function to do so is in Cashtab but not implemented) -- but is is low priority because we can only really use this when the user happens to want to burn the exact amount of a specific utxo. We would still need to support burn by send to give the user full control over the qty burned.

So, explicit burning is a good add to the spec. The old way covers all use cases (burn all or burn some), and the new way covers only a subset (burn all).

modules/ecash-lib/tests/slp.test.ts
268 ↗(On Diff #47502)

ok, should be in before we release ecash-lib

939 ↗(On Diff #47502)

comment would be useful. I would never have noticed had I not been implementing this for the last week.

it's the kind of thing an unsuspecting dev could just copy paste into an nft minting app...also it "works" so mb never discovered, dev might just think "oh so you just spend 1000 per NFT, cool."

seems a bit far fetched but in practice wouldn't surprise me at all, I personally have done similar things trying to reverse engineer a spec based on what I see in a library.

This revision now requires changes to proceed.Apr 30 2024, 21:28
modules/ecash-lib/tests/slp.test.ts
939 ↗(On Diff #47502)

I can also change it to 1 token, if you think these tests will serve exemplary then it’s a good idea

modules/ecash-lib/tests/slp.test.ts
939 ↗(On Diff #47502)

yeah this would be best. i know I'll be using these tests to model Cashtab txs. Hope others do the same.

tobias_ruck added inline comments.
modules/ecash-lib/src/token/slp.ts
26 ↗(On Diff #47502)

One downside is that it’s more verbose, you have to prefix the enum name every time you use it:

So slpGenesis(SlpTokenType.FUNGIBLE,…) instead of slpGenesis(SLP_FUNGIBLE,…)

The first one would only accept the enum, whereas the second one could accept any number (and then fail at runtime).

Up to you though, what would you want?

125 ↗(On Diff #47502)

I saw that Vin does a „preburn“ if the value doesn’t match exactly, and then burns that. Funny name choice imo

modules/ecash-lib/src/token/slp.ts
26 ↗(On Diff #47502)

don't do it.

125 ↗(On Diff #47502)

i see some advantages to this but ultimately i think it will just always be a quirk of SLP1

later tokens will have better ways of handling burns. SLP1 has effective ways but you gotta know what you're doing.

use 1 single token when creating an NFT

seems like everything is resolved after this passes tests, or did I forget anything?

seems like everything is resolved after this passes tests, or did I forget anything?

looks good, i am building the node to run the tests locally

Tail of the build log:

wallet_multiwallet.py --descriptors        | ✓ Passed  | 36 s
wallet_multiwallet.py --usecli             | ✓ Passed  | 9 s
wallet_reorgsrestore.py                    | ✓ Passed  | 3 s
wallet_resendwallettransactions.py         | ✓ Passed  | 4 s
wallet_send.py                             | ✓ Passed  | 6 s
wallet_startup.py                          | ✓ Passed  | 2 s
wallet_timelock.py                         | ✓ Passed  | 1 s
wallet_txn_clone.py                        | ✓ Passed  | 1 s
wallet_txn_clone.py --mineblock            | ✓ Passed  | 2 s
wallet_txn_doublespend.py                  | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock      | ✓ Passed  | 3 s
wallet_watchonly.py                        | ✓ Passed  | 1 s
wallet_watchonly.py --usecli               | ✓ Passed  | 1 s
chronik_avalanche.py                       | ○ Skipped | 0 s
chronik_block.py                           | ○ Skipped | 0 s
chronik_block_info.py                      | ○ Skipped | 0 s
chronik_block_txs.py                       | ○ Skipped | 0 s
chronik_blockchain_info.py                 | ○ Skipped | 0 s
chronik_blocks.py                          | ○ Skipped | 0 s
chronik_chronik_info.py                    | ○ Skipped | 0 s
chronik_disable_token_index.py             | ○ Skipped | 0 s
chronik_disallow_prune.py                  | ○ Skipped | 0 s
chronik_lokad_id_group.py                  | ○ Skipped | 0 s
chronik_mempool_conflicts.py               | ○ Skipped | 0 s
chronik_pause.py                           | ○ Skipped | 0 s
chronik_plugins_setup.py                   | ○ Skipped | 0 s
chronik_raw_tx.py                          | ○ Skipped | 0 s
chronik_resync.py                          | ○ Skipped | 0 s
chronik_script_confirmed_txs.py            | ○ Skipped | 0 s
chronik_script_history.py                  | ○ Skipped | 0 s
chronik_script_unconfirmed_txs.py          | ○ Skipped | 0 s
chronik_script_utxos.py                    | ○ Skipped | 0 s
chronik_serve.py                           | ○ Skipped | 0 s
chronik_spent_by.py                        | ○ Skipped | 0 s
chronik_token_alp.py                       | ○ Skipped | 0 s
chronik_token_broadcast_txs.py             | ○ Skipped | 0 s
chronik_token_burn.py                      | ○ Skipped | 0 s
chronik_token_id_group.py                  | ○ Skipped | 0 s
chronik_token_parse_failure.py             | ○ Skipped | 0 s
chronik_token_script_group.py              | ○ Skipped | 0 s
chronik_token_slp_fungible.py              | ○ Skipped | 0 s
chronik_token_slp_mint_vault.py            | ○ Skipped | 0 s
chronik_token_slp_nft1.py                  | ○ Skipped | 0 s
chronik_tx.py                              | ○ Skipped | 0 s
chronik_tx_removal_order.py                | ○ Skipped | 0 s
chronik_ws.py                              | ○ Skipped | 0 s
chronik_ws_ordering.py                     | ○ Skipped | 0 s
chronik_ws_ping.py                         | ○ Skipped | 0 s
chronik_ws_script.py                       | ○ Skipped | 0 s
feature_bind_port_discover.py              | ○ Skipped | 0 s
feature_bind_port_externalip.py            | ○ Skipped | 0 s
interface_usdt_net.py                      | ○ Skipped | 0 s
interface_usdt_utxocache.py                | ○ Skipped | 0 s
interface_usdt_validation.py               | ○ Skipped | 0 s

ALL                                        | ✓ Passed  | 1347 s (accumulated) 
Runtime: 270 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
This revision is now accepted and ready to land.Apr 30 2024, 23:59
This revision was automatically updated to reflect the committed changes.