View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0006373 | Taler | exchange | public | 2020-06-10 13:45 | 2021-08-24 16:23 |
| Reporter | oec | Assigned To | Christian Grothoff | ||
| Priority | normal | Severity | tweak | Reproducibility | always |
| Status | closed | Resolution | won't fix | ||
| Product Version | 0.7.1 | ||||
| Target Version | 0.8 | Fixed in Version | 0.8 | ||
| Summary | 0006373: Simplify check_for_denomination_key by making denomination_key_lookup_by_hash also return the DenominationKeyUse | ||||
| Description | The current signature of TEH_KS_denomination_key_lookup_by_hash expects a TEH_KS_DenominationKeyUse parameter. It is called in the http-handlers for withdraw, deposit, recoup, refund, melt and reveal. Except for melt, the function is called once in each handler. However, the logic in taler-exchange-httpd_melt suffers from multiple calls to ...lookup_by_hash with different values for the ...DenominationKeyUse in nested if-statements. I believe it would be easier to reason about this particular case when TEH_KS_denomination_key_lookup_by_hash would be called with a pointer to ...DenominationKeyUse that will be set to the correct value according to in which map it was found (if at all). Something along the lines of: enum TEH_KS_DenominationKeyUse use; dki = TEH_KS_denomination_key_lookup_by_hash( key_state, &rmc->refresh_session.coin.denom_pub_hash, &use; &ec, &hc); if (NULL = dki) { // free resources; // return with error } switch(use) { case TEH_KS_DKU_DEPOSIT: ... case TEH_KS_DKU_RECOUP: ... case TEH_KS_DKU_ZOMBIE: ... default: ... } IMHO, this would simplify the logic and increase readability a lot For refactoring the other handlers, one could introduce a new function and rename the existing one to TEH_KS_denomination_key_lookup_by_hash_with_use() or so. | ||||
| Tags | No tags attached. | ||||
|
|
The problem with your proposal is that we cannot simply return a 'use' for a denomination key: a denomination key can be valid for deposit and withdraw (at the same time!), or just deposit, or for recoup and zombie (at the same time), or just for zombie but not recoup. Also, as you observed, we do the lookup in different data structures, depending on the use. Thus, if we were to return a 'use' as you propose, a single enum value would have to represent a combination of uses. Messy. So for me, your proposal would not actually simplify the code. |
|
|
I've looked over the code, and for now decided that this will not result in code that is definitively more readable. At best, it seems likely to shift complexity and at worst it may make the 'use' field more difficult to understand. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2020-06-10 13:45 | oec | New Issue | |
| 2020-06-10 13:45 | oec | Status | new => assigned |
| 2020-06-10 13:45 | oec | Assigned To | => Christian Grothoff |
| 2020-06-10 15:22 | Christian Grothoff | Severity | minor => tweak |
| 2020-06-10 15:22 | Christian Grothoff | Product Version | => 0.7.1 |
| 2020-06-10 15:22 | Christian Grothoff | Target Version | => 0.8 |
| 2020-07-15 14:31 | Christian Grothoff | Note Added: 0016455 | |
| 2020-07-15 14:32 | Christian Grothoff | Status | assigned => resolved |
| 2020-07-15 14:32 | Christian Grothoff | Resolution | open => won't fix |
| 2020-07-15 14:32 | Christian Grothoff | Fixed in Version | => 0.8 |
| 2020-07-15 14:32 | Christian Grothoff | Note Added: 0016456 | |
| 2021-08-24 16:23 | Christian Grothoff | Status | resolved => closed |
