View Issue Details

IDProjectCategoryView StatusLast Update
0005987Talermechant backendpublic2020-05-02 19:49
ReporterFlorian DoldAssigned ToChristian Grothoff 
PrioritylowSeverityminorReproducibilityhave not tried
Status resolvedResolutionfixed 
Product Versiongit (master) 
Target Version0.8Fixed in Version0.8 
Summary0005987: review abort-refund mode of /pay
DescriptionIt is currently not clear why the API needs the deposit permissions, when the merchant could just refund all coins deposited so far for this order.

Maybe this was intended to abort a partial payment on device A and complete it on device B? It is not clear from the documentation whether an "aborted" payment can be continued.
TagsNo tags attached.

Activities

Florian Dold

2019-12-06 00:34

manager   ~0015127

Last edited: 2019-12-06 00:35

View 2 revisions

Looks like the "two devices" scenario is why the API was designed this way.

Leaving this open since the rationale for this needs to be clarified in the documentation, and it needs to be emphasized that an aborted payment can still be completed.

Furthermore, it is not clear how this interacts with the API for the merchant frontend: If I pay something for 10 EUR, but do (1) partial pay (2) abort-refund (3) partial pay (3) abort-refund (4) partial pay, then I have a refund of a bit less than 20 EUR, but actually paid 10 EUR plus fees. How is this information presented to a merchant frontend in the /check-payment API?

Christian Grothoff

2020-04-10 21:41

manager   ~0015614

We should make the merchant call the exchange's /refund API here as well, and once this is done, we can likely kill the TALER_EXCHANGE_refund2() API.

Christian Grothoff

2020-04-13 21:34

manager   ~0015641

API redesign does away with the deposit permissions on /abort. Only requires the h_contract_terms for the client to 'authorize' the request.

Christian Grothoff

2020-04-30 21:54

manager   ~0015818

f5609ae..3fb8353 pushes an updated /abort spec. Florian: could you please review & comment? I'm (already) implementing this, but this would be a good time to request changes.

Florian Dold

2020-05-01 09:26

manager   ~0015826

Some feedback on the docs and API:

* We should really make the /abort API and /refund API (or rather /orders/.../ after RESTification) return the list of refund results in the same way!
   (The format for normal refunds isn't specified in the docs at the moment, but the current implementation in master is certainly different.)
* Your current spec doesn't accommodate for the refund potentially failing, it always returns a success result.
* I'm not sure why "AbortingCoin" contains the refund_fee. We don't require this anywhere for normal refunds
* It would be great if the documentation could mention the rationale of *why* we have to give the list of coins to the merchant, when the merchant perfectly knows what coins were used.
   (I'm pretty sure there is a reason and we even discussed it. I think it was to make sure that when two wallets try to pay and then abort, each wallet gets their own set of coins refunded. But if would be great to make that explicit in the documentation.)

Christian Grothoff

2020-05-01 12:04

manager   ~0015827

1) I agree in principle, albeit didn't yet look at /refund.
2) Ack, will need to add those cases;
3) See `struct TALER_RefundRequestPS`: we need to sign over the refund fee as per exchange API. So this raises two questions:
  a) why do we even have/need the refund fee in that structure? Is the exchange API wrong? I suspect we could simplify things
       by removing the refund fee there, but that's a bigger change to the exchange (and auditor!).
  b) if we do need it there, should the merchant fill it in? My conclusion was that if we need it, it should come from the wallet.
4) Yes, I think that is the reason, and we should document it...

Florian Dold

2020-05-01 12:19

manager   ~0015828

Regarding (3):
The merchant should already know the refund fee (since it needs to have downloaded /keys and know about the denomination to have accepted the coin in the first place). I think the reason why `struct TALER_RefundRequestPS` has the refund fees is to just be explicit about it and to avoid misinterpreting the amount fields involved.

Since the merchant is the one making the signature, the merchant should provide the refund fee value. Otherwise the wallet can trick the merchant into signing a refund request with some totally wrong refund fee.

Christian Grothoff

2020-05-01 12:37

manager   ~0015829

I worry about the merchant having 'forgotten' /keys in the meantime (crash, etc). But the primary question is why the refund fee needs to be signed here at all. Right now, seems to just complicate everything (merchant to look it up and exchange who needs to check). But I can remove it from the *merchant* API for now and get it from /keys at the merchant. Still, for the record, I'm not sure why it is required by the _exchange_ API at this point.

Christian Grothoff

2020-05-01 12:46

manager   ~0015830

Last edited: 2020-05-01 12:46

View 2 revisions

About (1): looking at the equivalence of the APIs, I think this is NOT efficient:
1) for /abort refunds, the rtransaction_id is always 0, not so for "normal" refunds;
2) for /abort refunds, the wallet knows the coin pubs and in which order, again not so for "normal" refunds;

So that's be an extra approx. 64 bytes (40 bytes payload, + keys and syntax) per coin to return on /abort that is really redundant. Do we really want to do that? Also, this would still only mean that the "refunds" field would be identicial, the rest of the response would still differ. WDYT?

Florian Dold

2020-05-01 13:38

manager   ~0015831

Ah. Well, if we can make the response significantly smaller, then I guess it's okay to have a different response schema.

A short comment that explains why the schema is different would be great to have in the docs.

Christian Grothoff

2020-05-01 14:03

manager   ~0015832

Actually, leaving out the refund fee does NOT work *unless* we change the exchange API:
The merchant may not know the coin (say it had a database failure), and in particular that means we do not know the denomination public key. So the merchant cannot just lookup the fee in /keys -- unless we also upload h_denom_pub per coin, but that would be MUCH bigger!

