View Issue Details

IDProjectCategoryView StatusLast Update
0006373Talerexchangepublic2020-07-15 14:32
Reporteroec Assigned ToChristian Grothoff  
PrioritynormalSeveritytweakReproducibilityalways
Status resolvedResolutionwon't fix 
Product Version0.7.1 
Target Version0.8Fixed in Version0.8 
Summary0006373: Simplify check_for_denomination_key by making denomination_key_lookup_by_hash also return the DenominationKeyUse
DescriptionThe 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.
TagsNo tags attached.

Activities

Christian Grothoff

2020-07-15 14:31

manager   ~0016455

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.

Christian Grothoff

2020-07-15 14:32

manager   ~0016456

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.

Issue History

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