Skip to content

Commit 09fae9c

Browse files
OttoAllmendingerOttoAllmendinger
authored
Merge pull request #7977 from BitGo/BTC-2962.fix-external-change-addr-expected
feat(abstract-utxo): enhance output handling and fix address comparison
2 parents b99cd65 + d79d95a commit 09fae9c

6 files changed

Lines changed: 145 additions & 42 deletions

File tree

‎modules/abstract-utxo/src/abstractUtxoCoin.ts‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,11 @@ export abstract class AbstractUtxoCoin
479479
}
480480
}
481481

482-
preprocessBuildParams(params: Record<string, any>): Record<string, any> {
482+
/**
483+
* This is called before crafting the HTTP request to the BitGo API.
484+
* It converts the recipient address `scriptPubKey:...` to { script: string } | { address: string }.
485+
*/
486+
override preprocessBuildParams(params: Record<string, any>): Record<string, any> {
483487
if (params.recipients !== undefined) {
484488
params.recipients =
485489
params.recipients instanceof Array
@@ -588,19 +592,6 @@ export abstract class AbstractUtxoCoin
588592
return this.decodeTransaction(string, decodeWith);
589593
}
590594

591-
toCanonicalTransactionRecipient(output: { valueString: string; address?: string }): {
592-
amount: bigint;
593-
address: string;
594-
} {
595-
const amount = BigInt(output.valueString);
596-
assertValidTransactionRecipient({ amount, address: output.address });
597-
assert(output.address, 'address is required');
598-
if (isScriptRecipient(output.address)) {
599-
return { amount, address: output.address };
600-
}
601-
return { amount, address: this.canonicalAddress(output.address) };
602-
}
603-
604595
/**
605596
* Extract and fill transaction details such as internal/change spend, external spend (explicit vs. implicit), etc.
606597
* @param params

‎modules/abstract-utxo/src/transaction/fixedScript/parseOutput.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414

1515
import { AbstractUtxoCoin } from '../../abstractUtxoCoin';
1616
import { Output, FixedScriptWalletOutput } from '../types';
17+
import { fromExtendedAddressFormatToScript } from '../recipient';
1718

1819
const debug = debugLib('bitgo:v2:parseoutput');
1920

@@ -284,7 +285,9 @@ export async function parseOutput({
284285
*/
285286
if (txParams.recipients !== undefined && txParams.recipients.length > RECIPIENT_THRESHOLD) {
286287
const isCurrentAddressInRecipients = txParams.recipients.some((recipient) =>
287-
recipient.address.includes(currentAddress)
288+
fromExtendedAddressFormatToScript(recipient.address, coin.name).equals(
289+
fromExtendedAddressFormatToScript(currentAddress, coin.name)
290+
)
288291
);
289292

290293
if (isCurrentAddressInRecipients) {

‎modules/abstract-utxo/src/transaction/fixedScript/parseTransaction.ts‎

Lines changed: 53 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,38 @@ import * as utxolib from '@bitgo/utxo-lib';
77
import type { AbstractUtxoCoin, ParseTransactionOptions } from '../../abstractUtxoCoin';
88
import type { FixedScriptWalletOutput, Output, ParsedTransaction } from '../types';
99
import { fetchKeychains, getKeySignatures, toKeychainTriple, UtxoKeychain, UtxoNamedKeychains } from '../../keychains';
10-
import { ComparableOutput, outputDifference } from '../outputDifference';
11-
import { fromExtendedAddressFormatToScript, toExtendedAddressFormat } from '../recipient';
10+
import {
11+
assertValidTransactionRecipient,
12+
fromExtendedAddressFormatToScript,
13+
isScriptRecipient,
14+
toExtendedAddressFormat,
15+
toOutputScript,
16+
} from '../recipient';
17+
import { ComparableOutput, ExpectedOutput, outputDifference } from '../outputDifference';
1218

1319
import type { TransactionExplanation } from './explainTransaction';
1420
import { CustomChangeOptions, parseOutput } from './parseOutput';
1521

16-
export type ComparableOutputWithExternal<TValue> = ComparableOutput<TValue> & {
22+
export type ComparableOutputWithExternal<TValue> = (ComparableOutput<TValue> | ExpectedOutput) & {
1723
external: boolean | undefined;
1824
};
1925

26+
function toCanonicalTransactionRecipient(
27+
coin: AbstractUtxoCoin,
28+
output: { valueString: string; address?: string }
29+
): {
30+
amount: bigint;
31+
address: string;
32+
} {
33+
const amount = BigInt(output.valueString);
34+
assertValidTransactionRecipient({ amount, address: output.address });
35+
assert(output.address, 'address is required');
36+
if (isScriptRecipient(output.address)) {
37+
return { amount, address: output.address };
38+
}
39+
return { amount, address: coin.canonicalAddress(output.address) };
40+
}
41+
2042
async function parseRbfTransaction<TNumber extends bigint | number>(
2143
coin: AbstractUtxoCoin,
2244
params: ParseTransactionOptions<TNumber>
@@ -33,7 +55,7 @@ async function parseRbfTransaction<TNumber extends bigint | number>(
3355
if (output.wallet === wallet.id()) {
3456
return [];
3557
}
36-
return [coin.toCanonicalTransactionRecipient(output)];
58+
return [toCanonicalTransactionRecipient(coin, output)];
3759
}
3860
);
3961

@@ -55,21 +77,37 @@ function toExpectedOutputs(
5577
allowExternalChangeAddress?: boolean;
5678
changeAddress?: string;
5779
}
58-
): Output[] {
80+
): ExpectedOutput[] {
5981
// verify that each recipient from txParams has their own output
60-
const expectedOutputs = (txParams.recipients ?? []).flatMap((output) => {
82+
const expectedOutputs: ExpectedOutput[] = (txParams.recipients ?? []).flatMap((output) => {
6183
if (output.address === undefined) {
84+
assert('script' in output, 'script is required for non-encodeable scriptPubkeys');
6285
if (output.amount.toString() !== '0') {
6386
throw new Error(`Only zero amounts allowed for non-encodeable scriptPubkeys: ${output}`);
6487
}
65-
return [output];
88+
return [
89+
{
90+
script: toOutputScript(output, coin.name),
91+
value: output.amount === 'max' ? 'max' : BigInt(output.amount),
92+
},
93+
];
6694
}
67-
return [{ ...output, address: coin.canonicalAddress(output.address) }];
95+
return [
96+
{
97+
script: fromExtendedAddressFormatToScript(output.address, coin.name),
98+
value: output.amount === 'max' ? 'max' : BigInt(output.amount),
99+
},
100+
];
68101
});
69102
if (txParams.allowExternalChangeAddress && txParams.changeAddress) {
70-
// when an external change address is explicitly specified, count all outputs going towards that
71-
// address in the expected outputs (regardless of the output amount)
72-
expectedOutputs.push({ address: coin.canonicalAddress(txParams.changeAddress), amount: 'max' });
103+
expectedOutputs.push({
104+
script: toOutputScript(txParams.changeAddress, coin.name),
105+
// When an external change address is explicitly specified, count all outputs going towards that
106+
// address in the expected outputs (regardless of the output amount)
107+
value: 'max',
108+
// Note that the change output is not required to exist, so we mark it as optional.
109+
optional: true,
110+
});
73111
}
74112
return expectedOutputs;
75113
}
@@ -162,7 +200,7 @@ export async function parseTransaction<TNumber extends bigint | number>(
162200
keychainArray: toKeychainTriple(keychains),
163201
wallet,
164202
txParams: {
165-
recipients: expectedOutputs,
203+
recipients: txParams.recipients ?? [],
166204
changeAddress: txParams.changeAddress,
167205
},
168206
customChange,
@@ -185,15 +223,9 @@ export async function parseTransaction<TNumber extends bigint | number>(
185223
}));
186224
}
187225

188-
const missingOutputs = outputDifference(
189-
toComparableOutputsWithExternal(expectedOutputs),
190-
toComparableOutputsWithExternal(allOutputs)
191-
);
226+
const missingOutputs = outputDifference(expectedOutputs, toComparableOutputsWithExternal(allOutputs));
192227

193-
const implicitOutputs = outputDifference(
194-
toComparableOutputsWithExternal(allOutputDetails),
195-
toComparableOutputsWithExternal(expectedOutputs)
196-
);
228+
const implicitOutputs = outputDifference(toComparableOutputsWithExternal(allOutputDetails), expectedOutputs);
197229
const explicitOutputs = outputDifference(toComparableOutputsWithExternal(allOutputDetails), implicitOutputs);
198230

199231
// these are all the non-wallet outputs that had been originally explicitly specified in recipients
@@ -223,7 +255,7 @@ export async function parseTransaction<TNumber extends bigint | number>(
223255
coin.amountType
224256
);
225257

226-
function toOutputs(outputs: ComparableOutputWithExternal<bigint | 'max'>[]): Output[] {
258+
function toOutputs(outputs: ExpectedOutput[] | ComparableOutputWithExternal<bigint | 'max'>[]): Output[] {
227259
return outputs.map((output) => ({
228260
address: toExtendedAddressFormat(output.script, coin.name),
229261
amount: output.value.toString(),

‎modules/abstract-utxo/src/transaction/outputDifference.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,10 @@ export type ComparableOutput<TValue> = {
77
export type ActualOutput = ComparableOutput<bigint>;
88

99
/** Expected outputs can have a fixed value or 'max'. */
10-
export type ExpectedOutput = ComparableOutput<bigint | 'max'>;
10+
export type ExpectedOutput = ComparableOutput<bigint | 'max'> & {
11+
/** When true, the output is not required to be present in the transaction. */
12+
optional?: boolean;
13+
};
1114

1215
/**
1316
* @param a
@@ -40,6 +43,13 @@ export function outputDifference<A extends ActualOutput | ExpectedOutput, B exte
4043
return first;
4144
}
4245

46+
export function getMissingOutputs<A extends ActualOutput, B extends ExpectedOutput>(
47+
actualOutputs: A[],
48+
expectedOutputs: B[]
49+
): B[] {
50+
return outputDifference(expectedOutputs, actualOutputs).filter((o) => !o.optional);
51+
}
52+
4353
export type OutputDifferenceWithExpected<TActual extends ActualOutput, TExpected extends ExpectedOutput> = {
4454
/** These are the external outputs that were expected and found in the transaction. */
4555
explicitOutputs: TActual[];
@@ -65,7 +75,7 @@ export function outputDifferencesWithExpected<TActual extends ActualOutput, TExp
6575
): OutputDifferenceWithExpected<TActual, TExpected> {
6676
const implicitOutputs = outputDifference(actualOutputs, expectedOutputs);
6777
const explicitOutputs = outputDifference(actualOutputs, implicitOutputs);
68-
const missingOutputs = outputDifference(expectedOutputs, actualOutputs);
78+
const missingOutputs = getMissingOutputs(actualOutputs, expectedOutputs);
6979
return {
7080
explicitOutputs,
7181
implicitOutputs,

‎modules/abstract-utxo/src/transaction/recipient.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,19 @@ export function fromExtendedAddressFormatToScript(extendedAddress: string, coinN
3333
return utxolib.addressFormat.toOutputScriptTryFormats(result.address, network);
3434
}
3535

36+
export function toOutputScript(v: string | { address: string } | { script: string }, coinName: UtxoCoinName): Buffer {
37+
if (typeof v === 'string') {
38+
return fromExtendedAddressFormatToScript(v, coinName);
39+
}
40+
if ('script' in v) {
41+
return Buffer.from(v.script, 'hex');
42+
}
43+
if ('address' in v) {
44+
return fromExtendedAddressFormatToScript(v.address, coinName);
45+
}
46+
throw new Error('invalid input');
47+
}
48+
3649
/**
3750
* Convert a script or address to the extended address format.
3851
* @param script

modules/abstract-utxo/test/unit/transaction/descriptor/outputDifference.ts renamed to modules/abstract-utxo/test/unit/transaction/outputDifference.ts

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,25 +3,31 @@ import assert from 'assert';
33
import {
44
ActualOutput,
55
ExpectedOutput,
6+
getMissingOutputs,
67
matchingOutput,
78
outputDifference,
89
outputDifferencesWithExpected,
9-
} from '../../../../src/transaction/outputDifference';
10+
} from '../../../src/transaction/outputDifference';
1011

1112
describe('outputDifference', function () {
12-
function output(script: string, value: bigint | number): ActualOutput;
13-
function output(script: string, value: 'max'): ExpectedOutput;
14-
function output(script: string, value: bigint | number | 'max'): ActualOutput | ExpectedOutput {
13+
function output(script: string, value: bigint | number, optional?: boolean): ActualOutput;
14+
function output(script: string, value: 'max', optional?: boolean): ExpectedOutput;
15+
function output(script: string, value: bigint | number | 'max', optional?: boolean): ActualOutput | ExpectedOutput {
1516
const scriptBuffer = Buffer.from(script, 'hex');
1617
if (scriptBuffer.toString('hex') !== script) {
1718
throw new Error('invalid script');
1819
}
1920
return {
2021
script: Buffer.from(script, 'hex'),
2122
value: value === 'max' ? 'max' : BigInt(value),
23+
...(optional !== undefined ? { optional } : {}),
2224
};
2325
}
2426

27+
function expectedOutput(script: string, value: bigint | number | 'max', optional?: boolean): ExpectedOutput {
28+
return output(script, value as 'max', optional) as ExpectedOutput;
29+
}
30+
2531
const a = output('aa', 1);
2632
const a2 = output('aa', 2);
2733
const aMax = output('aa', 'max');
@@ -64,6 +70,32 @@ describe('outputDifference', function () {
6470
});
6571
});
6672

73+
describe('getMissingOutputs', function () {
74+
it('returns missing non-optional outputs', function () {
75+
const aOptional = expectedOutput('aa', 1, true);
76+
const bOptional = expectedOutput('bb', 1, true);
77+
78+
// No expected outputs means no missing outputs
79+
assert.deepStrictEqual(getMissingOutputs([a], []), []);
80+
81+
// Missing required output is returned
82+
assert.deepStrictEqual(getMissingOutputs([], [a]), [a]);
83+
assert.deepStrictEqual(getMissingOutputs([b], [a]), [a]);
84+
85+
// Missing optional output is filtered out
86+
assert.deepStrictEqual(getMissingOutputs([], [aOptional]), []);
87+
assert.deepStrictEqual(getMissingOutputs([b], [aOptional]), []);
88+
89+
// Mix of optional and required: only required missing outputs returned
90+
assert.deepStrictEqual(getMissingOutputs([], [a, bOptional]), [a]);
91+
assert.deepStrictEqual(getMissingOutputs([], [aOptional, b]), [b]);
92+
93+
// Present outputs are not returned regardless of optional flag
94+
assert.deepStrictEqual(getMissingOutputs([a], [a]), []);
95+
assert.deepStrictEqual(getMissingOutputs([a], [aOptional]), []);
96+
});
97+
});
98+
6799
describe('outputDifferencesWithExpected', function () {
68100
function test(
69101
outputs: ActualOutput[],
@@ -91,5 +123,27 @@ describe('outputDifference', function () {
91123
test([a, a], [a], { missing: [], explicit: [a], implicit: [a] });
92124
test([a, b], [a], { missing: [], explicit: [a], implicit: [b] });
93125
});
126+
127+
it('handles optional expected outputs', function () {
128+
const aOptional = expectedOutput('aa', 1, true);
129+
const bOptional = expectedOutput('bb', 1, true);
130+
131+
// Missing optional output is not reported as missing
132+
test([], [aOptional], { missing: [], explicit: [], implicit: [] });
133+
test([b], [aOptional], { missing: [], explicit: [], implicit: [b] });
134+
135+
// Present optional output is still explicit
136+
test([a], [aOptional], { missing: [], explicit: [a], implicit: [] });
137+
138+
// Mix of required and optional outputs
139+
test([], [a, bOptional], { missing: [a], explicit: [], implicit: [] });
140+
test([a], [a, bOptional], { missing: [], explicit: [a], implicit: [] });
141+
test([a, b], [a, bOptional], { missing: [], explicit: [a, b], implicit: [] });
142+
143+
// Multiple optional outputs
144+
test([], [aOptional, bOptional], { missing: [], explicit: [], implicit: [] });
145+
test([a], [aOptional, bOptional], { missing: [], explicit: [a], implicit: [] });
146+
test([a, b], [aOptional, bOptional], { missing: [], explicit: [a, b], implicit: [] });
147+
});
94148
});
95149
});

0 commit comments

Comments
 (0)