Page MenuHomePhabricator

[Apps][Examples] Sending XECs
ClosedPublic

Authored by emack on Jul 3 2023, 14:27.

Details

Reviewers
bytesofman
Fabien
Group Reviewers
Restricted Project
Commits
rABC2f3d77d9adbf: [Apps][Examples] Sending XECs
Summary

An example using the bitgo transaction builder to:

  • collate XEC input UTXOs (filtering out eToken UTXOs)
  • generate tx outputs
  • sign the individual UTXOs
  • build the transaction into raw hex
  • broadcast the raw hex via chronik onto the network
Test Plan

npm test
pre-fund an existing wallet and copy wallet address and mnemonic into sendXec.js
npm run sendXec <destinationAddress> <sendXecAmount>

Diff Detail

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
apps/examples/scripts/sendXec.js
35 ↗(On Diff #41235)

Already specified in function comments that sendAmount input must be a number

105 ↗(On Diff #41235)

is chronik's output for utxo.value a string? if so, worth a comment

120 ↗(On Diff #41235)

could use some comments here

the min fee is 1 sat/byte. To use this, you need to make sure you have accurately determined the bytecount of your tx.

...really, for an example like this, we should accurately calculate the bytecount of the tx and use 1.0 -- which we should also extend to Cashtab

141–142 ↗(On Diff #41235)

For an example, we should use "the" dust amount - 546 sats

Cashtab's use of 550 is arbitrary + an additional safeguard against confusion with SLP utxos

212 ↗(On Diff #41235)

Not clear why we are using Number() so much

If the input type is a Number, just use that.

We only really need Number if we are converting a string.

212–217 ↗(On Diff #41235)
264 ↗(On Diff #41235)

For a general constant like this, use style of

const ECASH_DECIMALS = 2

apps/examples/test/sendXec.test.js
109 ↗(On Diff #41235)

Add unit tests here with large numbers approaching total XEC supply to verify that Number is ok

This revision now requires changes to proceed.Jul 4 2023, 13:31
emack marked 8 inline comments as done.

updated per various feedback, added unit test for an XEC amount equal to current circulating supply and updated example to use 1 sat / byte for tx fee calculation

bytesofman requested changes to this revision.Jul 5 2023, 03:29
bytesofman added inline comments.
apps/examples/scripts/sendXec.js
144–145 ↗(On Diff #41253)
210–211 ↗(On Diff #41253)
214 ↗(On Diff #41253)
215–216 ↗(On Diff #41253)
217 ↗(On Diff #41253)

need to reference where all these numbers are coming from

the source in cashtab (some guy's github comment from 2017) is not that good, but it's better than nothing

268–269 ↗(On Diff #41253)
This revision now requires changes to proceed.Jul 5 2023, 03:29
apps/examples/test/sendXec.test.js
117 ↗(On Diff #41253)

add

  • one that goes over total circulating supply to prove that usage of Number is acceptable for xec txs
  • one that does the same but also has 2 decimal places, e.g. totalCirculatingSuply.42
emack marked 6 inline comments as done.

Updated comments and variables as per feedback, added two unit tests for greater than circulating supply and with decimal points

I think this example demonstrates that we are missing a higher level API to interact with the network.
We should think at how such an example SHOULD look like and make it happen without having the implementation details appear through the examples.

Even without looking at the code, this is an example for sending an XEC tx, assuming you already have some wallet object at hand, and still it takes >300 lines of code to achieve that.

apps/examples/scripts/sendXec.js
34 ↗(On Diff #41256)

This doesn't seem relevant for the example

37 ↗(On Diff #41256)

Nit: capitalize everywhere it's needed

57 ↗(On Diff #41256)

That's not really what you want to check, but rather than the balance is sufficient

78 ↗(On Diff #41256)

2 remarks;

  • You can avoid this whole explanation by providing a simple isToken() method in some of the libs.
  • I don't understand how you can get a mix of eToken and XEC, does chronik returns tokens when calling for utxos ?
102 ↗(On Diff #41256)

There should be a function in the API to do this. You can't really expect every user to implement a coin selection algorithm, which is not necessarily an easy task, before being able to create some transaction.

139 ↗(On Diff #41256)

This is the kind of things that we don't want a user to have to deal with, and we should provide a better API

emack planned changes to this revision.Jul 6 2023, 12:11
emack marked an inline comment as done.
emack marked 6 inline comments as done.
  • Installed the new ecash-coinselect module to encapsulate the coin selection logic and significantly simplify this example
  • Added mocha-suppress-logs to reduce output noise
  • Updated comments capitalization throughout example
apps/examples/scripts/sendXec.js
34 ↗(On Diff #41256)

The sendAmount input to this function is in XEC, where we can receive values such as 500.55 XEC. BigInt can't take decimal points.

37 ↗(On Diff #41256)

Updated throughout file.

57 ↗(On Diff #41256)

This whole block is now superseded by the ecash-coinselect library

78 ↗(On Diff #41256)

Superseded.

102 ↗(On Diff #41256)

Superseded

139 ↗(On Diff #41256)

We have a separate task T2912 for this. It's likely easier to submit a PR to bitgo rather than forking our own tx builder, but both will take some time.

Fabien requested changes to this revision.Aug 7 2023, 08:22
Fabien added inline comments.
apps/examples/scripts/sendXec.js
37 ↗(On Diff #41731)

Can't you simply call the input param sendAmountInXec rather than copying ? The comment can go above or where the conversion happens

82 ↗(On Diff #41731)

Why do you need parseInt if you're using native js integers as per your comment above?

114 ↗(On Diff #41731)

This and the above are constants and should not be in the loop

128 ↗(On Diff #41731)

it's obvious, the comment isn't useful

142 ↗(On Diff #41731)

Is there anything more than the txid in here ? If not you should just return that so it's straightforward to use

185 ↗(On Diff #41731)

It's certainly useful outside of this example. Is this never done anywhere else in our codebase ?

This revision now requires changes to proceed.Aug 7 2023, 08:22
emack marked 6 inline comments as done.

Responding to feedback

apps/examples/scripts/sendXec.js
37 ↗(On Diff #41731)

Updated and comment moved

82 ↗(On Diff #41731)

Good point, removed and tested all ok with bitgo

114 ↗(On Diff #41731)

Moved out as constants

128 ↗(On Diff #41731)

Removed

142 ↗(On Diff #41731)

Tobias plans on adding associated txHash and other mempool info into this return object so I would prefer to keep this as is so we don't have to update this later to align with new chronik-client versions.

185 ↗(On Diff #41731)
Fabien requested changes to this revision.Aug 8 2023, 11:14
Fabien added inline comments.
apps/examples/scripts/sendXec.js
113 ↗(On Diff #41746)

this should also be outside of the loop

142 ↗(On Diff #41731)

OK

185 ↗(On Diff #41731)

It's worth looking into moving this out of cashtab then. Also cashtab uses bignum, is that required ?
Can be done after this diff though

This revision now requires changes to proceed.Aug 8 2023, 11:14
emack marked 2 inline comments as done.

Responding to feedback

apps/examples/scripts/sendXec.js
113 ↗(On Diff #41746)

Moved to above the loop

185 ↗(On Diff #41731)

Cashtab's use of bignum is a legacy thing from the fork days and is on the list of things Joey and I are looking to deprecate. Pretty sure we won't be handling numbers a few magnitudes larger than our total supply.

And yes in terms of moving it into a separate module, have added it to the workboard as T3260.

bytesofman added inline comments.
apps/examples/README.md
74 ↗(On Diff #41748)

mb worth adding the requirement of manual input in sendXec.js

This revision is now accepted and ready to land.Aug 11 2023, 14:12
emack marked an inline comment as done.

Updated usage instructions

This revision was landed with ongoing or failed builds.Aug 11 2023, 14:33
This revision was automatically updated to reflect the committed changes.