From a15eec55d3136b4f737c68ac41e3042624b8e25f Mon Sep 17 00:00:00 2001 From: Florian Dold Date: Fri, 15 Sep 2023 16:45:12 +0200 Subject: [PATCH] wallet-core: correctly consider deposit fee in p2p coin selection --- .../src/util/coinSelection.test.ts | 209 +++++++++--------- .../src/util/coinSelection.ts | 24 +- 2 files changed, 127 insertions(+), 106 deletions(-) diff --git a/packages/taler-wallet-core/src/util/coinSelection.test.ts b/packages/taler-wallet-core/src/util/coinSelection.test.ts index f809c4e60..81a656f8a 100644 --- a/packages/taler-wallet-core/src/util/coinSelection.test.ts +++ b/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([{ - amount: "LOCAL:10", - numAvailable: 5, - depositFee: "LOCAL:0.1", - fromExchange: "http://exchange.localhost/", - }]), - instructedAmount, - tally + createCandidates([ + { + amount: "LOCAL:10", + numAvailable: 5, + depositFee: "LOCAL:0.1", + fromExchange: "http://exchange.localhost/", + }, + ]), + instructedAmount, + 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([{ - amount: "LOCAL:10", - numAvailable: 5, - depositFee: "LOCAL:0.1", - fromExchange: "http://exchange.localhost/", - }]), - instructedAmount, - tally + createCandidates([ + { + amount: "LOCAL:10", + numAvailable: 5, + depositFee: "LOCAL:0.1", + fromExchange: "http://exchange.localhost/", + }, + ]), + instructedAmount, + 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([{ - amount: "LOCAL:10", - numAvailable: 5, - depositFee: "LOCAL:0.1", - fromExchange: "http://exchange.localhost/", - }]), - instructedAmount, - tally + createCandidates([ + { + amount: "LOCAL:10", + numAvailable: 5, + depositFee: "LOCAL:0.1", + fromExchange: "http://exchange.localhost/", + }, + ]), + instructedAmount, + 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 exchangeWireFee = Amounts.parseOrThrow("LOCAL:0.1") - const zero = Amounts.zeroOfCurrency(payment.currency) + const payment = Amounts.parseOrThrow("LOCAL:2"); + const exchangeWireFee = Amounts.parseOrThrow("LOCAL:0.1"); + 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": [], - "exchanges": [ - { - "exchangeBaseUrl": "http://exchange.localhost/", - "exchangePub": "E5M8CGRDHXF1RCVP3B8TQCTDYNQ7T4XHWR5SVEQRGVVMVME41VJ0" - } + auditors: [], + exchanges: [ + { + exchangeBaseUrl: "http://exchange.localhost/", + exchangePub: "E5M8CGRDHXF1RCVP3B8TQCTDYNQ7T4XHWR5SVEQRGVVMVME41VJ0", + }, ], - "contractTermsAmount": payment, - "depositFeeLimit": zero, - "wireFeeAmortization": 1, - "wireFeeLimit": zero, - "prevPayCoins": [], - "wireMethod": "x-taler-bank" + 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 + createCandidates([ + { + amount: "LOCAL:10", + numAvailable: 5, + depositFee: "LOCAL:0.1", + fromExchange: "http://exchange.localhost/", + }, + ]), + { "http://exchange.localhost/": exchangeWireFee }, + 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: [ - Amounts.parseOrThrow("LOCAL:2.2"), - ], - } + contributions: [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, depositFee: AmountString, numAvailable: number, fromExchange: string}[]): AvailableDenom[] { - return ar.map((r,idx) => { +function createCandidates( + ar: { + amount: AmountString; + depositFee: AmountString; + numAvailable: number; + fromExchange: string; + }[], +): AvailableDenom[] { + return ar.map((r, idx) => { return { - "denomPub": { - "age_mask": 0, - "cipher": DenomKeyType.Rsa, - "rsa_public_key": "PPP" + denomPub: { + age_mask: 0, + cipher: DenomKeyType.Rsa, + rsa_public_key: "PPP", }, - "denomPubHash": `hash${idx}`, - "value": r.amount, - "feeDeposit": r.depositFee, - "feeRefresh": "LOCAL:0", - "feeRefund": "LOCAL:0", - "feeWithdraw": "LOCAL:0", - "stampExpireDeposit": inTheDistantFuture, - "stampExpireLegal": inTheDistantFuture, - "stampExpireWithdraw": inTheDistantFuture, - "stampStart": inThePast, - "exchangeBaseUrl": r.fromExchange, - "numAvailable": r.numAvailable, - "maxAge": 32, - - } - }) + denomPubHash: `hash${idx}`, + value: r.amount, + feeDeposit: r.depositFee, + feeRefresh: "LOCAL:0", + feeRefund: "LOCAL:0", + feeWithdraw: "LOCAL:0", + stampExpireDeposit: inTheDistantFuture, + stampExpireLegal: inTheDistantFuture, + stampExpireWithdraw: inTheDistantFuture, + stampStart: inThePast, + exchangeBaseUrl: r.fromExchange, + numAvailable: r.numAvailable, + maxAge: 32, + }; + }); } type Tester = { deep: { equal(another: T): ReturnType; equals(another: T): ReturnType; - } -} + }; +}; function expect(t: ExecutionContext, thing: T): Tester { return { @@ -241,4 +253,3 @@ function expect(t: ExecutionContext, thing: T): Tester { }, }; } - diff --git a/packages/taler-wallet-core/src/util/coinSelection.ts b/packages/taler-wallet-core/src/util/coinSelection.ts index 0b1be881f..8c90f26f1 100644 --- a/packages/taler-wallet-core/src/util/coinSelection.ts +++ b/packages/taler-wallet-core/src/util/coinSelection.ts @@ -419,8 +419,10 @@ interface SelResult { }; } -export function testing_selectGreedy(...args: Parameters): ReturnType{ - return selectGreedy(...args) +export function testing_selectGreedy( + ...args: Parameters +): ReturnType { + return selectGreedy(...args); } function selectGreedy( req: SelectPayCoinRequestNg, @@ -900,9 +902,12 @@ interface PeerCoinSelectionTally { /** * exporting for testing */ -export function testing_greedySelectPeer(...args: Parameters): ReturnType { - return greedySelectPeer(...args) +export function testing_greedySelectPeer( + ...args: Parameters +): ReturnType { + 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( + Amounts.add(amountPayRemaining, denom.feeDeposit).amount, + maxCoinContrib, + ); - const coinSpend = Amounts.min(amountPayRemaining, coinContrib) - tally.amountAcc = Amounts.add(tally.amountAcc, coinSpend).amount; + tally.amountAcc = Amounts.sub(tally.amountAcc, denom.feeDeposit).amount; tally.depositFeesAcc = Amounts.add( tally.depositFeesAcc,