fix(core): respect Signer.prepareTransaction return value in Transaction.completeFee

.changeset/olive-wings-fix.md

@@ -0,0 +1,7 @@
+---
+"@ckb-ccc/core": patch
+---
+
+fix(core): respect `Signer.prepareTransaction` return value in `Transaction.completeFee`
+
+Fix underestimated fees when `Signer.prepareTransaction` returns a new transaction, including when the transaction and signer come from different `@ckb-ccc/core` instances.

packages/core/src/ckb/transaction.test.ts

@@ -534,6 +534,87 @@ describe("Transaction", () => {
       expect(tx.outputs[1].capacity).toBeGreaterThan(minChangeCapacity);
     });
 
+    it("keeps preparations made in place on the caller's transaction", async () => {
+      const cellDep = {
+        outPoint: { txHash: `0x${"2".repeat(64)}`, index: 0 },
+        depType: "code" as const,
+      };
+      const dummyLock = `0x${"00".repeat(65)}`;
+      vi.mocked(signer.prepareTransaction).mockImplementation(
+        async (txLike) => {
+          const prepared = ccc.Transaction.from(txLike);
+          prepared.addCellDeps(cellDep);
+          prepared.setWitnessArgs(0, { lock: dummyLock });
+          return prepared;
+        },
+      );
+      const tx = ccc.Transaction.from({
+        inputs: [{ previousOutput: mockCapacityCells[0].outPoint }],
+        outputs: [{ capacity: ccc.fixedPointFrom(30), lock }],
+      });
+
+      await tx.completeFeeBy(signer, 1000n);
+
+      expect(tx.cellDeps).toHaveLength(1);
+      expect(tx.getWitnessArgs(0)?.lock).toBe(dummyLock);
+      expect(signer.prepareTransaction).toHaveBeenCalled();
+    });
+
+    it("copies a newly returned prepared transaction back and charges for its final size", async () => {
+      const cellDep = {
+        outPoint: { txHash: `0x${"3".repeat(64)}`, index: 0 },
+        depType: "code" as const,
+      };
+      const dummyLock = `0x${"00".repeat(572)}`;
+      const signedLock = `0x${"01".repeat(572)}`;
+      const returnedTransactions: ccc.Transaction[] = [];
+      vi.mocked(signer.prepareTransaction).mockImplementation(
+        async (txLike) => {
+          const original = ccc.Transaction.from(txLike);
+          // A plain structural value models a Transaction supplied across a
+          // package instance/runtime realm, where instanceof Transaction is false.
+          const foreignRealmTxLike: ccc.TransactionLike = {
+            ...original.clone(),
+          };
+          expect(foreignRealmTxLike).not.toBeInstanceOf(ccc.Transaction);
+          const prepared = ccc.Transaction.from(foreignRealmTxLike);
+          expect(prepared).not.toBe(original);
+          prepared.addCellDeps(cellDep);
+          prepared.setWitnessArgs(0, { lock: dummyLock });
+          if (returnedTransactions.length === 0) {
+            expect(original.cellDeps).toHaveLength(0);
+            expect(original.getWitnessArgs(0)?.lock).toBeUndefined();
+            expect(prepared.inputs[0].cellOutput).toBeDefined();
+            expect(prepared.inputs[0].outputData).toBeDefined();
+          }
+          returnedTransactions.push(prepared);
+          return prepared;
+        },
+      );
+      const tx = ccc.Transaction.from({
+        inputs: [{ previousOutput: mockCapacityCells[0].outPoint }],
+        outputs: [{ capacity: ccc.fixedPointFrom(30), lock }],
+      });
+
+      await tx.completeFeeBy(signer, 1000n);
+
+      expect(returnedTransactions.length).toBeGreaterThan(0);
+      expect(tx.cellDeps).toHaveLength(1);
+      expect(tx.getWitnessArgs(0)?.lock).toBe(dummyLock);
+      const preparedSize = tx.toBytes().length;
+
+      vi.spyOn(signer, "signOnlyTransaction").mockImplementation(
+        async (txLike) => {
+          const signed = ccc.Transaction.from(txLike);
+          signed.setWitnessArgs(0, { lock: signedLock });
+          return signed;
+        },
+      );
+      const signed = await signer.signTransaction(tx);
+      expect(signed.toBytes()).toHaveLength(preparedSize);
+      expect(await signed.getFeeRate(client)).toBeGreaterThanOrEqual(1000n);
+    });
+
     it("should add inputs when insufficient capacity for fee", async () => {
       const tx = ccc.Transaction.from({
         outputs: [
@@ -643,6 +724,28 @@ describe("Transaction", () => {
       ).rejects.toThrow("Insufficient CKB");
     });
 
+    it("should complete a funded prepared transaction without adding inputs", async () => {
+      const tx = ccc.Transaction.from({
+        inputs: [
+          { previousOutput: mockCapacityCells[0].outPoint },
+          { previousOutput: mockCapacityCells[1].outPoint },
+        ],
+        outputs: [{ capacity: ccc.fixedPointFrom(30), lock }],
+      });
+
+      const [addedInputs, hasChange] = await tx.completeFeeBy(
+        signer,
+        1000n,
+        undefined,
+        { shouldAddInputs: false },
+      );
+
+      expect(addedInputs).toBe(0);
+      expect(hasChange).toBe(true);
+      expect(tx.inputs).toHaveLength(2);
+      expect(await tx.getFeeRate(client)).toBeGreaterThanOrEqual(1000n);
+    });
+
     it("should handle filter parameter for input selection", async () => {
       const customFilter = {
         scriptLenRange: [0, 1] as [number, number],

packages/core/src/ckb/transaction.ts

@@ -1193,6 +1193,10 @@ export class Transaction extends Entity.Base<TransactionLike, Transaction>() {
    * ```
    */
   copy(txLike: TransactionLike) {
+    if (txLike === this) {
+      return;
+    }
+
     const tx = Transaction.from(txLike);
     this.version = tx.version;
     this.cellDeps = tx.cellDeps;
@@ -2422,6 +2426,8 @@ export class Transaction extends Entity.Base<TransactionLike, Transaction>() {
     let leastFee = Zero;
     let leastExtraCapacity = Zero;
     let collected = 0;
+    // eslint-disable-next-line @typescript-eslint/no-this-alias
+    let tx: Transaction = this;
 
     // ===
     // Usually, for the worst situation, three iterations are needed
@@ -2436,7 +2442,7 @@ export class Transaction extends Entity.Base<TransactionLike, Transaction>() {
         }
 
         try {
-          return await this.completeInputsByCapacity(
+          return await tx.completeInputsByCapacity(
             from,
             leastFee + leastExtraCapacity,
             filter,
@@ -2455,52 +2461,54 @@ export class Transaction extends Entity.Base<TransactionLike, Transaction>() {
         }
       })();
 
-      const fee = await this.getFee(from.client);
+      tx = await from.prepareTransaction(tx);
+      const fee = await tx.getFee(from.client);
       if (fee < leastFee + leastExtraCapacity) {
         // Not enough capacity are collected, it should only happens when shouldAddInputs is false
         throw new ErrorTransactionInsufficientCapacity(
           leastFee + leastExtraCapacity - fee,
           { isForChange: leastExtraCapacity !== Zero },
         );
       }
-
-      await from.prepareTransaction(this);
       if (leastFee === Zero) {
         // The initial fee is calculated based on prepared transaction
         // This should only happens during the first iteration
-        leastFee = this.estimateFee(feeRate);
+        leastFee = tx.estimateFee(feeRate);
       }
       // The extra capacity paid the fee without a change
       // leastExtraCapacity should be 0 here, otherwise we should failed in the previous check
       // So this only happens in the first iteration
       if (fee === leastFee) {
+        this.copy(tx);
         return [collected, false];
       }
 
       // Invoke the change function on a transaction multiple times may cause problems, so we clone it
-      const tx = this.clone();
-      const needed = numFrom(await Promise.resolve(change(tx, fee - leastFee)));
+      let changedTx = tx.clone();
+      const needed = numFrom(
+        await Promise.resolve(change(changedTx, fee - leastFee)),
+      );
       if (needed > Zero) {
         // No enough extra capacity to create new cells for change, collect inputs again
         leastExtraCapacity = needed;
         continue;
       }
 
-      if ((await tx.getFee(from.client)) !== leastFee) {
+      if ((await changedTx.getFee(from.client)) !== leastFee) {
         throw new Error(
           "The change function doesn't use all available capacity",
         );
       }
 
       // New change cells created, update the fee
-      await from.prepareTransaction(tx);
-      const changedFee = tx.estimateFee(feeRate);
+      changedTx = await from.prepareTransaction(changedTx);
+      const changedFee = changedTx.estimateFee(feeRate);
       if (leastFee > changedFee) {
         throw new Error("The change function removed existed transaction data");
       }
       // The fee has been paid
       if (leastFee === changedFee) {
-        this.copy(tx);
+        this.copy(changedTx);
         return [collected, true];
       }