Page MenuHomePhabricator

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

Authored by emack on Sep 13 2023, 10:41.

Details

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 requested review of this revision.Sep 13 2023, 10:41
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
emack marked 4 inline comments as done.

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

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
emack marked 2 inline comments as done.
emack edited the summary of this revision. (Show Details)
  • Added p2pkh script type validation on outputArray
  • Reversed the change output logic

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

Fabien added inline comments.
modules/ecash-coinselect/src/utxo.js
48 ↗(On Diff #42196)
This revision is now accepted and ready to land.Sep 14 2023, 17:35
emack marked an inline comment as done.

Updated error message