Page MenuHomePhabricator
Differential D16081

[ecash-lib] Add SLP support
ClosedPublic
Actions

Authored by tobias_ruck on Tue, Apr 30, 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

tobias_ruck created this revision.Tue, Apr 30, 01:18
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Apr 30, 01:18
tobias_ruck requested review of this revision.Tue, Apr 30, 01:18
tobias_ruck updated this revision to Diff 47501.Tue, Apr 30, 01:26

add SLP BURN

tobias_ruck updated this revision to Diff 47502.Tue, Apr 30, 01:32

improve readability

Harbormaster completed remote builds in B28806: Diff 47500.Tue, Apr 30, 01:36
Harbormaster completed remote builds in B28807: Diff 47501.Tue, Apr 30, 01:51
Harbormaster completed remote builds in B28808: Diff 47502.Tue, Apr 30, 01:58
tobias_ruck added a child revision: D16087: [ecash-agora] Add `AgoraOneshot`.Tue, Apr 30, 15:38
bytesofman requested changes to this revision.Tue, Apr 30, 16:34

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.Tue, Apr 30, 16:34
tobias_ruck added a comment.Tue, Apr 30, 17:38

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.

tobias_ruck updated this revision to Diff 47531.Tue, Apr 30, 19:36

verify inputs to slpGenesis, slpMint etc.

Harbormaster completed remote builds in B28827: Diff 47531.Tue, Apr 30, 20:04
tobias_ruck updated this revision to Diff 47534.Tue, Apr 30, 20:27

add BURN spec

Harbormaster completed remote builds in B28830: Diff 47534.Tue, Apr 30, 20:45
bytesofman added inline comments.Tue, Apr 30, 21:18
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.EditedTue, Apr 30, 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.Tue, Apr 30, 21:28
tobias_ruck added inline comments.Tue, Apr 30, 22:05
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

bytesofman added inline comments.Tue, Apr 30, 22:06
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 marked an inline comment as done.Tue, Apr 30, 22:11
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

bytesofman added inline comments.Tue, Apr 30, 22:34
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.

tobias_ruck updated this revision to Diff 47537.Tue, Apr 30, 22:40

use 1 single token when creating an NFT

tobias_ruck added a comment.Tue, Apr 30, 22:42

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

bytesofman added a comment.Tue, Apr 30, 22:47
In D16081#364187, @tobias_ruck wrote:

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

Harbormaster completed remote builds in B28833: Diff 47537.Tue, Apr 30, 23:09
teamcity added a comment.Tue, Apr 30, 23:09
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

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
tobias_ruck updated this revision to Diff 47538.Tue, Apr 30, 23:26

rerun ci

Harbormaster completed remote builds in B28834: Diff 47538.Tue, Apr 30, 23:39
bytesofman accepted this revision.Tue, Apr 30, 23:59
This revision is now accepted and ready to land.Tue, Apr 30, 23:59
Closed by commit rABC2c1f37ee6d76: [ecash-lib] Add SLP support (authored by tobias_ruck). · Explain WhyWed, May 1, 00:02
This revision was automatically updated to reflect the committed changes.
tobias_ruck added a commit: rABC2c1f37ee6d76: [ecash-lib] Add SLP support.

Revision Contents
Changeset List

PathSize
modules/
ecash-lib/
src/
4 lines
test/
4 lines
token/
4 lines
64 lines
212 lines
tests/
1124 lines
test/
functional/
setup_scripts/
3 lines

Diff 47540

View Options

modules/ecash-lib/src/index.ts

Loading...
View Options

modules/ecash-lib/src/test/testRunner.ts

Loading...
View Options

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

Loading...
View Options

modules/ecash-lib/src/token/slp.test.ts

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

test/functional/setup_scripts/setup_framework.py

Loading...