Page MenuHomePhabricator

[ecash-coinselect] [SLP] Part 1/2 - Add SLP utxo selection algo for token send transactions
AbandonedPublic

Authored by emack on Aug 5 2023, 12:16.

Details

Reviewers
bytesofman
Fabien
Group Reviewers
Restricted Project
Summary

T3225

This diff builds on the initial v1.0.0 release of the ecash-coinselect library and adds getSlpInputUtxos which supports SLP token send transactions by collecting the necessary XEC and SLP utxos.

Note: this will be a 2 part stacked diff with the subsequent diff adding support for token genesis and burn transactions.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 24753
Build 49098: Build Diff
Build 49097: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
emack requested review of this revision.Aug 5 2023, 12:16
emack retitled this revision from [ecash-coinselect] Add SLP utxo selection algo for token send transactions to [ecash-coinselect] [SLP] Part 1/3 - Add SLP utxo selection algo for token send transactions.Aug 5 2023, 12:24
emack edited the summary of this revision. (Show Details)
Fabien requested changes to this revision.Aug 9 2023, 10:54
Fabien added a subscriber: Fabien.
Fabien added inline comments.
modules/ecash-coinselect/src/utxo.js
72 ↗(On Diff #41732)

please avoid this pattern, it's not the first time:

if (true) {
  return true;
} else {
  return false;
}

Just return from the expression

82 ↗(On Diff #41732)

why is that ? the parse function is your own code

142 ↗(On Diff #41732)

can you explain why there is no xec tx for the genesis ?

This revision now requires changes to proceed.Aug 9 2023, 10:54
bytesofman requested changes to this revision.Aug 9 2023, 16:34
bytesofman added inline comments.
modules/ecash-coinselect/src/utxo.js
7 ↗(On Diff #41732)

no need for a special SLP_DUST const, just use DUST_AMOUNT_SATOSHIS

cashtab's novel use of 550 for dust and 546 for slp dust is not really a best practice. Just easier to have 550 vs 546 as a public facing constant.

26 ↗(On Diff #41732)

since we already have an object input here, make this part of tokenObj

emack marked 5 inline comments as done.

Responding to feedback

modules/ecash-coinselect/src/utxo.js
7 ↗(On Diff #41732)

Updated

26 ↗(On Diff #41732)

Embedded tokenAction into the tokenObj param and updated unit tests accordingly.

72 ↗(On Diff #41732)

Corrected, my bad

82 ↗(On Diff #41732)

I've updated comment to say slpToken.amount is of type Long as seen here. My parsing function is merely splitting and passing on the UTXOs from chronik, untouched.

142 ↗(On Diff #41732)

The original comment was inaccurate, I updated it to say one for initial token supply transfer and one for XEC change.

i.e. see this genesis example:

  • The first output with OP_RETURN is marked as unspendable, hence does not count towards the outputCount
  • The second output transferring the tokens are supported by the 5.46 XEC dust
  • Then the 3rd and final output for the XEC change
bytesofman added inline comments.
modules/ecash-coinselect/src/utxo.js
82 ↗(On Diff #41732)

slpToken.amount is a string though -- it used to be long but this was cumbersome to handle in javascript front ends.

For slpToken.amount -- it used to be type long, and is now a string, because it's pretty common to exceed javascript's safe number limits when dealing with SLP quantities. You need some sort of BigNumber library.

You can use javascript's native BigInt, or a library if you believe this is the best way to handle it.

Cashtab uses bignumber.js which is pretty easy but not the most lightweight option. imo it's probably best to use BigInt with helper string-manipulation functions to get the decimal right (BigInt will not handle decimals).

In Ethereum, all the JS apps use https://www.npmjs.com/package/bn.js because it is already a dependency of https://www.npmjs.com/package/ethers and/or other ETH JS libraries. In our case, bn.js is already a dependency of utxo-lib

So -- we should use bn.js or javascripts native BigInt

Because bn.js is

  • relatively lightweight
  • already a commonly used dependency in crypto-land and thus well documented and tested
  • includes more useful features compared to BigInt

this is probably the correct library to use.

This revision now requires changes to proceed.Aug 10 2023, 17:21
modules/ecash-coinselect/src/utxo.js
78 ↗(On Diff #41769)
modules/ecash-coinselect/src/utxo.js
82 ↗(On Diff #41732)

hm wish I had reviewed the comment and edited before submitting -- it's a lot of thinking out loud, and confusingly starts with a preference for BigInt and ends with a preference for "bn.js"

After the research I did while working on the comment: "use bn.js unless you find a compelling reason not to"

emack marked 3 inline comments as done.

Updated SLP quantities to use bn.js

emack retitled this revision from [ecash-coinselect] [SLP] Part 1/3 - Add SLP utxo selection algo for token send transactions to [ecash-coinselect] [SLP] Part 1/2 - Add SLP utxo selection algo for token send transactions.Aug 11 2023, 14:21
emack edited the summary of this revision. (Show Details)
bytesofman added inline comments.
modules/ecash-coinselect/src/utxo.js
96 ↗(On Diff #41776)

slpRemainder is a BN here, use BN comparison methods

107 ↗(On Diff #41776)

We don't want to return an BN values. Return string

140–145 ↗(On Diff #41776)

Let's keep the token associated logic in the token logic function.

Instead of taking tokenAction as an optional param, getInputUtxos should take outputCount as an optional param -- defaulting to 2

Then we can determine the appropriate output count in getSlpInputUtxos

This revision now requires changes to proceed.Aug 11 2023, 20:09
emack marked 3 inline comments as done.

Responding to feedback

modules/ecash-coinselect/src/utxo.js
96 ↗(On Diff #41776)

Updated

107 ↗(On Diff #41776)

Updated to return string

140–145 ↗(On Diff #41776)

Updated to have outputCount as the optional param which is set in getSlpInputUtxos.

Removed debugging statement

bytesofman added inline comments.
modules/ecash-coinselect/src/utxo.js
102

There is an available isNeg method in BN.js

See

https://www.npmjs.com/package/bn.js?activeTab=readme

This revision now requires changes to proceed.Aug 12 2023, 21:36
emack marked an inline comment as done.

Responding to feedback

Fabien requested changes to this revision.Aug 14 2023, 12:41
Fabien added inline comments.
modules/ecash-coinselect/src/utxo.js
5 ↗(On Diff #41813)

Macro bnsmile:

91 ↗(On Diff #41813)

Don't create BN(tokenObj.tokenSendAmount) at each loop increment as it's a constant

99 ↗(On Diff #41813)

The would also save that one

141 ↗(On Diff #41813)

While looking at why this is used for I noticed your fee computation is expecting all p2pkh, which an op_return is obviously not.
So your fee computation is wrong.

This revision now requires changes to proceed.Aug 14 2023, 12:41
emack marked 3 inline comments as done.

Updated getSlpInputUtxos to calculate the OP_RETURN script as a Buffer, and then calls getInputUtxos using the length of this Buffer. The resulting txFee calculation now factors in the OP_RETURN bytecount.
Note: the slpMdm library is what's used by Cashtab to generate OP_RETURN scripts as well.

modules/ecash-coinselect/src/utxo.js
5 ↗(On Diff #41813)

image.png (252×376 px, 241 KB)

91 ↗(On Diff #41813)

Added as constant outside of loop

99 ↗(On Diff #41813)

Updated as above

Updated bytecount logic

Updated function comments

Fabien requested changes to this revision.Aug 15 2023, 11:59

You should unit test getInputUtxos with the new changes, better in its own diff. I can already spot a bug related to varint encoding in calcByteCount so there might be more.

This revision now requires changes to proceed.Aug 15 2023, 11:59

You should unit test getInputUtxos with the new changes, better in its own diff. I can already spot a bug related to varint encoding in calcByteCount so there might be more.

Implementing via D14380.