From 2569ad845570ba5ce89ac84393cf0b67fd830d1d Mon Sep 17 00:00:00 2001 From: Samuel Golland Date: Tue, 26 Nov 2024 11:10:26 -0500 Subject: [PATCH 1/3] fix: sort addresses --- src/utils/addressMap.test.ts | 120 ++++++++++++++++---------------- src/utils/addressesFromBytes.ts | 4 +- 2 files changed, 62 insertions(+), 62 deletions(-) diff --git a/src/utils/addressMap.test.ts b/src/utils/addressMap.test.ts index bb3734351..50ab067b6 100644 --- a/src/utils/addressMap.test.ts +++ b/src/utils/addressMap.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, test, vi } from 'vitest'; import { address } from '../fixtures/common'; import { Address } from '../serializable/fxs/common'; import { AddressMap, AddressMaps, matchOwners } from './addressMap'; @@ -245,67 +245,65 @@ describe('AddressMaps', () => { }); describe('matchOwners', () => { - it('matches owners', () => { - const owner1 = address(); - const owner2 = Address.fromHex('7db97c7cece249c2b98bdc0226cc4c2a57bf52fc'); - const ownerAddresses: Uint8Array[] = [owner1.toBytes(), owner2.toBytes()]; - const goodOwner = OutputOwners.fromNative(ownerAddresses, 0n, 1); - const threasholdTooHigh = OutputOwners.fromNative(ownerAddresses, 0n, 5); - const wrongOwner = OutputOwners.fromNative( - [hexToBuffer('0x12345123451234512345')], - 0n, - 5, - ); - const locked = OutputOwners.fromNative( - ownerAddresses, - 9999999999999999999999999999999999n, - 5, - ); + const owner1 = address(); + const owner2 = Address.fromHex('7db97c7cece249c2b98bdc0226cc4c2a57bf52fc'); + const ownerAddresses: Uint8Array[] = [owner1.toBytes(), owner2.toBytes()]; + const goodOwner = OutputOwners.fromNative(ownerAddresses, 0n, 1); + const threasholdTooHigh = OutputOwners.fromNative(ownerAddresses, 0n, 5); + const wrongOwner = OutputOwners.fromNative( + [hexToBuffer('0x12345123451234512345')], + 0n, + 5, + ); + const locked = OutputOwners.fromNative( + ownerAddresses, + 9999999999999999999999999999999999n, + 5, + ); - const specs = [ - { - testCase: goodOwner, - expectedSigIndices: [0], - expectedAddressMap: new AddressMap([[owner1, 0]]), - }, - { - testCase: threasholdTooHigh, - expectedSigIndices: undefined, - expectedAddressMap: undefined, - }, - { - testCase: locked, - expectedSigIndices: undefined, - expectedAddressMap: undefined, - }, - { - testCase: wrongOwner, - expectedSigIndices: undefined, - expectedAddressMap: undefined, - }, - { - testCase: goodOwner, - sigindices: [1], - expectedSigIndices: [1], - expectedAddressMap: new AddressMap([[owner2, 1]]), - }, - { - testCase: goodOwner, - sigindices: [2], - expectedSigIndices: undefined, - expectedAddressMap: undefined, - }, - ]; + const specs = [ + { + testCase: goodOwner, + expectedSigIndices: [0], + expectedAddressMap: new AddressMap([[owner2, 0]]), + }, + { + testCase: threasholdTooHigh, + expectedSigIndices: undefined, + expectedAddressMap: undefined, + }, + { + testCase: locked, + expectedSigIndices: undefined, + expectedAddressMap: undefined, + }, + { + testCase: wrongOwner, + expectedSigIndices: undefined, + expectedAddressMap: undefined, + }, + { + testCase: goodOwner, + sigindices: [1], + expectedSigIndices: [1], + expectedAddressMap: new AddressMap([[owner1, 1]]), + }, + { + testCase: goodOwner, + sigindices: [2], + expectedSigIndices: undefined, + expectedAddressMap: undefined, + }, + ]; - specs.forEach((spec) => { - const result = matchOwners( - spec.testCase, - addressesFromBytes(ownerAddresses), - 50n, - spec.sigindices, - ); - expect(result?.sigIndicies).toEqual(spec.expectedSigIndices); - expect(result?.addressMap).toEqual(spec.expectedAddressMap); - }); + test.each(specs)('matchOwners($testCase, $sigIndices)', (spec) => { + const result = matchOwners( + spec.testCase, + addressesFromBytes(ownerAddresses), + 50n, + spec.sigindices, + ); + expect(result?.sigIndicies).toEqual(spec.expectedSigIndices); + expect(result?.addressMap).toEqual(spec.expectedAddressMap); }); }); diff --git a/src/utils/addressesFromBytes.ts b/src/utils/addressesFromBytes.ts index cea2c5282..c15894084 100644 --- a/src/utils/addressesFromBytes.ts +++ b/src/utils/addressesFromBytes.ts @@ -1,5 +1,7 @@ import { Address } from '../serializable/fxs/common'; +import { bytesCompare } from './bytesCompare'; export function addressesFromBytes(bytes: readonly Uint8Array[]): Address[] { - return bytes.map((b) => new Address(b)); + const sortedBytes = bytes.toSorted(bytesCompare); + return sortedBytes.map((b) => new Address(b)); } From 9b0907e950f2e5af82150566d5f5c46bec6709fe Mon Sep 17 00:00:00 2001 From: Samuel Golland Date: Tue, 26 Nov 2024 13:28:54 -0500 Subject: [PATCH 2/3] chore: add multisig test case --- src/utils/addressMap.test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/utils/addressMap.test.ts b/src/utils/addressMap.test.ts index 50ab067b6..f764f2abe 100644 --- a/src/utils/addressMap.test.ts +++ b/src/utils/addressMap.test.ts @@ -248,7 +248,9 @@ describe('matchOwners', () => { const owner1 = address(); const owner2 = Address.fromHex('7db97c7cece249c2b98bdc0226cc4c2a57bf52fc'); const ownerAddresses: Uint8Array[] = [owner1.toBytes(), owner2.toBytes()]; + // NOTE: the ownerAddresses will be sorted in the OutputOwners -- owner2 is at index 0. const goodOwner = OutputOwners.fromNative(ownerAddresses, 0n, 1); + const goodOwnerMultisig = OutputOwners.fromNative(ownerAddresses, 0n, 2); const threasholdTooHigh = OutputOwners.fromNative(ownerAddresses, 0n, 5); const wrongOwner = OutputOwners.fromNative( [hexToBuffer('0x12345123451234512345')], @@ -288,6 +290,15 @@ describe('matchOwners', () => { expectedSigIndices: [1], expectedAddressMap: new AddressMap([[owner1, 1]]), }, + { + testCase: goodOwnerMultisig, + sigindices: [0, 1], + expectedSigIndices: [0, 1], + expectedAddressMap: new AddressMap([ + [owner2, 0], + [owner1, 1], + ]), + }, { testCase: goodOwner, sigindices: [2], From dd5cd7efa3727c07c01f91aa652706158d4b32ec Mon Sep 17 00:00:00 2001 From: Eric Taylor Date: Wed, 27 Nov 2024 14:54:36 -0700 Subject: [PATCH 3/3] fix: filter utxos in etna import tx builder (#930) * fix: filter utxos in etna import tx builder * fix: correct unsigned tx utxos on etna import tx builder * fix: use utxos that were imported --- src/vms/pvm/etna-builder/builder.test.ts | 7 ++ src/vms/pvm/etna-builder/builder.ts | 99 +++++++++++++----------- 2 files changed, 59 insertions(+), 47 deletions(-) diff --git a/src/vms/pvm/etna-builder/builder.test.ts b/src/vms/pvm/etna-builder/builder.test.ts index 1a0b71246..df0ee6d8c 100644 --- a/src/vms/pvm/etna-builder/builder.test.ts +++ b/src/vms/pvm/etna-builder/builder.test.ts @@ -308,6 +308,13 @@ describe('./src/vms/pvm/etna-builder/builder.test.ts', () => { ); expectTxs(unsignedTx.getTx(), expectedTx); + + // Ensure that the unsigned tx utxos are the filtered utxos, + // and not the inputUtxos registered in the spend helper. + // This is only relevant for the ImportTx. + expect(unsignedTx.utxos).toHaveLength(1); + expect(unsignedTx.utxos).not.toContain(utxos[0]); + expect(unsignedTx.utxos).not.toContain(utxos[1]); }); test('newExportTx', () => { diff --git a/src/vms/pvm/etna-builder/builder.ts b/src/vms/pvm/etna-builder/builder.ts index 8221c2cbf..2e26d9170 100644 --- a/src/vms/pvm/etna-builder/builder.ts +++ b/src/vms/pvm/etna-builder/builder.ts @@ -302,50 +302,52 @@ export const newImportTx: TxBuilderFn = ( ) => { const fromAddresses = addressesFromBytes(fromAddressesBytes); - const { importedInputs, importedAmounts } = utxos - .filter( - (utxo): utxo is Utxo => - isTransferOut(utxo.output) && - // Currently - only AVAX is allowed to be imported to the P-Chain - utxo.assetId.toString() === context.avaxAssetID, - ) - .reduce<{ - importedInputs: TransferableInput[]; - importedAmounts: Record; - }>( - (acc, utxo) => { - const { sigIndicies: inputSigIndices } = - matchOwners(utxo.getOutputOwners(), fromAddresses, minIssuanceTime) || - {}; - - if (inputSigIndices === undefined) { - // We couldn't spend this UTXO, so we skip to the next one. - return acc; - } - - const assetId = utxo.getAssetId(); - - return { - importedInputs: [ - ...acc.importedInputs, - new TransferableInput( - utxo.utxoId, - utxo.assetId, - new TransferInput( - utxo.output.amt, - new Input(inputSigIndices.map((value) => new Int(value))), - ), + const filteredUtxos = utxos.filter( + (utxo): utxo is Utxo => + isTransferOut(utxo.output) && + // Currently - only AVAX is allowed to be imported to the P-Chain + utxo.assetId.toString() === context.avaxAssetID, + ); + + const { importedInputs, importedAmounts, inputUtxos } = filteredUtxos.reduce<{ + importedInputs: TransferableInput[]; + importedAmounts: Record; + inputUtxos: Utxo[]; + }>( + (acc, utxo) => { + const { sigIndicies: inputSigIndices } = + matchOwners(utxo.getOutputOwners(), fromAddresses, minIssuanceTime) || + {}; + + if (inputSigIndices === undefined) { + // We couldn't spend this UTXO, so we skip to the next one. + return acc; + } + + const assetId = utxo.getAssetId(); + + return { + importedInputs: [ + ...acc.importedInputs, + new TransferableInput( + utxo.utxoId, + utxo.assetId, + new TransferInput( + utxo.output.amt, + new Input(inputSigIndices.map((value) => new Int(value))), ), - ], - importedAmounts: { - ...acc.importedAmounts, - [assetId]: - (acc.importedAmounts[assetId] ?? 0n) + utxo.output.amount(), - }, - }; - }, - { importedInputs: [], importedAmounts: {} }, - ); + ), + ], + importedAmounts: { + ...acc.importedAmounts, + [assetId]: + (acc.importedAmounts[assetId] ?? 0n) + utxo.output.amount(), + }, + inputUtxos: [...acc.inputUtxos, utxo], + }; + }, + { importedInputs: [], importedAmounts: {}, inputUtxos: [] }, + ); if (importedInputs.length === 0) { throw new Error('no UTXOs available to import'); @@ -355,7 +357,7 @@ export const newImportTx: TxBuilderFn = ( const addressMaps = AddressMaps.fromTransferableInputs( importedInputs, - utxos, + filteredUtxos, minIssuanceTime, fromAddressesBytes, ); @@ -391,13 +393,16 @@ export const newImportTx: TxBuilderFn = ( fromAddresses, initialComplexity: complexity, minIssuanceTime, - utxos, + utxos: filteredUtxos, }, [useUnlockedUTXOs], context, ); - const { changeOutputs, inputs, inputUTXOs } = spendResults; + // Note: We don't use the `inputUTXOs` from `spendResults` + // for the `UnsignedTx` because we want to use the original + // UTXOs that were imported. + const { changeOutputs, inputs } = spendResults; return new UnsignedTx( new ImportTx( @@ -411,7 +416,7 @@ export const newImportTx: TxBuilderFn = ( Id.fromString(sourceChainId), importedInputs.sort(TransferableInput.compare), ), - inputUTXOs, + inputUtxos, addressMaps, ); };