Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC71f4a9f3c96c: [Cashtab] Update getSlpSendTargetOutputs to match optimizations of…

Summary

Depends on D15100

Fix typo in toLocaleString() implementation
Use tokenUtxo.slpToken.amount which, in the chronik utxo object, is already undecimalized (so save the op)

Test Plan

npm test

Diff Detail

Repository

rABC Bitcoin ABC

Branch

slp-burn-fn

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 26216
Build 52004: Build Diff	cashtab-tests
Build 52003: arc lint + arc unit

Event Timeline

bytesofman created this revision.Jan 5 2024, 18:30

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 5 2024, 18:30

bytesofman requested review of this revision.Jan 5 2024, 18:30

bytesofman added inline comments.Jan 5 2024, 18:32

cashtab/src/slpv1/index.js
67 ↗	(On Diff #43949)	not caught previously as unit tests are working with a token that does not have any decimals. this case is now tested for getSlpBurnTargetOutput only impact is structure of error msg, I don't think it's worth adding new utxo mocks to also test it here

Harbormaster completed remote builds in B26186: Diff 43949.Jan 5 2024, 18:33

AFAICT it's not only the error message that is changed, but also the behavior.
Say you have a token T with 1 decimal and 2 utxos worth 10.6T each. If you want to send 21T it would fail before that change and succeed after the change.
This change of behavior deserves a test.

This revision now requires changes to proceed.Jan 5 2024, 20:30

add unit test

In D15101#338971, @Fabien wrote:

AFAICT it's not only the error message that is changed, but also the behavior.
Say you have a token T with 1 decimal and 2 utxos worth 10.6T each. If you want to send 21T it would fail before that change and succeed after the change.
This change of behavior deserves a test.

tokenUtxo.tokenQty !== tokenUtxo.slpToken.amount ... er well, it does, but only when decimals = 0.

new BN(tokenUtxo.tokenQty).times(10 ** decimals) === new BN(tokenUtxo.slpToken.amount) --> always true, assuming no issues with chronik indexing and delivery

chronik stores the "what's the actual amount when we take into account decimals" value at tokenUtxo.tokenQty as a string.

The SLP-spec-friendly way of storing it (integer) is kept at tokenUtxo.slpToken.amount as a string.

So, we should just use the string that is already an integer, instead of re-converting this new string.

Harbormaster completed remote builds in B26207: Diff 43978.Jan 8 2024, 18:01

bytesofman added inline comments.Jan 8 2024, 18:02

cashtab/src/slpv1/fixtures/mocks.js
113 ↗	(On Diff #43978)	note that `slpToken.amount` has one more zero than `tokenQty`, which is how chronik returns data for an slpv1 token with `decimals = 1`
cashtab/src/slpv1/fixtures/vectors.js
123 ↗	(On Diff #43978)	`04`, pushdata `534c5000`, SLP `01`, pushdata `01`, slp version `04`, pushdata `53454e44`, SEND `20`, pushdata `7bbf452698a24b138b0357f689587fc6ea58410c34503b1179b91e40e10bba8b`, tokenId `08`, pushdata `0000befe6f672000`, token amount sent, integer, 210000000000000 (note this is 21T with an "extra zero" bc token has 1 decimal place) `08`, pushdata `000001d1a94a2000`, token amount change, integer, 2000000000000 (10.6T + 10.6T = 21.2T, sent 21T, change is 200B, note figure here has "extra zero" bc token has 1 decimal place)

You misunderstood what I wrote, and that's my fault. I wrote the token is named T, so 10.6T is not Tera but 10.6 of the token quantity.
Let's call the token FOO, you have twice 10.6 FOO and want to send 21 FOO => you got the decimal case that is worth checking in the test.

This revision now requires changes to proceed.Jan 8 2024, 20:00

add unit test for 10.6 and 21

Harbormaster completed remote builds in B26216: Diff 43992.Jan 9 2024, 16:20

rebase

Harbormaster completed remote builds in B26218: Diff 43994.Jan 9 2024, 16:23

Fabien accepted this revision.Jan 10 2024, 08:38

This revision is now accepted and ready to land.Jan 10 2024, 08:38

Closed by commit rABC71f4a9f3c96c: [Cashtab] Update getSlpSendTargetOutputs to match optimizations of… (authored by bytesofman). · Explain WhyJan 10 2024, 14:00

This revision was automatically updated to reflect the committed changes.

bytesofman added a commit: rABC71f4a9f3c96c: [Cashtab] Update getSlpSendTargetOutputs to match optimizations of….