[taler-exchange] branch master updated: some comments on aggregator

[gnunet]

This is an automated email from the git hooks/post-receive script.

dold pushed a commit to branch master
in repository exchange.

The following commit(s) were added to refs/heads/master by this push:
     new c87eb30e some comments on aggregator
c87eb30e is described below

commit c87eb30e78f84696c1ad22abd97c119db8dfad26
Author: Florian Dold <address@hidden>
AuthorDate: Mon Jan 20 00:07:39 2020 +0100

    some comments on aggregator
---
 src/exchange/taler-exchange-aggregator.c | 46 ++++++++++++++++++++------------
 src/exchangedb/exchangedb_accounts.c     |  1 +
 src/include/taler_exchangedb_lib.h       |  1 +
 3 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/src/exchange/taler-exchange-aggregator.c 
b/src/exchange/taler-exchange-aggregator.c
index 662a8aa2..8c01213d 100644
--- a/src/exchange/taler-exchange-aggregator.c
+++ b/src/exchange/taler-exchange-aggregator.c
@@ -331,7 +331,7 @@ advance_fees (struct WireAccount *wa,
 
 
 /**
- * Update wire transfer fee data structure in @a wp.
+ * Update wire transfer fee data structure in @a wa.
  *
  * @param wa wire account data structure to update
  * @param now timestamp to update fees to
@@ -409,6 +409,7 @@ find_account_by_method (const char *method)
  * @param url wire address we need an account for
  * @return NULL on error
  */
+// FIXME(dold): should be find_account_by_uri
 static struct WireAccount *
 find_account_by_url (const char *url)
 {
@@ -603,24 +604,21 @@ exchange_serve_process_config ()
     return GNUNET_SYSERR;
   }
 
+  if ( (GNUNET_OK !=
+        TALER_config_get_amount (cfg,
+                                 "taler",
+                                 "CURRENCY_ROUND_UNIT",
+                                 &currency_round_unit)) ||
+       (0 != strcasecmp (exchange_currency_string,
+                         currency_round_unit.currency)) ||
+       ( (0 != currency_round_unit.fraction) &&
+         (0 != currency_round_unit.value) ) )
   {
-    if ( (GNUNET_OK !=
-          TALER_config_get_amount (cfg,
-                                   "taler",
-                                   "CURRENCY_ROUND_UNIT",
-                                   &currency_round_unit)) ||
-         (0 != strcasecmp (exchange_currency_string,
-                           currency_round_unit.currency)) ||
-         ( (0 != currency_round_unit.fraction) &&
-           (0 != currency_round_unit.value) ) )
-    {
-      GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
-                  "Invalid value specified in section `TALER' under 
`CURRENCY_ROUND_UNIT'\n");
-      return GNUNET_SYSERR;
-    }
+    GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
+                "Invalid value specified in section `TALER' under 
`CURRENCY_ROUND_UNIT'\n");
+    return GNUNET_SYSERR;
   }
 
-
   if (NULL ==
       (db_plugin = TALER_EXCHANGEDB_plugin_load (cfg)))
   {
@@ -744,6 +742,9 @@ deposit_cb (void *cls,
                                amount_with_fee,
                                deposit_fee))
     {
+      // FIXME(dold): Shouldn't we somehow survive this?  Of course it should 
show up in the auditor's report,
+      // but due to some misconfiguration with restarts in between deposit 
fees could have changed.
+      // That's bad, but should we really refuse to continue completely?
       GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
                   "Fatally malformed record at row %llu over %s\n",
                   (unsigned long long) row_id,
@@ -963,6 +964,8 @@ aggregate_cb (void *cls,
 }
 
 
+// FIXME(dold): we should move these to the top
+
 /**
  * Main work function that finds and triggers transfers for reserves
  * closures.
@@ -1044,6 +1047,7 @@ struct ExpiredReserveContext
  * @param cls a `struct ExpiredReserveContext *`
  * @param reserve_pub public key of the reserve
  * @param left amount left in the reserve
+ * // FIXME(dold):  So the following parameter is a payto URI?
  * @param account_details information about the reserve's bank account
  * @param expiration_date when did the reserve expire
  * @return transaction status code
@@ -1310,6 +1314,8 @@ run_reserve_closures (void *cls)
     task = GNUNET_SCHEDULER_add_now (&run_reserve_closures,
                                      NULL);
     return;
+    // FIXME(dold): shouldn't we at least GNUNET_break on the case where
+    // we have more than one result?  Not clear that get_expired reserves 
can't return more results?
   }
 }
 
@@ -1323,6 +1329,7 @@ run_reserve_closures (void *cls)
 static void
 run_aggregation (void *cls)
 {
+  // FIXME(dold): This should be unsigned, as behavior on signed overflow is 
undefined ;-)
   static int swap;
   struct TALER_EXCHANGEDB_Session *session;
   enum GNUNET_DB_QueryStatus qs;
@@ -1404,6 +1411,8 @@ run_aggregation (void *cls)
         task = GNUNET_SCHEDULER_add_now (&run_reserve_closures,
                                          NULL);
       else
+        // FIXME(dold): Why a minute?  Maybe for some giant "wallet 
integration test" we want this
+        // to be configurable to something much faster?
         /* nothing to do, sleep for a minute and try again */
         task = GNUNET_SCHEDULER_add_delayed (GNUNET_TIME_UNIT_MINUTES,
                                              &run_aggregation,
@@ -1746,7 +1755,7 @@ wire_prepare_cb (void *cls,
   if (NULL == wpd->wa)
   {
     /* Should really never happen here, as when we get
-       here the plugin should be in the cache. */
+       here the wire account should be in the cache. */
     GNUNET_break (0);
     db_plugin->rollback (db_plugin->cls,
                          wpd->session);
@@ -1765,6 +1774,7 @@ wire_prepare_cb (void *cls,
                                               NULL);
   if (NULL == wpd->eh)
   {
+    // FIXME(dold): Comment below is a fixme, but without a marker!
     GNUNET_break (0); /* why? how to best recover? */
     db_plugin->rollback (db_plugin->cls,
                          wpd->session);
@@ -1822,6 +1832,8 @@ run_transfers (void *cls)
                                          session,
                                          &wire_prepare_cb,
                                          NULL);
+  // FIXME(dold): comment below is misleading, should't that be 
wire_confirm_cb,
+  // as wire_prepare_cb is already called?
   if (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT == qs)
     return;  /* continues in #wire_prepare_cb() */
   db_plugin->rollback (db_plugin->cls,
diff --git a/src/exchangedb/exchangedb_accounts.c 
b/src/exchangedb/exchangedb_accounts.c
index 532e27f6..aa923c5d 100644
--- a/src/exchangedb/exchangedb_accounts.c
+++ b/src/exchangedb/exchangedb_accounts.c
@@ -121,6 +121,7 @@ check_for_account (void *cls,
  * @param cb callback to invoke
  * @param cb_cls closure for @a cb
  */
+// FIXME(dold): why is this part of the exchange database?  Does this really 
belong here?
 void
 TALER_EXCHANGEDB_find_accounts (const struct GNUNET_CONFIGURATION_Handle *cfg,
                                 TALER_EXCHANGEDB_AccountCallback cb,
diff --git a/src/include/taler_exchangedb_lib.h 
b/src/include/taler_exchangedb_lib.h
index 981cda84..59864dd4 100644
--- a/src/include/taler_exchangedb_lib.h
+++ b/src/include/taler_exchangedb_lib.h
@@ -298,6 +298,7 @@ TALER_EXCHANGEDB_plugin_unload (struct 
TALER_EXCHANGEDB_Plugin *plugin);
 
 
 /**
+ * FIXME(dold): Sorted by what, start or end?
  * Sorted list of fees to be paid for aggregate wire transfers.
  */
 struct TALER_EXCHANGEDB_AggregateFees

-- 
To stop receiving notification emails like this one, please contact
address@hidden.