So we _either_ remove it also from the exchange API (as in, what the merchant needs to sign over!), or we MUST have it in the merchant API upload. WDYT?

Florian Dold

2020-05-01 16:26

manager   ~0015833

The DepositPermission signature includes the fee separately:
  struct TALER_AmountNBO amount_with_fee;
  struct TALER_AmountNBO deposit_fee;

So for consistency, I think it is a bad idea to remove it from the refund permission.

We already do the "exchange info fetching logic" for /pay, can't we just do the same for /abort? I also wouldn't mind the extra size of the h_denom_pubs.

I expect /abort to be *much* less frequent than /pay, and we should IMHO value consistency over these micro-optimizations.

Christian Grothoff

2020-05-01 16:56

manager   ~0015834

Well, consistency-wise, one could argue for removing it from _both_. Albeit, bigger protocol change.

That said, I really don't like the idea of pushing 512 bit *per coin* to the exchange, if we can just do with 0. I _think_ we added those two fees into the signatures for no good reason...

Florian Dold

2020-05-01 17:35

manager   ~0015835

We should decide first what the value of having (deposit|refund)_fee in the signature blob.

It is basically a free way to double-check that the wallet/merchant indeed agrees with the exchange on the fee.
(It's free because the exchange can just put the value in the signed blob from its own DB and verify the signature, there's no need to send it in the message.)

But of course we can argue that we don't really need this check, since the wallet has proof for the exchange's fee structure via the master_pub-signed denomination.

Christian Grothoff

2020-05-01 17:56

manager   ~0015836

Exactly, and the exchange 'disagreeing' with the wallet merely adds another (pretty useless) error case. Also interesting: the exchange's /withdraw and /refund APIs require the client to send the fee *to* the exchange, while /deposit and /refresh do not (inconsistency!).

Florian Dold

2020-05-01 17:58

manager   ~0015837

Yes, and the disagreement results in a rather useless error message "signature verification failed", which is far removed from what's actually going on.

So I'd vote to eliminate the fees entirely from the signature blob and the exchange's /withdraw and /refund APIs.

Christian Grothoff

2020-05-01 19:08

manager   ~0015838

I'm starting an exchange protocolv8 branch for this (and also Fefe's protocol breaking changes should go there).

Christian Grothoff

2020-05-01 20:00

manager   ~0015840

Ok, fees were removed in the v8 branch of the exchange and now in the merchant as well.

Christian Grothoff

2020-05-02 19:48

manager   ~0015841

Last edited: 2020-05-02 19:49

View 2 revisions

ebe1455..05aa6d5 updates the specification on the errors. So at least in the protocolv1 branch, this issue should be resolved.

Issue History

Date Modified Username Field Change
2019-12-06 00:26 Florian Dold New Issue
2019-12-06 00:26 Florian Dold Status new => assigned
2019-12-06 00:26 Florian Dold Assigned To => Christian Grothoff
2019-12-06 00:34 Florian Dold Note Added: 0015127
2019-12-06 00:35 Florian Dold Note Edited: 0015127 View Revisions
2019-12-06 23:19 Christian Grothoff Priority normal => low
2020-04-10 21:41 Christian Grothoff Note Added: 0015614
2020-04-13 02:42 Christian Grothoff Product Version => git (master)
2020-04-13 02:42 Christian Grothoff Target Version => 0.7.1
2020-04-13 21:33 Christian Grothoff Target Version 0.7.1 => 0.8
2020-04-13 21:34 Christian Grothoff Note Added: 0015641
2020-04-30 21:54 Christian Grothoff Note Added: 0015818
2020-04-30 21:54 Christian Grothoff Assigned To Christian Grothoff => Florian Dold
2020-04-30 21:54 Christian Grothoff Status assigned => feedback
2020-05-01 09:26 Florian Dold Note Added: 0015826
2020-05-01 09:27 Florian Dold Assigned To Florian Dold => Christian Grothoff
2020-05-01 12:04 Christian Grothoff Note Added: 0015827
2020-05-01 12:19 Florian Dold Note Added: 0015828
2020-05-01 12:19 Florian Dold Status feedback => assigned
2020-05-01 12:37 Christian Grothoff Note Added: 0015829
2020-05-01 12:46 Christian Grothoff Note Added: 0015830
2020-05-01 12:46 Christian Grothoff Note Edited: 0015830 View Revisions
2020-05-01 13:38 Florian Dold Note Added: 0015831
2020-05-01 14:03 Christian Grothoff Note Added: 0015832
2020-05-01 16:26 Florian Dold Note Added: 0015833
2020-05-01 16:56 Christian Grothoff Note Added: 0015834
2020-05-01 17:35 Florian Dold Note Added: 0015835
2020-05-01 17:56 Christian Grothoff Note Added: 0015836
2020-05-01 17:58 Florian Dold Note Added: 0015837
2020-05-01 19:08 Christian Grothoff Note Added: 0015838
2020-05-01 20:00 Christian Grothoff Note Added: 0015840
2020-05-02 19:48 Christian Grothoff Note Added: 0015841
2020-05-02 19:49 Christian Grothoff Status assigned => resolved
2020-05-02 19:49 Christian Grothoff Resolution open => fixed
2020-05-02 19:49 Christian Grothoff Fixed in Version => 0.8
2020-05-02 19:49 Christian Grothoff Note Edited: 0015841 View Revisions