Details

Reviewers

Fabien
bytesofman

Group Reviewers

Restricted Project

Commits

rABC29e90aae613a: [ecash-coinselect] Update getInputUtxos to support N x p2pkh outputs

Summary

T3225

As per feedback from D14417, this diff forms part of a series of diffs to incrementally implement the key concepts from D14417:

1. Get a working API for N x p2pkh outputs (this diff)
2. Extend with N x op_return
3. Extend with N x slp outputs
4. Extend with N x p2sh outputs

Unlike D14417 however, this API is more aligned with the existing coinselect module in passing an object array of outputs rather than using a singular output ammount.

Note: the scriptType gating logic between p2pkh and p2sh will be added when we get to the p2sh diff.

Test Plan

npm test

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

emack created this revision.Sep 13 2023, 10:41

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 13 2023, 10:41

emack requested review of this revision.Sep 13 2023, 10:41

Harbormaster completed remote builds in B25011: Diff 42171.Sep 13 2023, 10:42

bytesofman requested changes to this revision.Sep 13 2023, 16:38

bytesofman added inline comments.

modules/ecash-coinselect/README.md
32 ↗	(On Diff #42171)	not clear here that `outputArray` is different from the result of `await chronik.script("p2pkh", hash160).utxos();`
modules/ecash-coinselect/src/utxo.js
35–38 ↗	(On Diff #42171)	If we are going to support case-insensitive inputs (prob a good idea) -- lets define this at the beginning, since scriptType will be used to gate different function logic when different types are supported.
39 ↗	(On Diff #42171)	right now this is the only place where `scriptType` is used -- function should be setup with `scriptType` gating the appropriate logic, so that it's straightforward to add different types.
51 ↗	(On Diff #42171)

This revision now requires changes to proceed.Sep 13 2023, 16:38

Responding to feedback

modules/ecash-coinselect/README.md
32 ↗	(On Diff #42171)	Added additional contextual comments.
modules/ecash-coinselect/src/utxo.js
35–38 ↗	(On Diff #42171)	Updated as per feedback
39 ↗	(On Diff #42171)	Removed `scriptType` gating until we get to the p2sh diff.
51 ↗	(On Diff #42171)	Updated

emack edited the summary of this revision. (Show Details)Sep 14 2023, 01:59

Harbormaster completed remote builds in B25024: Diff 42186.Sep 14 2023, 01:59

Fabien requested changes to this revision.Sep 14 2023, 12:49

Fabien added inline comments.

modules/ecash-coinselect/src/utxo.js
39 ↗	(On Diff #42186)	Since this is assuming p2pkh outputs, you should check all the addresses are of p2pkh type. Otherwise you're computing the wrong fees silently and your tx relying on this function might be rejected.
43 ↗	(On Diff #42186)	Nit: for the sake of readability you should do the opposite. Not blocking but please reverse the logic if you have to touch the diff

This revision now requires changes to proceed.Sep 14 2023, 12:49

Added p2pkh script type validation on outputArray
Reversed the change output logic

Harbormaster completed remote builds in B25032: Diff 42195.Sep 14 2023, 14:09

Build Bitcoin ABC Diffs / Diff Testing (ecash-coinselect) failed.

Tail of the build log:

Run `npm audit` for details.

> ecash-coinselect@1.0.2 test
> mocha --reporter mocha-junit-reporter --reporter-options mochaFile=test_results/ecash-coinselect-junit.xml --reporter-options testsuitesTitle=Ecash Coinselect Unit Tests --reporter-options rootSuiteTitle=Ecash Coinselect


Error: Cannot find module 'ecashaddrjs'
Require stack:
- /work/modules/ecash-coinselect/src/utxo.js
- /work/modules/ecash-coinselect/test/utxo.test.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:1028:15)
    at Function.Module._load (node:internal/modules/cjs/loader:873:27)
    at Module.require (node:internal/modules/cjs/loader:1100:19)
    at require (node:internal/modules/cjs/helpers:119:18)
    at Object.<anonymous> (/work/modules/ecash-coinselect/src/utxo.js:5:133)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Module.replacementCompile (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:60:13)
    at Module._extensions..js (node:internal/modules/cjs/loader:1252:10)
    at Object.<anonymous> (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:64:4)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at Module.require (node:internal/modules/cjs/loader:1100:19)
    at require (node:internal/modules/cjs/helpers:119:18)
    at Object.<anonymous> (/work/modules/ecash-coinselect/test/utxo.test.js:10:5)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Module.replacementCompile (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:60:13)
    at Module._extensions..js (node:internal/modules/cjs/loader:1252:10)
    at Object.<anonymous> (/usr/lib/node_modules/nyc/node_modules/append-transform/index.js:64:4)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:169:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:530:24)
    at async importModuleDynamicallyWrapper (node:internal/vm/module:438:15)
    at async formattedImport (/work/modules/ecash-coinselect/node_modules/mocha/lib/nodejs/esm-utils.js:9:14)
    at async Object.exports.requireOrImport (/work/modules/ecash-coinselect/node_modules/mocha/lib/nodejs/esm-utils.js:42:28)
    at async Object.exports.loadFilesAsync (/work/modules/ecash-coinselect/node_modules/mocha/lib/nodejs/esm-utils.js:100:20)
    at async singleRun (/work/modules/ecash-coinselect/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at async Object.exports.handler (/work/modules/ecash-coinselect/node_modules/mocha/lib/cli/run.js:370:5)
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |    1.47 |        0 |       0 |    1.51 |                   
 utxo.js  |    1.47 |        0 |       0 |    1.51 | 7-253             
----------|---------|----------|---------|---------|-------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='1']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='68']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='0']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='22']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='0']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='3']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='1']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='66']
##teamcity[blockClosed name='Code Coverage Summary']
mv: cannot stat 'test_results/ecash-coinselect-junit.xml': No such file or directory
Build ecash-coinselect failed with exit code 1

Added ecashaddrjs as a dependency

Harbormaster completed remote builds in B25033: Diff 42196.Sep 14 2023, 14:14

Fabien accepted this revision.Sep 14 2023, 16:19

Fabien added inline comments.