[Cashtab] [Chronik] [Tx Gen] Generate Tx Output
AbandonedPublic
Actions

Authored by emack on Jul 5 2022, 03:33.

Details

Reviewers

bytesofman

Group Reviewers

Restricted Project

Summary

As per T2503, this is part of a series of diffs to refactor the transaction generation process within useBCH.

This diff is a standalone utility function which adds tx outputs to the existing transaction builder instance, catering for both one to one and one to many send XEC txs.

The function returns an object containing the transaction builder instance.

Test Plan

npm test
observe successful execution of generateTxOutput() unit tests

Diff Detail

Repository

rABC Bitcoin ABC

Branch

generateTxOutput

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 19552
Build 38823: Build Diff	cashtab-tests
Build 38822: arc lint + arc unit

Event Timeline

emack created this revision.Jul 5 2022, 03:33

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 5 2022, 03:33

emack requested review of this revision.Jul 5 2022, 03:33

Harbormaster completed remote builds in B19552: Diff 34242.Jul 5 2022, 03:36

bytesofman requested changes to this revision.Jul 5 2022, 17:50

bytesofman added inline comments.

web/cashtab/src/components/Common/Ticker.js
48	Add a link to relevant part of the source
web/cashtab/src/utils/__tests__/cashMethods.test.js
210	Can we test for the whole `outputObj` or is there some jest conflict? We should test the whole expected output.
219	Is it just a coincidence that the change here is an order of magnitude up? i.e. satoshisToSend is 70221900 and inputUtxoValue is 702219000 (one extra zero) -- is this intended?
241	Can we test the full output?
web/cashtab/src/utils/cashMethods.js
43	What is the value we are adding by including this code? I'm not saying we need to remove it just yet -- but please clarify in the review the expected impact of including it.

This revision now requires changes to proceed.Jul 5 2022, 17:50

emack marked 4 inline comments as done.Jul 6 2022, 02:36

emack added inline comments.

web/cashtab/src/utils/__tests__/cashMethods.test.js
219	yes the extra magnitude added for this unit test is to ensure there is more than enough input UTXOs to cover for satoshisToSend and the txFee. Will adjust in revision to be more realistic.
web/cashtab/src/utils/cashMethods.js
43	It looks like when the insufficient funds error is thrown here and caught in sendXec, the catch block doesn't actually do anything with this error code - see here https://github.com/Bitcoin-ABC/bitcoin-abc/blob/5eabea5d08b5988713c7d3563bb927371e180755/web/cashtab/src/hooks/useBCH.js#L1684-L1698 On that basis I don't think we need this error code even in the existing codebase. This also makes the error codes in Ticker.js redundant as well if we decide to remove this.

Superseded by D11719

bytesofman added inline comments.Jul 6 2022, 22:54

web/cashtab/src/utils/__tests__/cashMethods.test.js
219	It's annoying, but should aim to make every test from an actual transaction. You can do this by `console.log` ing inputs and then actually sending a tx in Cashtab, and then going back and putting the component parts into this refactored function and ensuring you get the same output. Ultimately, we will be combining all of these functions together and comparing them with the existing unit tests. So the txs in the existing unit test are also a good resource.
web/cashtab/src/utils/cashMethods.js
43	OK, please remove