The previous calcP2pkhByteCount function is significantly more granula and accurate, with detailed documentation that makes it easier to extend to p2sh, uncompressed pubkeys...etc. I think it's a net positive to use it instead of coinselect's version of estimates.

Is BLANK_OUTPUT representing the unspendable OP_RETURN output at index 0?

Previous feedback from Fabien on usage of Buffer here:

What is the reason for using a Buffer here ? IMO we should not enforce the callsite to add an external dependency to use our library.
What about passing the hex string, and the doing the buffer conversion internally if that is required ?

Afaict Buffer is a node feature. Are we expecting all calling apps to use node ?

This function is almost identical to inputBytes, only difference being the constants used and the name.

I looked into combining them but imo requiring inputOrOutput to be specified as a param to gate different constants is clumsier than keeping distinct functions.

i think it's good to use the existing baseline here with coinselect values so that we can have a diff later focused on improving them. Don't necessarily need to publish this library after this diff.

In this case, it's testing how the fee would change if a change output is added.

If a change output is less than dust, don't have one, even though this does make the fee higher.

The sum function is defined in utils.js -- it's adding up all the value keys of the`targetOutputs`.

sumValues is probably a better name here, replaced.

Note: the previous sum function from coinselect also performed validation checks to make sure the value key was an integer and not infinite. I have dropped these checks, since parseInt is required to convert string types, and this will also eliminate decimals.

This means the function could create unexpected results if called with inputs and outputs that have decimal places at the value key.

imo it is ok to assume uncorrupted responses from the indexer. failure mode of rounding to the nearest satoshi is acceptable. failure mode if user supplying target outputs with decimal places and these in turn being rounded to the nearest satoshi also acceptable.

This is correct in the linter ... but yes, it is a weird JS exception that this is valid only for one-line if statements, which are used nowhere else in the monorepo. Changed.

The way inputBytes and outputBytes functions are written -- it does need to be a Buffer (or, at least, some kind of byte array, where script.length is equal to byteCount -- vs hex string where byteCount would be script.length / 2.

Buffer is what's used by TransactionBuilder, so it is convenient for this library to support this type. That said, easy enough to add support for hex strings as well.

Are we expecting all calling apps to use node ?

This is a recurring issue in JS apps that work with cryptocurrency -- bitcoin, BCH, ETH, etc.

Buffer is often used in encryption libraries and to build transactions. So, dependencies support Buffer. But your React app might not support this. So it needs tools like webpack to be configured so that these types are supported.

It's possible to use webpack in the lib itself to "build" the dependency before you publish it -- i.e., the dependency will be a converted version of what is in the source code that, say, works on an earlier standard of JS or one without node-specific types. But since amost all app devs are using webpack or some equivalent, and it's not possible to predict their preferred settings, this is usually not worth it.