View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0005987 | Taler | mechant backend | public | 2019-12-06 00:26 | 2021-08-24 16:23 |
| Reporter | Florian Dold | Assigned To | Christian Grothoff | ||
| Priority | low | Severity | minor | Reproducibility | have not tried |
| Status | closed | Resolution | fixed | ||
| Product Version | git (master) | ||||
| Target Version | 0.8 | Fixed in Version | 0.8 | ||
| Summary | 0005987: review abort-refund mode of /pay | ||||
| Description | It 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. | ||||
| Tags | No tags attached. | ||||
|
|
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? |
|
|
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. |
|
|
API redesign does away with the deposit permissions on /abort. Only requires the h_contract_terms for the client to 'authorize' the request. |
|
|
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. |
|
|
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.) |
|
|
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... |
|
|
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. |
|
|
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. |
|
|
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? |
|
|
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. |
|
|
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? |
|
|
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. |
|
|
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... |
|
|
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. |
|
|
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!). |
|
|
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. |
|
|
I'm starting an exchange protocolv8 branch for this (and also Fefe's protocol breaking changes should go there). |
|
|
Ok, fees were removed in the v8 branch of the exchange and now in the merchant as well. |
|
|
ebe1455..05aa6d5 updates the specification on the errors. So at least in the protocolv1 branch, this issue should be resolved. |
| 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 | |
| 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 | |
| 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 | |
| 2021-08-24 16:23 | Christian Grothoff | Status | resolved => closed |
