Amount types mismatch in psbtv2.ts - PR proposal (breaking change)

[landabaso]

In the current implementation, the amount parameter in the following declarations is sometimes defined as a Buffer and other times as a number:

setOutputAmount(outputIndex: number, amount: number);

setInputWitnessUtxo(
  inputIndex: number,
  amount: Buffer,
  scriptPubKey: Buffer
);

(also getInputWitnessUtxo returns a Buffer for the amount)

This can cause confusion and make it easier to make errors, as the user must remember to use uint64LE(amount) when using setInputWitnessUtxo, but not when using setOutputAmount. Additionally, uint64LE is not exposed in the API.

I propose a change to consistently use number for the amount parameter in both declarations, as follows:

setOutputAmount(outputIndex: number, amount: number);

setInputWitnessUtxo(
  inputIndex: number,
  amount: number,
  scriptPubKey: Buffer
);

(also change getInputWitnessUtxo)

This would simplify the API and reduce the risk of errors. However, this would be a breaking change and would require careful consideration of its impact on existing users.

Please let me know your thoughts on this proposal. Thank you for the great work on this library, I've been able to use it to spend wsh miniscripts with the Ledger with no issues at all.

[bigspider]

Aaah, I flip out whenever I see number in JS; but actually it's fine here as you never need more than 51 bits for amounts in bitcoin (and numbers are precise until 53 bits).

I think it could make sense to use a normalizing function like:

function normalizeAmount(amount: Buffer | number): Buffer {
  if (Buffer.isBuffer(amount)) return amount;
  else return uint64LE(amount);
}

and apply it to all the setters with amounts (accepting Buffer | number):

   amount = normalizeAmount(amount);

so that the amount arguments would accept number as a preferred parameter, but it still works if a Buffer is passed.

[landabaso]

I also considered this solution. It would resolve the issue while still maintaining backwards compatibility. This could also be done with setOutputAmount for symmetry.

However, there is a concern regarding adding ambiguity to the arguments. What should getInputWitnessUtxo return for the amount?

getInputWitnessUtxo(inputIndex: number): {
  readonly amount: Buffer; 
  readonly scriptPubKey: Buffer
} | undefined

If a user sets a number, they may then receive a Buffer, or vice versa. This could cause confusion.

Honestly, this is just a minor issue. If maintaining backwards compatibility is a priority, this can be left as it is. I would be happy to submit a pull request with whatever solution you find to be the most appropriate (if any).

[bigspider]

The library has never been released, so I don't think it's a problem to have breaking changes. I think returning amount as a Buffer in getInputWitnessUtxo is indeed weird, as it should be the library's job to decode fields semantically.

Probably, the PsbtV2 module in its entirety should be replaced with an external psbt library once one is available.

In the latest python client library, the psbt argument now accepts a base64 string (which is internally converted from PsbtV0 to PsbtV2 if necessary), and that would probably be the safest approach; but at this time, the deserialization code isn't properly tested, so that would be too risky of a change.

[landabaso]

I'm converting a bitcoinjs-lib PsbtV0 object to psbtv2.ts field by field, without using serializers.
It functions correctly with timelocks and sequences.

Once I have obtained the signatures, I insert them back into bitcoinjs-lib, finalize the transaction, and broadcast it.

If this approach is acceptable, I will submit a PR to set the amount to number type and include a method .fromBitcoinJsLib() as a helper to initialize the PSBT.

[bigspider]

That seems a great approach (and it doesn't prevent future improvement to the API)!