Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC0849583a096c: [Cashtab] Commence refactor of transaction building

Summary

Cashtab transaction functions are overloaded and complex. They are all in the utils/ directory, with all of the other Cashtab functions.

Cashtab should have better organized functions and unit tests.

This diff starts improving Cashtab's transaction building functions by creating a new function sendXecToOneAddress. This is currently handled by sendXec, which also sends xec to many addresses, sends OP_RETURN msgs, sends encrypted msgs, and sends those to many addresses as well if it comes to that.

This function implements the new coinSelect function from ecash-coinselect to bring transaction fees down to 1 satoshi per byte (or ~1.01 satoshis/byte for txs sent to P2SH addresses).

A new supporting function signInputs is also introduced, as the existing signUtxosByAddress function returns TransactionBuilder. This is not necessary as TransactionBuilder is an object. However, it is done this way in existing transaction functions, many unit tests are there for it, and it would be a lot of makework to remove it from the existing transaction building functions that I would like to deprecate anyway. So -- new function.

Note: this diff does not implement the function anywhere in Cashtab. user-facing behavior is not impacted by this diff.

Test Plan

npm test

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bytesofman created this revision.Sep 22 2023, 22:42

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 22 2023, 22:42

bytesofman requested review of this revision.Sep 22 2023, 22:42

Harbormaster completed remote builds in B25141: Diff 42352.Sep 22 2023, 22:45

emack added a subscriber: emack.Sep 24 2023, 14:48

emack added inline comments.

