Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC8dc6cac67987: [Cashtab] [Chronik] [Tx Gen] Sign and build Tx

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 signs the input UTXOs and converts the transaction builder instance into a raw tx hex ready for broadcast to a node.

This function returns a hex string.

Test Plan

npm test
observe successful execution of signAndBuildTx() unit tests

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

emack created this revision.Jul 5 2022, 05:14

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 5 2022, 05:14

emack requested review of this revision.Jul 5 2022, 05:14

Harbormaster completed remote builds in B19553: Diff 34243.Jul 5 2022, 05:17

bytesofman requested changes to this revision.Jul 5 2022, 18:00

bytesofman added inline comments.

web/cashtab/src/utils/__tests__/cashMethods.test.js
179 ↗	(On Diff #34243)	Add a test with multiple input utxos Add a test with multiple input utxos and multiple output utxos
web/cashtab/src/utils/cashMethods.js
11 ↗	(On Diff #34243)	Let's call this `signAndBuildTx`, just to be more descriptive -- also change references to this function name in comments

This revision now requires changes to proceed.Jul 5 2022, 18:00

Added additional unit test coverage for multiple input and output UTXOs.
Function name change.

emack retitled this revision from [Cashtab] [Chronik] [Tx Gen] Build Tx to [Cashtab] [Chronik] [Tx Gen] Sign and build Tx.Jul 6 2022, 03:51

emack edited the summary of this revision. (Show Details)

emack edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B19572: Diff 34268.Jul 6 2022, 03:56

bytesofman requested changes to this revision.Jul 6 2022, 23:11

bytesofman added inline comments.

web/cashtab/src/utils/__tests__/cashMethods.test.js
244 ↗	(On Diff #34268)	I can see what you're up to here, but I don't think we can assume some future dev will. It's confusing that this output is made from `mockMultipleInputUtxos` The best way to do all of these tests is to use data from real transactions. I can see some idealized cases being used for these fragment functions. We'll need to test the new combined `sendXec` function against all of the old txs and unit tests as well, so might as well start now -- there are lots of input utxos and tx hashes in those mocks. If there is not a good candidate there, at least include a few sentences about what you are doing to build this mocked output.
web/cashtab/src/utils/cashMethods.js
13 ↗	(On Diff #34268)	At this point, is it possible to do any kind of validation check on `txBuilder`? It should have inputs and outputs. I'm not sure what this JS object looks like though. If this is not practical, use `try...catch.` in case `txBuilder.build()` or `txBuilder.sign()` fails in some way.

This revision now requires changes to proceed.Jul 6 2022, 23:11

cashMethods.js

see https://reviews.bitcoinabc.org/D11719#change-YgPIGHoiluM7 for what the txBuilder object looks like as it is passed into this signAndBuildTx function. The inputs are empty until the inputUtxos are signed with the wif. Therefore the input validation at the start of cashMethods.js is only checking against txBuilder.transaction.tx.outs.length
try catch block added around txBuilder.build() and txBuilder.sign()

cashMethods.test.js

real data now being used in mockTxBuilderData.js to reflect one to one and one to many send XEC txs. There weren't any existing mock candidates for this because the same inputUtxo object needs to also have the wif when passed into the signAndBuildTx() function to sign the input utxos.

Harbormaster completed remote builds in B19601: Diff 34308.Jul 8 2022, 09:09

bytesofman requested changes to this revision.Jul 8 2022, 16:43

bytesofman added inline comments.