refactor: cleanup based on review feedback

src/crypto/secp256k1.ts

@@ -4,6 +4,7 @@ import * as secp from '@noble/secp256k1';
 import { Address } from 'micro-eth-signer';
 import { concatBytes, hexToBuffer } from '../utils/buffer';
 
+/** Number of bytes per signature */
 export const SIGNATURE_LENGTH = 65;
 
 export function randomPrivateKey() {

src/vms/pvm/etna-builder/spend-reducers/handleFeeAndChange.test.ts

@@ -7,6 +7,12 @@ import {
   getInitialReducerState,
   getSpendHelper,
 } from './fixtures/reducers';
+import {
+  BigIntPr,
+  Id,
+  TransferOutput,
+  TransferableOutput,
+} from '../../../../serializable';
 
 describe('handleFeeAndChange', () => {
   test('throws an error if excessAVAX is less than the required fee', () => {
@@ -25,15 +31,11 @@ describe('handleFeeAndChange', () => {
     const state = getInitialReducerState({ excessAVAX: 4n });
     const spendHelper = getSpendHelper();
     const addChangeOutputSpy = jest.spyOn(spendHelper, 'addChangeOutput');
-    const calculateFeeWithTemporaryOutputComplexitySpy = jest.spyOn(
-      spendHelper,
-      'calculateFeeWithTemporaryOutputComplexity',
-    );
+    const calculateFeeSpy = jest.spyOn(spendHelper, 'calculateFee');
 
-    expect(handleFeeAndChange(state, getSpendHelper(), testContext)).toEqual(
-      state,
-    );
-    expect(calculateFeeWithTemporaryOutputComplexitySpy).not.toHaveBeenCalled();
+    expect(handleFeeAndChange(state, spendHelper, testContext)).toEqual(state);
+    expect(calculateFeeSpy).toHaveBeenCalledTimes(1);
+    expect(calculateFeeSpy).toHaveBeenCalledWith();
     expect(addChangeOutputSpy).not.toHaveBeenCalled();
   });
 
@@ -45,17 +47,18 @@ describe('handleFeeAndChange', () => {
     const spendHelper = getSpendHelper();
 
     const addChangeOutputSpy = jest.spyOn(spendHelper, 'addChangeOutput');
-    const calculateFeeWithTemporaryOutputComplexitySpy = jest.spyOn(
-      spendHelper,
-      'calculateFeeWithTemporaryOutputComplexity',
-    );
+    const calculateFeeSpy = jest.spyOn(spendHelper, 'calculateFee');
 
     expect(handleFeeAndChange(state, spendHelper, testContext)).toEqual({
       ...state,
       excessAVAX,
     });
-    expect(calculateFeeWithTemporaryOutputComplexitySpy).toHaveBeenCalledTimes(
-      1,
+    expect(calculateFeeSpy).toHaveBeenCalledTimes(2);
+    expect(calculateFeeSpy).toHaveBeenCalledWith(
+      new TransferableOutput(
+        Id.fromString(testContext.avaxAssetID),
+        new TransferOutput(new BigIntPr(0n), CHANGE_OWNERS),
+      ),
     );
     expect(addChangeOutputSpy).toHaveBeenCalledTimes(1);
 

src/vms/pvm/etna-builder/spend-reducers/handleFeeAndChange.ts

@@ -72,17 +72,16 @@ export const handleFeeAndChange: SpendReducerFunction = (
     } else {
       // Calculate the fee with a temporary output complexity
       // as if we added the change output.
-      const requiredFeeWithChangeOutput =
-        spendHelper.calculateFeeWithTemporaryOutputComplexity(
-          new TransferableOutput(
-            Id.fromString(context.avaxAssetID),
-            new TransferOutput(new BigIntPr(0n), changeOwners),
-          ),
-        );
+      const requiredFeeWithChangeOutput = spendHelper.calculateFee(
+        new TransferableOutput(
+          Id.fromString(context.avaxAssetID),
+          new TransferOutput(new BigIntPr(0n), changeOwners),
+        ),
+      );
 
       // If the excess AVAX is greater than the new fee, add a change output.
       // Otherwise, ignore and burn the excess because it can't be returned
-      // (ie we can't pay the fee to return the excess).
+      // (ie there is no point in adding a change output if you can't afford to add it).
       if (state.excessAVAX > requiredFeeWithChangeOutput) {
         spendHelper.addChangeOutput(
           new TransferableOutput(

src/vms/pvm/etna-builder/spend-reducers/useUnlockedUTXOs.ts

@@ -63,19 +63,31 @@ export const useUnlockedUTXOs: SpendReducerFunction = (
   );
 
   // 3. Split verified usable UTXOs into AVAX assetId UTXOs and other assetId UTXOs.
-  const [otherVerifiedUsableUTXOs, avaxVerifiedUsableUTXOs] =
-    verifiedUsableUTXOs.reduce(
-      (result, { sigData, data: utxo }) => {
-        if (utxo.assetId.value() === context.avaxAssetID) {
-          return [result[0], [...result[1], { sigData, data: utxo }]];
+  const { otherVerifiedUsableUTXOs, avaxVerifiedUsableUTXOs } =
+    verifiedUsableUTXOs.reduce<{
+      avaxVerifiedUsableUTXOs: typeof verifiedUsableUTXOs;
+      otherVerifiedUsableUTXOs: typeof verifiedUsableUTXOs;
+    }>(
+      (result, verifiedUsableUTXO) => {
+        if (verifiedUsableUTXO.data.assetId.value() === context.avaxAssetID) {
+          return {
+            ...result,
+            avaxVerifiedUsableUTXOs: [
+              ...result.avaxVerifiedUsableUTXOs,
+              verifiedUsableUTXO,
+            ],
+          };
         }
 
-        return [[...result[0], { sigData, data: utxo }], result[1]];
+        return {
+          ...result,
+          otherVerifiedUsableUTXOs: [
+            ...result.otherVerifiedUsableUTXOs,
+            verifiedUsableUTXO,
+          ],
+        };
       },
-      [[], []] as [
-        other: typeof verifiedUsableUTXOs,
-        avax: typeof verifiedUsableUTXOs,
-      ],
+      { otherVerifiedUsableUTXOs: [], avaxVerifiedUsableUTXOs: [] },
     );
 
   // 4. Handle all the non-AVAX asset UTXOs first.

src/vms/pvm/etna-builder/spend.ts

@@ -58,7 +58,7 @@ export type SpendProps = Readonly<{
    */
   ownerOverride?: OutputOwners | null;
   /**
-   * Whether to consolidate outputs.
+   * Whether to consolidate change and stake outputs.
    *
    * @default false
    */
@@ -100,7 +100,7 @@ export type SpendProps = Readonly<{
  */
 export const spend = (
   {
-    excessAVAX: _excessAVAX = 0n,
+    excessAVAX = 0n,
     fromAddresses,
     initialComplexity,
     ownerOverride,
@@ -116,7 +116,6 @@ export const spend = (
   try {
     const changeOwners =
       ownerOverride || OutputOwners.fromNative(spendOptions.changeAddresses);
-    const excessAVAX: bigint = _excessAVAX;
 
     const spendHelper = new SpendHelper({
       changeOutputs: [],
@@ -143,9 +142,6 @@ export const spend = (
 
     const spendReducerFunctions: readonly SpendReducerFunction[] = [
       ...spendReducers,
-      // useSpendableLockedUTXOs,
-      // TODO: Should we just default include this? Used on every builder.
-      // useUnlockedUTXOs,
       verifyAssetsConsumed,
       handleFeeAndChange,
       // Consolidation and sorting happens in the SpendHelper.

src/vms/pvm/etna-builder/spendHelper.test.ts

@@ -61,18 +61,14 @@ describe('src/vms/pvm/etna-builder/spendHelper', () => {
       stakeOutputs: [],
     });
 
-    spendHelper.addOutputComplexity(transferableOutput());
-
-    expect(spendHelper.calculateFee()).toBe(339n);
-
     const inputUtxo = utxo();
     const inputTransferableInput = transferableInput();
 
     spendHelper.addInput(inputUtxo, inputTransferableInput);
 
     expect(spendHelper.getInputsOutputs()).toEqual({
       changeOutputs: [],
-      fee: 1251n,
+      fee: 942n,
       inputs: [inputTransferableInput],
       inputUTXOs: [inputUtxo],
       stakeOutputs: [],
@@ -84,7 +80,7 @@ describe('src/vms/pvm/etna-builder/spendHelper', () => {
 
     expect(spendHelper.getInputsOutputs()).toEqual({
       changeOutputs: [changeOutput],
-      fee: 1560n,
+      fee: 1251n,
       inputs: [inputTransferableInput],
       inputUTXOs: [inputUtxo],
       stakeOutputs: [],
@@ -96,7 +92,7 @@ describe('src/vms/pvm/etna-builder/spendHelper', () => {
 
     expect(spendHelper.getInputsOutputs()).toEqual({
       changeOutputs: [changeOutput],
-      fee: 1869n,
+      fee: 1560n,
       inputs: [inputTransferableInput],
       inputUTXOs: [inputUtxo],
       stakeOutputs: [stakeOutput],
@@ -397,9 +393,9 @@ describe('src/vms/pvm/etna-builder/spendHelper', () => {
 
     const temporaryOutput = transferableOutput();
 
-    expect(
-      spendHelper.calculateFeeWithTemporaryOutputComplexity(temporaryOutput),
-    ).toBeGreaterThan(originalFee);
+    expect(spendHelper.calculateFee(temporaryOutput)).toBeGreaterThan(
+      originalFee,
+    );
 
     expect(spendHelper.calculateFee()).toBe(originalFee);
   });

src/vms/pvm/etna-builder/spendHelper.ts

@@ -41,8 +41,6 @@ export class SpendHelper {
 
   private changeOutputs: readonly TransferableOutput[];
   private inputs: readonly TransferableInput[];
-  private inputComplexity: Dimensions = createEmptyDimensions();
-  private outputComplexity: Dimensions = createEmptyDimensions();
   private stakeOutputs: readonly TransferableOutput[];
 
   private inputUTXOs: readonly Utxo[] = [];
@@ -78,13 +76,6 @@ export class SpendHelper {
    * @returns {SpendHelper} The current instance of SpendHelper for chaining.
    */
   addInput(utxo: Utxo, transferableInput: TransferableInput): SpendHelper {
-    const newInputComplexity = getInputComplexity([transferableInput]);
-
-    this.inputComplexity = addDimensions(
-      this.inputComplexity,
-      newInputComplexity,
-    );
-
     this.inputs = [...this.inputs, transferableInput];
     this.inputUTXOs = [...this.inputUTXOs, utxo];
 
@@ -118,29 +109,19 @@ export class SpendHelper {
   }
 
   /**
-   * Adds a transferable output to the SpendHelper.
-   *
-   * @param {TransferableOutput} transferableOutput - The transferable output to be added.
-   * @returns {SpendHelper} The current instance of SpendHelper for chaining.
+   * When computing the complexity/fee of a transaction that needs change but doesn't yet have
+   * a corresponding change output, `additionalComplexity` may be used to calculate the complexity
+   * and therefore the fee as if the change output was already added.
    */
-  addOutputComplexity(transferableOutput: TransferableOutput): SpendHelper {
-    const newOutputComplexity = getOutputComplexity([transferableOutput]);
-
-    this.outputComplexity = addDimensions(
-      this.outputComplexity,
-      newOutputComplexity,
-    );
-
-    return this;
-  }
-
-  private getComplexity(): Dimensions {
+  private getComplexity(
+    additionalComplexity: Dimensions = createEmptyDimensions(),
+  ): Dimensions {
     return addDimensions(
       this.initialComplexity,
       getInputComplexity(this.inputs),
       getOutputComplexity(this.changeOutputs),
       getOutputComplexity(this.stakeOutputs),
-      this.outputComplexity,
+      additionalComplexity,
     );
   }
 
@@ -231,30 +212,24 @@ export class SpendHelper {
 
   /**
    * Calculates the fee for the SpendHelper based on its complexity and gas price.
+   * Provide an empty change output as a parameter to calculate the fee as if the change output was already added.
    *
+   * @param {TransferableOutput} additionalOutput - The change output that has not yet been added to the SpendHelper.
    * @returns {bigint} The fee for the SpendHelper.
    */
-  calculateFee(): bigint {
+  calculateFee(additionalOutput?: TransferableOutput): bigint {
     this.consolidateOutputs();
 
-    const gas = dimensionsToGas(this.getComplexity(), this.weights);
+    const gas = dimensionsToGas(
+      this.getComplexity(
+        additionalOutput ? getOutputComplexity([additionalOutput]) : undefined,
+      ),
+      this.weights,
+    );
 
     return gas * this.gasPrice;
   }
 
-  calculateFeeWithTemporaryOutputComplexity(
-    transferableOutput: TransferableOutput,
-  ): bigint {
-    const oldOutputComplexity = this.outputComplexity;
-    this.addOutputComplexity(transferableOutput);
-
-    const fee = this.calculateFee();
-
-    this.outputComplexity = oldOutputComplexity;
-
-    return fee;
-  }
-
   /**
    * Determines if a change output with a matching asset ID and output owners exists.
    *