wallet-core: correctly consider deposit fee in p2p coin selection

packages/taler-wallet-core/src/util/coinSelection.test.ts

@@ -15,49 +15,58 @@
  */
 import {
   AbsoluteTime,
-  AgeRestriction,
-  AmountJson,
   AmountString,
   Amounts,
   DenomKeyType,
   Duration,
-  TransactionAmountMode,
+  j2s,
 } from "@gnu-taler/taler-util";
 import test, { ExecutionContext } from "ava";
-import { AvailableDenom, testing_greedySelectPeer, testing_selectGreedy } from "./coinSelection.js"
+import {
+  AvailableDenom,
+  testing_greedySelectPeer,
+  testing_selectGreedy,
+} from "./coinSelection.js";
 
 const inTheDistantFuture = AbsoluteTime.toProtocolTimestamp(
-  AbsoluteTime.addDuration(AbsoluteTime.now(), Duration.fromSpec({ hours: 1 }))
+  AbsoluteTime.addDuration(AbsoluteTime.now(), Duration.fromSpec({ hours: 1 })),
-)
+);
 const inThePast = AbsoluteTime.toProtocolTimestamp(
-  AbsoluteTime.subtractDuraction(AbsoluteTime.now(), Duration.fromSpec({ hours: 1 }))
+  AbsoluteTime.subtractDuraction(
-)
+    AbsoluteTime.now(),
+    Duration.fromSpec({ hours: 1 }),
+  ),
+);
 
 test("p2p: should select the coin", (t) => {
-  const instructedAmount = Amounts.parseOrThrow("LOCAL:2")
+  const instructedAmount = Amounts.parseOrThrow("LOCAL:2");
   const tally = {
     amountAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
     depositFeesAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
     lastDepositFee: Amounts.zeroOfCurrency(instructedAmount.currency),
   };
   const coins = testing_greedySelectPeer(
-    createCandidates([{
+    createCandidates([
+      {
         amount: "LOCAL:10",
         numAvailable: 5,
         depositFee: "LOCAL:0.1",
         fromExchange: "http://exchange.localhost/",
-    }]),
+      },
+    ]),
     instructedAmount,
-    tally
+    tally,
   );
 
+  t.log(j2s(coins));
+
   expect(t, coins).deep.equal({
     "hash0;32;http://exchange.localhost/": {
       exchangeBaseUrl: "http://exchange.localhost/",
       denomPubHash: "hash0",
       maxAge: 32,
-      contributions: [Amounts.parseOrThrow("LOCAL:2")],
+      contributions: [Amounts.parseOrThrow("LOCAL:2.1")],
-    }
+    },
   });
 
   expect(t, tally).deep.equal({
@@ -65,25 +74,26 @@ test("p2p: should select the coin", (t) => {
     depositFeesAcc: Amounts.parseOrThrow("LOCAL:0.1"),
     lastDepositFee: Amounts.parseOrThrow("LOCAL:0.1"),
   });
-
 });
 
 test("p2p: should select 3 coins", (t) => {
-  const instructedAmount = Amounts.parseOrThrow("LOCAL:20")
+  const instructedAmount = Amounts.parseOrThrow("LOCAL:20");
   const tally = {
     amountAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
     depositFeesAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
     lastDepositFee: Amounts.zeroOfCurrency(instructedAmount.currency),
   };
   const coins = testing_greedySelectPeer(
-    createCandidates([{
+    createCandidates([
+      {
         amount: "LOCAL:10",
         numAvailable: 5,
         depositFee: "LOCAL:0.1",
         fromExchange: "http://exchange.localhost/",
-    }]),
+      },
+    ]),
     instructedAmount,
-    tally
+    tally,
   );
 
   expect(t, coins).deep.equal({
@@ -94,9 +104,9 @@ test("p2p: should select 3 coins", (t) => {
       contributions: [
         Amounts.parseOrThrow("LOCAL:9.9"),
         Amounts.parseOrThrow("LOCAL:9.9"),
-        Amounts.parseOrThrow("LOCAL:0.2")
+        Amounts.parseOrThrow("LOCAL:0.5"),
       ],
-    }
+    },
   });
 
   expect(t, tally).deep.equal({
@@ -104,41 +114,41 @@ test("p2p: should select 3 coins", (t) => {
     depositFeesAcc: Amounts.parseOrThrow("LOCAL:0.3"),
     lastDepositFee: Amounts.parseOrThrow("LOCAL:0.1"),
   });
-
 });
 
 test("p2p: can't select since the instructed amount is too high", (t) => {
-  const instructedAmount = Amounts.parseOrThrow("LOCAL:60")
+  const instructedAmount = Amounts.parseOrThrow("LOCAL:60");
   const tally = {
     amountAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
     depositFeesAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
     lastDepositFee: Amounts.zeroOfCurrency(instructedAmount.currency),
   };
   const coins = testing_greedySelectPeer(
-    createCandidates([{
+    createCandidates([
+      {
         amount: "LOCAL:10",
         numAvailable: 5,
         depositFee: "LOCAL:0.1",
         fromExchange: "http://exchange.localhost/",
-    }]),
+      },
+    ]),
     instructedAmount,
-    tally
+    tally,
   );
 
   expect(t, coins).deep.equal(undefined);
 
   expect(t, tally).deep.equal({
-    amountAcc: Amounts.parseOrThrow("LOCAL:49.5"),
+    amountAcc: Amounts.parseOrThrow("LOCAL:49"),
     depositFeesAcc: Amounts.parseOrThrow("LOCAL:0.5"),
     lastDepositFee: Amounts.parseOrThrow("LOCAL:0.1"),
   });
-
 });
 
 test("pay: select one coin to pay with fee", (t) => {
-  const payment = Amounts.parseOrThrow("LOCAL:2")
+  const payment = Amounts.parseOrThrow("LOCAL:2");
-  const exchangeWireFee = Amounts.parseOrThrow("LOCAL:0.1")
+  const exchangeWireFee = Amounts.parseOrThrow("LOCAL:0.1");
-  const zero = Amounts.zeroOfCurrency(payment.currency)
+  const zero = Amounts.zeroOfCurrency(payment.currency);
   const tally = {
     amountPayRemaining: payment,
     amountWireFeeLimitRemaining: zero,
@@ -150,28 +160,30 @@ test("pay: select one coin to pay with fee", (t) => {
   };
   const coins = testing_selectGreedy(
     {
-      "auditors": [],
+      auditors: [],
-      "exchanges": [
+      exchanges: [
         {
-              "exchangeBaseUrl": "http://exchange.localhost/",
+          exchangeBaseUrl: "http://exchange.localhost/",
-              "exchangePub": "E5M8CGRDHXF1RCVP3B8TQCTDYNQ7T4XHWR5SVEQRGVVMVME41VJ0"
+          exchangePub: "E5M8CGRDHXF1RCVP3B8TQCTDYNQ7T4XHWR5SVEQRGVVMVME41VJ0",
-          }
-      ],
-      "contractTermsAmount": payment,
-      "depositFeeLimit": zero,
-      "wireFeeAmortization": 1,
-      "wireFeeLimit": zero,
-      "prevPayCoins": [],
-      "wireMethod": "x-taler-bank"
         },
-    createCandidates([{
+      ],
+      contractTermsAmount: payment,
+      depositFeeLimit: zero,
+      wireFeeAmortization: 1,
+      wireFeeLimit: zero,
+      prevPayCoins: [],
+      wireMethod: "x-taler-bank",
+    },
+    createCandidates([
+      {
         amount: "LOCAL:10",
         numAvailable: 5,
         depositFee: "LOCAL:0.1",
         fromExchange: "http://exchange.localhost/",
-    }]),
+      },
+    ]),
     { "http://exchange.localhost/": exchangeWireFee },
-    tally
+    tally,
   );
 
   expect(t, coins).deep.equal({
@@ -179,10 +191,8 @@ test("pay: select one coin to pay with fee", (t) => {
       exchangeBaseUrl: "http://exchange.localhost/",
       denomPubHash: "hash0",
       maxAge: 32,
-      contributions: [
+      contributions: [Amounts.parseOrThrow("LOCAL:2.2")],
-        Amounts.parseOrThrow("LOCAL:2.2"),
+    },
-      ],
-    }
   });
 
   expect(t, tally).deep.equal({
@@ -194,44 +204,46 @@ test("pay: select one coin to pay with fee", (t) => {
     wireFeeCoveredForExchange: new Set(),
     lastDepositFee: zero,
   });
-
 });
 
-
+function createCandidates(
-
+  ar: {
-
+    amount: AmountString;
-function createCandidates(ar: {amount: AmountString, depositFee: AmountString, numAvailable: number, fromExchange: string}[]): AvailableDenom[] {
+    depositFee: AmountString;
+    numAvailable: number;
+    fromExchange: string;
+  }[],
+): AvailableDenom[] {
   return ar.map((r, idx) => {
     return {
-      "denomPub": {
+      denomPub: {
-        "age_mask": 0,
+        age_mask: 0,
-        "cipher": DenomKeyType.Rsa,
+        cipher: DenomKeyType.Rsa,
-        "rsa_public_key": "PPP"
+        rsa_public_key: "PPP",
       },
-      "denomPubHash": `hash${idx}`,
+      denomPubHash: `hash${idx}`,
-      "value": r.amount,
+      value: r.amount,
-      "feeDeposit": r.depositFee,
+      feeDeposit: r.depositFee,
-      "feeRefresh": "LOCAL:0",
+      feeRefresh: "LOCAL:0",
-      "feeRefund": "LOCAL:0",
+      feeRefund: "LOCAL:0",
-      "feeWithdraw": "LOCAL:0",
+      feeWithdraw: "LOCAL:0",
-      "stampExpireDeposit": inTheDistantFuture,
+      stampExpireDeposit: inTheDistantFuture,
-      "stampExpireLegal": inTheDistantFuture,
+      stampExpireLegal: inTheDistantFuture,
-      "stampExpireWithdraw": inTheDistantFuture,
+      stampExpireWithdraw: inTheDistantFuture,
-      "stampStart": inThePast,
+      stampStart: inThePast,
-      "exchangeBaseUrl": r.fromExchange,
+      exchangeBaseUrl: r.fromExchange,
-      "numAvailable": r.numAvailable,
+      numAvailable: r.numAvailable,
-      "maxAge": 32,
+      maxAge: 32,
-
+    };
-    }
+  });
-  })
 }
 
 type Tester<T> = {
   deep: {
     equal(another: T): ReturnType<ExecutionContext["deepEqual"]>;
     equals(another: T): ReturnType<ExecutionContext["deepEqual"]>;
-  }
+  };
-}
+};
 
 function expect<T>(t: ExecutionContext, thing: T): Tester<T> {
   return {
@@ -241,4 +253,3 @@ function expect<T>(t: ExecutionContext, thing: T): Tester<T> {
     },
   };
 }
-

packages/taler-wallet-core/src/util/coinSelection.ts

@@ -419,8 +419,10 @@ interface SelResult {
   };
 }
 
-export function testing_selectGreedy(...args: Parameters<typeof selectGreedy>): ReturnType<typeof selectGreedy>{
+export function testing_selectGreedy(
-  return selectGreedy(...args)
+  ...args: Parameters<typeof selectGreedy>
+): ReturnType<typeof selectGreedy> {
+  return selectGreedy(...args);
 }
 function selectGreedy(
   req: SelectPayCoinRequestNg,
@@ -900,9 +902,12 @@ interface PeerCoinSelectionTally {
 /**
  * exporting for testing
  */
-export function testing_greedySelectPeer(...args: Parameters<typeof greedySelectPeer>): ReturnType<typeof greedySelectPeer> {
+export function testing_greedySelectPeer(
-  return greedySelectPeer(...args)
+  ...args: Parameters<typeof greedySelectPeer>
+): ReturnType<typeof greedySelectPeer> {
+  return greedySelectPeer(...args);
 }
+
 function greedySelectPeer(
   candidates: AvailableDenom[],
   instructedAmount: AmountLike,
@@ -921,11 +926,16 @@ function greedySelectPeer(
         instructedAmount,
         tally.amountAcc,
       ).amount;
-      const coinContrib = Amounts.sub(denom.value, denom.feeDeposit).amount
+      // Maximum amount the coin could effectively contribute.
+      const maxCoinContrib = Amounts.sub(denom.value, denom.feeDeposit).amount;
 
-      const coinSpend = Amounts.min(amountPayRemaining, coinContrib)
+      const coinSpend = Amounts.min(
+        Amounts.add(amountPayRemaining, denom.feeDeposit).amount,
+        maxCoinContrib,
+      );
 
       tally.amountAcc = Amounts.add(tally.amountAcc, coinSpend).amount;
+      tally.amountAcc = Amounts.sub(tally.amountAcc, denom.feeDeposit).amount;
 
       tally.depositFeesAcc = Amounts.add(
         tally.depositFeesAcc,