From b78c6e2a5c26b5bf67c461d67b62b4087e12290f Mon Sep 17 00:00:00 2001 From: jdabbech-ledger Date: Fri, 17 Jan 2025 13:11:25 +0100 Subject: [PATCH] :art: (signer-btc): Reviews --- .../task/ExtractTransactionTask.test.ts | 2 +- .../app-binder/task/ExtractTransactionTask.ts | 20 +++--- .../app-binder/task/UpdatePsbtTask.test.ts | 4 +- .../app-binder/task/UpdatePsbtTask.ts | 26 +++++--- .../src/internal/psbt/model/Psbt.test.ts | 7 +-- .../src/internal/psbt/model/Psbt.ts | 12 ++-- .../service/value/DefaultValueParser.test.ts | 62 +++++++++++++++++++ .../psbt/service/value/DefaultValueParser.ts | 4 ++ .../psbt/service/value/ValueParser.ts | 1 + 9 files changed, 104 insertions(+), 34 deletions(-) diff --git a/packages/signer/signer-btc/src/internal/app-binder/task/ExtractTransactionTask.test.ts b/packages/signer/signer-btc/src/internal/app-binder/task/ExtractTransactionTask.test.ts index 4b0097085..6309287e1 100644 --- a/packages/signer/signer-btc/src/internal/app-binder/task/ExtractTransactionTask.test.ts +++ b/packages/signer/signer-btc/src/internal/app-binder/task/ExtractTransactionTask.test.ts @@ -77,7 +77,7 @@ describe("ExtractTransactionTask", () => { // then expect(tx).toStrictEqual( CommandResultFactory({ - data: "0x0000000000010108000000000293980000000000203009080706", + data: "0x000000000001010800000000029398000000000100000000000000000109203009080706", }), ); }); diff --git a/packages/signer/signer-btc/src/internal/app-binder/task/ExtractTransactionTask.ts b/packages/signer/signer-btc/src/internal/app-binder/task/ExtractTransactionTask.ts index df1e7b9c0..d6c58d84a 100644 --- a/packages/signer/signer-btc/src/internal/app-binder/task/ExtractTransactionTask.ts +++ b/packages/signer/signer-btc/src/internal/app-binder/task/ExtractTransactionTask.ts @@ -65,7 +65,8 @@ export class ExtractTransactionTask { const scriptSig = psbt .getInputValue(i, PsbtIn.FINAL_SCRIPTSIG) .mapOrDefault((v) => v.data, Uint8Array.from([])); - transaction.encodeInLVFromBuffer(scriptSig); + transaction.addBufferToData(encodeVarint(scriptSig.length).extract()!); + transaction.addBufferToData(scriptSig); const sequence = psbt .getInputValue(i, PsbtIn.SEQUENCE) .chain((value) => this._valueParser.getUInt32LE(value.data)) @@ -78,21 +79,22 @@ export class ExtractTransactionTask { witnessBuffer.addBufferToData(witness); } } - const ouputCount = psbt + const outputCount = psbt .getGlobalValue(PsbtGlobal.OUTPUT_COUNT) - .chain((value) => this._valueParser.getUInt32LE(value.data)) + .chain((value) => this._valueParser.getVarint(value.data)) .orDefault(0); - transaction.addBufferToData(encodeVarint(ouputCount).extract()!); - for (let o = 0; o < ouputCount; o++) { + transaction.addBufferToData(encodeVarint(outputCount).extract()!); + for (let o = 0; o < outputCount; o++) { const amount = psbt .getOutputValue(o, PsbtOut.AMOUNT) - .chain((value) => this._valueParser.getVarint(value.data)) - .orDefault(0); + .chain((value) => this._valueParser.getInt64LE(value.data)) + .orDefault(0n); const script = psbt .getOutputValue(o, PsbtOut.SCRIPT) .mapOrDefault((v) => v.data, Buffer.from([])); - transaction.addBufferToData(encodeVarint(amount).extract()!); - transaction.encodeInLVFromBuffer(script); + transaction.add64BitIntToData(amount, false); + transaction.addBufferToData(encodeVarint(script.length).extract()!); + transaction.addBufferToData(script); } transaction.addBufferToData(witnessBuffer.build()); const locktime = psbt diff --git a/packages/signer/signer-btc/src/internal/app-binder/task/UpdatePsbtTask.test.ts b/packages/signer/signer-btc/src/internal/app-binder/task/UpdatePsbtTask.test.ts index 73b688145..230668da3 100644 --- a/packages/signer/signer-btc/src/internal/app-binder/task/UpdatePsbtTask.test.ts +++ b/packages/signer/signer-btc/src/internal/app-binder/task/UpdatePsbtTask.test.ts @@ -162,7 +162,7 @@ describe("UpdatePsbtTask", () => { ], [ new Key(PsbtIn.FINAL_SCRIPTSIG).toHexaString(), - new Value(Uint8Array.from([0x01, 0x21, 0x01, 0x42])), + new Value(Uint8Array.from([0x01, 0x42, 0x01, 0x21])), ], ]), ], @@ -242,7 +242,7 @@ describe("UpdatePsbtTask", () => { ], [ new Key(PsbtIn.FINAL_SCRIPTWITNESS).toHexaString(), - new Value(Uint8Array.from([0x02, 0x01, 0x21, 0x01, 0x42])), + new Value(Uint8Array.from([0x02, 0x01, 0x42, 0x01, 0x21])), ], [ new Key(PsbtIn.FINAL_SCRIPTSIG).toHexaString(), diff --git a/packages/signer/signer-btc/src/internal/app-binder/task/UpdatePsbtTask.ts b/packages/signer/signer-btc/src/internal/app-binder/task/UpdatePsbtTask.ts index 4873c35a0..66c3a22e6 100644 --- a/packages/signer/signer-btc/src/internal/app-binder/task/UpdatePsbtTask.ts +++ b/packages/signer/signer-btc/src/internal/app-binder/task/UpdatePsbtTask.ts @@ -29,6 +29,9 @@ type UpdatePsbtTaskArgs = { signatures: PsbtSignature[]; }; +const SCHORR_SIG_LENGTH = 64; +const SCHORR_SIG_LENGTH_WITH_EXTRA_BYTE = 65; + export class UpdatePsbtTask { constructor( private readonly _args: UpdatePsbtTaskArgs, @@ -118,6 +121,12 @@ export class UpdatePsbtTask { `Missing pubkey derivation for input ${psbtSignature.inputIndex}`, ); } + // @todo handle PSBT_IN_TAP_SCRIPT_SIG as described here https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki#specification + if (psbtSignature.tapleafHash !== undefined) { + throw new Error( + "Unhandled psbt key type PSBT_IN_TAP_SCRIPT_SIG", + ); + } psbt.setInputValue( psbtSignature.inputIndex, PsbtIn.TAP_KEY_SIG, @@ -125,11 +134,11 @@ export class UpdatePsbtTask { ); }); } else { - psbt.setKeyDataInputValue( + psbt.setInputValue( psbtSignature.inputIndex, PsbtIn.PARTIAL_SIG, - psbtSignature.signature, - new Value(psbtSignature.pubkey), + new Value(psbtSignature.signature), + psbtSignature.pubkey, ); } }); @@ -325,7 +334,6 @@ export class UpdatePsbtTask { inputIndex: number, ): InternalPsbt { const psbt = fromPsbt; - // Taproot input const maybeSignature = psbt.getInputValue(inputIndex, PsbtIn.TAP_KEY_SIG); if (maybeSignature.isNothing()) { throw Error("No signature for taproot input " + inputIndex); @@ -335,13 +343,15 @@ export class UpdatePsbtTask { Uint8Array.from([]), ); - if (signature.length != 64 && signature.length != 65) { + if ( + ![SCHORR_SIG_LENGTH, SCHORR_SIG_LENGTH_WITH_EXTRA_BYTE].includes( + signature.length, + ) + ) { throw Error("Unexpected length of schnorr signature."); } const witnessBufferBuilder = new ByteArrayBuilder(); - witnessBufferBuilder.addBufferToData( - encodeVarint(1).mapOrDefault((v) => v, Uint8Array.from([1])), - ); + witnessBufferBuilder.add8BitUIntToData(0x01); witnessBufferBuilder.encodeInLVFromBuffer(signature); psbt.setInputValue( inputIndex, diff --git a/packages/signer/signer-btc/src/internal/psbt/model/Psbt.test.ts b/packages/signer/signer-btc/src/internal/psbt/model/Psbt.test.ts index a95062746..bc07981ee 100644 --- a/packages/signer/signer-btc/src/internal/psbt/model/Psbt.test.ts +++ b/packages/signer/signer-btc/src/internal/psbt/model/Psbt.test.ts @@ -186,12 +186,7 @@ describe("Psbt", () => { const psbt = new Psbt(new Map(), [new Map()]); const value = new Value(Uint8Array.of(0x03)); // when - psbt.setKeyDataInputValue( - 0, - PsbtIn.PARTIAL_SIG, - Uint8Array.from([0x42]), - value, - ); + psbt.setInputValue(0, PsbtIn.PARTIAL_SIG, value, Uint8Array.from([0x42])); // then expect(psbt).toStrictEqual( new Psbt( diff --git a/packages/signer/signer-btc/src/internal/psbt/model/Psbt.ts b/packages/signer/signer-btc/src/internal/psbt/model/Psbt.ts index b0742061d..f5dcfab35 100644 --- a/packages/signer/signer-btc/src/internal/psbt/model/Psbt.ts +++ b/packages/signer/signer-btc/src/internal/psbt/model/Psbt.ts @@ -114,18 +114,14 @@ export class Psbt { this.globalMap.set(new Key(key).toHexaString(), value); } - setInputValue(inputIndex: number, key: PsbtIn, value: Value) { - this.inputMaps[inputIndex]?.set(new Key(key).toHexaString(), value); - } - - setKeyDataInputValue( + setInputValue( inputIndex: number, - keyIn: PsbtIn, - keyData: Uint8Array, + key: PsbtIn, value: Value, + keyData?: Uint8Array, ) { this.inputMaps[inputIndex]?.set( - new Key(keyIn, keyData).toHexaString(), + new Key(key, keyData).toHexaString(), value, ); } diff --git a/packages/signer/signer-btc/src/internal/psbt/service/value/DefaultValueParser.test.ts b/packages/signer/signer-btc/src/internal/psbt/service/value/DefaultValueParser.test.ts index 825ef0ada..5e716889e 100644 --- a/packages/signer/signer-btc/src/internal/psbt/service/value/DefaultValueParser.test.ts +++ b/packages/signer/signer-btc/src/internal/psbt/service/value/DefaultValueParser.test.ts @@ -44,6 +44,68 @@ describe("DefaultValueParser", () => { }); }); + describe("getUInt32LE", () => { + it("Get an unsigned 32-bit positive integer", () => { + // GIVEN + const data = Uint8Array.from([214, 255, 255, 255]); + + // WHEN + const result = service.getUInt32LE(data); + + // THEN + expect(result.isJust()).toStrictEqual(true); + expect(result.unsafeCoerce()).toStrictEqual(4294967254); + }); + + it("Invalid data", () => { + // GIVEN + const data = Uint8Array.from([0, 0, 0]); + + // WHEN + const result = service.getUInt32LE(data); + + // THEN + expect(result.isJust()).toStrictEqual(false); + }); + }); + + describe("getInt64LE", () => { + it("Get a signed 64-bit positive integer", () => { + // GIVEN + const data = Uint8Array.from([42, 0, 0, 0, 0, 0, 0, 0]); + + // WHEN + const result = service.getInt64LE(data); + + // THEN + expect(result.isJust()).toStrictEqual(true); + expect(result.unsafeCoerce()).toStrictEqual(42n); + }); + + it("Get a signed 64-bit negative integer", () => { + // GIVEN + const data = Uint8Array.from([214, 255, 255, 255, 255, 255, 255, 255]); + + // WHEN + const result = service.getInt64LE(data); + + // THEN + expect(result.isJust()).toStrictEqual(true); + expect(result.unsafeCoerce()).toStrictEqual(-42n); + }); + + it("Invalid data", () => { + // GIVEN + const data = Uint8Array.from([0, 0, 0, 0, 0]); + + // WHEN + const result = service.getInt64LE(data); + + // THEN + expect(result.isJust()).toStrictEqual(false); + }); + }); + describe("getVarint", () => { it("Get a varint", () => { // GIVEN diff --git a/packages/signer/signer-btc/src/internal/psbt/service/value/DefaultValueParser.ts b/packages/signer/signer-btc/src/internal/psbt/service/value/DefaultValueParser.ts index a804c9d30..07c2a1da7 100644 --- a/packages/signer/signer-btc/src/internal/psbt/service/value/DefaultValueParser.ts +++ b/packages/signer/signer-btc/src/internal/psbt/service/value/DefaultValueParser.ts @@ -15,6 +15,10 @@ export class DefaultValueParser implements ValueParser { ); } + getInt64LE(data: Uint8Array): Maybe { + return Maybe.fromNullable(new ByteArrayParser(data).extract64BitInt(false)); + } + getVarint(data: Uint8Array): Maybe { return extractVarint(new ByteArrayParser(data)).map( (varint) => varint.value, diff --git a/packages/signer/signer-btc/src/internal/psbt/service/value/ValueParser.ts b/packages/signer/signer-btc/src/internal/psbt/service/value/ValueParser.ts index 6411b7cda..3fb9caf99 100644 --- a/packages/signer/signer-btc/src/internal/psbt/service/value/ValueParser.ts +++ b/packages/signer/signer-btc/src/internal/psbt/service/value/ValueParser.ts @@ -3,5 +3,6 @@ import { type Maybe } from "purify-ts"; export interface ValueParser { getInt32LE(data: Uint8Array): Maybe; getUInt32LE(data: Uint8Array): Maybe; + getInt64LE(data: Uint8Array): Maybe; getVarint(data: Uint8Array): Maybe; }