View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0006188 | Taler | exchange | public | 2020-04-17 19:25 | 2021-08-24 16:23 |
| Reporter | oec | Assigned To | Christian Grothoff | ||
| Priority | none | Severity | tweak | Reproducibility | N/A |
| Status | closed | Resolution | fixed | ||
| Product Version | git (master) | ||||
| Target Version | 0.8 | Fixed in Version | 0.8 | ||
| Summary | 0006188: Lift binary arguments into function names | ||||
| Description | Compare ``` get_coin_transactions(foo, bar, buz, GNUNET_OK, buf); get_coin_transactions(foo, bar, buz, GNUNET_NO, buf); ``` against ``` get_coin_transactions_including_recoup(foo, bur, buz, buf); get_coin_transactions_without_recoup(foo, bur, buz, buf); ``` Advantages of the later: - function names become more descriptive - for developers: less likely to pick the wrong function than to use the wrong boolean argument - for code auditors: easier to reason about Non-exhaustive list of APIs, that would benefit from this: - `get_coin_transactions`, i.e. `postgres_get_coin_transactions` - `decode_keys_json` - `TALER_EXCHANGE_check_keys_current` PS: Also, of course a lot of APIs in gnunet would benefit from this: - `GNUNET_OS_start_process` - `GNUNET_CURL_job_add` - `GNUNET_DISK_pipe` - `GNUNET_ATS_AddressInformationCallback` - `GNUNET_ATS_PeerInfo_Iterator` - `GNUNET_ATS_performance_list_addresses` - ... | ||||
| Tags | No tags attached. | ||||
|
|
Ok, sounds reasonable, assuming the two "new" functions internally call the old one unless they don't share much code. |
|
|
If you find other candidates, please add as notes to this bugreport. |
|
|
Also: - postgres_have_deposit , have_deposit - TALER_EXCHANGEDB_DepositCallback - TALER_EXCHANGEDB_WirePreparationCallback |
|
|
Looking at the first details, I'm not so convinced: - `get_coin_transactions`, i.e. `postgres_get_coin_transactions` --- that's in the Postgres DB API. Any change here means that all plugins will have to provide both functions, we have to declare both functions in the header, initialize them in the plugin struct, etc. Very messy, for very limited gain (the function is not used all that often). - `decode_keys_json` --- that's not even in an API, that's a purely internal function inside a file, and only called twice. Also the two code paths are inherently very mangled, so it is not like it would make sense to really have two functions. - `TALER_EXCHANGE_check_keys_current` --- here I would worry about the combinatorial explosion, as it takes two boolean arguments. I think here a good solution would be to replace the two booleans with an enum, that way the caller will pass readable flags, and we get down from three args to two. |
|
|
c326a5bd..b9f1384b improvesthe check_keys_current API, introducing an enum. |
|
|
b9f1384b..4fde7604 updates GNUNET_CURL_job_add() to avoid boolean arg. |
|
|
Todo: - `GNUNET_OS_start_process` - use enum? - `GNUNET_DISK_pipe` - use enum? ATS APIs: dead with TNG anyway, no point. |
|
|
GNUNET_OS_start_process() API fixed in c7e00e63..62963ae4 (exchange). |
|
|
80ba1c6ebe03021dc464d4c3273607d1fa990de5 (exchange.git) updates the GNUNET_DISK_pipe() API. While there may be others left that in principle have this problem, I'm resolving the bug as the ones that where specifically mentioned have been addressed or at least reviewed. If somebody finds others, a new bug should be opened. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2020-04-17 19:25 | oec | New Issue | |
| 2020-04-17 19:25 | oec | Status | new => assigned |
| 2020-04-17 19:25 | oec | Assigned To | => Christian Grothoff |
| 2020-04-17 19:46 | Christian Grothoff | Note Added: 0015684 | |
| 2020-04-17 19:46 | Christian Grothoff | Note Added: 0015685 | |
| 2020-04-20 18:20 | oec | Note Added: 0015709 | |
| 2020-04-21 21:00 | Christian Grothoff | Assigned To | Christian Grothoff => |
| 2020-04-21 21:00 | Christian Grothoff | Status | assigned => confirmed |
| 2020-04-21 21:00 | Christian Grothoff | Product Version | => git (master) |
| 2020-04-21 21:00 | Christian Grothoff | Target Version | => 0.9 |
| 2020-07-16 15:18 | Christian Grothoff | Assigned To | => Christian Grothoff |
| 2020-07-16 15:18 | Christian Grothoff | Status | confirmed => assigned |
| 2020-07-16 18:17 | Christian Grothoff | Note Added: 0016470 | |
| 2020-07-16 20:34 | Christian Grothoff | Note Added: 0016476 | |
| 2020-07-16 20:43 | Christian Grothoff | Note Added: 0016477 | |
| 2020-07-16 21:08 | Christian Grothoff | Note Added: 0016478 | |
| 2020-07-17 22:43 | Christian Grothoff | Note Added: 0016486 | |
| 2020-07-18 00:53 | Christian Grothoff | Note Added: 0016487 | |
| 2020-07-18 00:53 | Christian Grothoff | Status | assigned => resolved |
| 2020-07-18 00:53 | Christian Grothoff | Resolution | open => fixed |
| 2020-07-18 00:53 | Christian Grothoff | Fixed in Version | => 0.8 |
| 2020-07-18 00:53 | Christian Grothoff | Target Version | 0.9 => 0.8 |
| 2021-08-24 16:23 | Christian Grothoff | Status | resolved => closed |