cashtab/package-lock.json
44 ↗	(On Diff #42352)	I'm not seeing a `Depends on D14526` in the diff description since 2.0.0 isn't published yet. It might choke when you go to land this.
cashtab/src/transactions/index.js
56 ↗	(On Diff #42352)	Does this mean there will be a separate function for: send XEC to many addresses send XEC to many addresses with an OP_RETURN message send XEC to many addresses with an encrypted OP_RETURN message send SLP to one address send SLP to many addresses create SLP token burn SLP token send XEC to airdrop recipients send XEC to airdrop recipients with an OP_RETURN message alias registration This might start leaning towards the original issue back in 2021 where too many functions were duplicating similar code (collecting UTXOs, signing txs, broadcasting...etc)
64 ↗	(On Diff #42352)	Let's take this opportunity to improve on this vague legacy error e.g. `Send amount is less than the minimum for an eCash transaction`.
114 ↗	(On Diff #42352)	Should be in a try/catch block so you can better translate the node errors into layman speak for the user?

@emack Your questions are valid, and the missing dependency is indeed an issue: the test plan won't work without it.
Please request changes when you have such remarks: comments with no state change are more to ask a general question about the diff in a non-blocking way but also it makes it harder to spot for dev that the diff needs his attention.

This revision now requires changes to proceed.Sep 25 2023, 09:01

bytesofman marked 3 inline comments as done.Sep 25 2023, 19:17

bytesofman added inline comments.

cashtab/package-lock.json
44 ↗	(On Diff #42352)	I published the `ecash-coinselect` with an `rc` tag at 2.0.0 -- however that published version is no longer identical to the most recent version in D14526 So -- it is published, which is why the CI tests are passing on this diff. This diff will need to be rebased after 14526 goes in, at which point it will have that version of the dependency installed. However still ok to review this now, since improvements to `ecash-coinselect` are not expected to (drastically) impact how these functions are designed.
cashtab/src/transactions/index.js
56 ↗	(On Diff #42352)	There will probably be opportunities for combining some of these use cases. For example, I think this function could handle multi-send and OP_RETURN msgs almost trivially by having it accept `targetOutputs` as the param instead of `satsToSend` -- then have helper functions to build this correctly in `Send.js`. This same approach could feasibly extend to all of the use cases, depending on what we are able to implement in `ecash-coinselect` regarding SLP txs and input selection. It's hard to say now though. It will be on us to monitor for this as we review. What we have now is imo the worst of both worlds. Overloaded functions that are not easy to maintain, extend, or unit test.
114 ↗	(On Diff #42352)	imo it's okay to just keep the raw node errors. If you look at how we are implementing this in the existing tx functions, it's a big mess of repeated code blocks that is not adding significant value.

better dust error

Harbormaster completed remote builds in B25159: Diff 42378.Sep 25 2023, 19:22

Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Failed tests logs:

====== CashTab Unit Tests: Improved Cashtab transaction broadcasting functions sendXecToOneAddress throws on: Sending below dust threshold ======
Error: expect(received).rejects.toThrow(expected)

Expected substring: "dust"
Received message:   "Send amount 549 sats is less than 550, the minimum for an eCash transaction"

      62 | ) => {
      63 |     if (satsToSend < appConfig.dustSats) {
    > 64 |         throw new Error(
         |               ^
      65 |             `Send amount ${satsToSend} sats is less than ${appConfig.dustSats}, the minimum for an eCash transaction`,
      66 |         );
      67 |     }

      at sendXecToOneAddress (src/transactions/index.js:64:15)
      at Object.<anonymous> (src/transactions/__tests__/index.test.js:66:36)
    at Object.toThrow (/work/cashtab/node_modules/jest-circus/node_modules/expect/build/index.js:285:22)
    at Object.<anonymous> (/work/cashtab/src/transactions/__tests__/index.test.js:73:23)
    at Promise.then.completed (/work/cashtab/node_modules/jest-circus/build/utils.js:391:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/work/cashtab/node_modules/jest-circus/build/utils.js:316:10)
    at _callCircusTest (/work/cashtab/node_modules/jest-circus/build/run.js:218:40)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at _runTest (/work/cashtab/node_modules/jest-circus/build/run.js:155:3)
    at _runTestsForDescribeBlock (/work/cashtab/node_modules/jest-circus/build/run.js:66:9)
    at _runTestsForDescribeBlock (/work/cashtab/node_modules/jest-circus/build/run.js:60:9)
    at run (/work/cashtab/node_modules/jest-circus/build/run.js:25:3)
    at runAndTransformResultsToJestFormat (/work/cashtab/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:170:21)
    at jestAdapter (/work/cashtab/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:82:19)
    at runTestInternal (/work/cashtab/node_modules/jest-runner/build/runTest.js:389:16)
    at runTest (/work/cashtab/node_modules/jest-runner/build/runTest.js:475:34)
    at Object.worker (/work/cashtab/node_modules/jest-runner/build/testWorker.js:133:12)

Each failure log is accessible here:
CashTab Unit Tests: Improved Cashtab transaction broadcasting functions sendXecToOneAddress throws on: Sending below dust threshold

Update dust error, bump ecash-coinselect to latest version in parallel diff

ok now this version has the latest rev of D14526 installed.

Before landing, will need to publish 14526 as a non-release-candidate version. But, useful to have this up to show that 14526 works as intended in this implementation.

Harbormaster completed remote builds in B25160: Diff 42379.Sep 25 2023, 19:27

bump to latest published ecash-coinselect

Harbormaster completed remote builds in B25190: Diff 42422.Sep 27 2023, 20:46

Fabien requested changes to this revision.Sep 28 2023, 09:05

Fabien added inline comments.

cashtab/src/transactions/index.js
65 ↗	(On Diff #42422)	This is needlessly duplicated from the lib. Also how do you ensure the dustSats config is the same as the dust config in the lib ?
70 ↗	(On Diff #42422)	dito. It's nice to have a better message but no need to duplicate the logic
77 ↗	(On Diff #42422)	No need to initialize early when the below code can throw and exit before it's getting used
98 ↗	(On Diff #42422)	It's certainly not the best way to do it. Is that what Cashtab currently does ?
115 ↗	(On Diff #42422)	very bad examples as none of this can occur

This revision now requires changes to proceed.Sep 28 2023, 09:05

calc change address from wallet, remove duplicated error msgs that library catches, remove legacy example, update unit tests

Harbormaster completed remote builds in B25204: Diff 42442.Sep 28 2023, 16:44

lint

cashtab/src/transactions/index.js
98 ↗	(On Diff #42422)	Yes, this is current Cashtab behavior (artifact of legacy wallet structure). But, this is the kind of thing that should be eliminated in refactoring the transaction functions.

Harbormaster completed remote builds in B25205: Diff 42443.Sep 28 2023, 16:56

Fabien accepted this revision.Sep 29 2023, 14:50

Fabien added inline comments.

cashtab/src/transactions/fixtures/vectors.js
99 ↗	(On Diff #42443)	I know it's from the lib, but might be worth improving in a follow up, that's the benefit of a monorepo

This revision is now accepted and ready to land.Sep 29 2023, 14:50

Closed by commit rABC0849583a096c: [Cashtab] Commence refactor of transaction building (authored by bytesofman). · Explain WhySep 29 2023, 16:45

This revision was automatically updated to reflect the committed changes.

bytesofman added a commit: rABC0849583a096c: [Cashtab] Commence refactor of transaction building.

[Cashtab] Commence refactor of transaction building
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42461

cashtab/package-lock.json

cashtab/package.json

cashtab/src/transactions/tests/index.test.js

cashtab/src/transactions/fixtures/mocks.js

cashtab/src/transactions/fixtures/vectors.js

cashtab/src/transactions/index.js

[Cashtab] Commence refactor of transaction buildingClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42461

cashtab/package-lock.json

cashtab/package.json

cashtab/src/transactions/__tests__/index.test.js

cashtab/src/transactions/fixtures/mocks.js

cashtab/src/transactions/fixtures/vectors.js

cashtab/src/transactions/index.js

[Cashtab] Commence refactor of transaction building
ClosedPublic
Actions

Revision Contents
Changeset List

cashtab/src/transactions/tests/index.test.js