Remove Tx model

There was a split between a transaction request and an actual
transaction. This was kind of annoying because transactions were one-way
only, while transaction requests were two-way - which is what I believe
most transactions will be using. Tx model has been removed and the
responsibilities of it are covered by TxRequest. It simplifies
everything surrounding transactions, since we have only one model to
deal with instead of two.

Signed-off-by: Alek Ratzloff <alekratz@gmail.com>

trading/migrations/0001_initial.py

@@ -1,4 +1,4 @@
-# Generated by Django 2.2 on 2020-03-21 18:57
+# Generated by Django 2.2 on 2020-03-26 16:27
 
 from django.conf import settings
 from django.db import migrations, models
@@ -56,32 +56,16 @@ class Migration(migrations.Migration):
         migrations.CreateModel(
             name='TxRequest',
             fields=[
-                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('id', hashid_field.field.HashidAutoField(alphabet='abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890', min_length=7, primary_key=True, serialize=False)),
                 ('status', models.CharField(choices=[('OPEN', 'Open'), ('DECL', 'Declined'), ('ACC', 'Accepted')], default='OPEN', max_length=2)),
                 ('source_amount', models.PositiveIntegerField(editable=False)),
                 ('dest_amount', models.PositiveIntegerField(editable=False)),
-                ('dest', models.ForeignKey(editable=False, on_delete=django.db.models.deletion.CASCADE, related_name='transaction_requests_received', to=settings.AUTH_USER_MODEL)),
+                ('dest', models.ForeignKey(editable=False, on_delete=django.db.models.deletion.CASCADE, related_name='tx_received', to=settings.AUTH_USER_MODEL)),
                 ('dest_sends', models.ForeignKey(editable=False, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='+', to='trading.Commodity')),
-                ('source', models.ForeignKey(editable=False, on_delete=django.db.models.deletion.CASCADE, related_name='transaction_requests_sent', to=settings.AUTH_USER_MODEL)),
+                ('source', models.ForeignKey(editable=False, on_delete=django.db.models.deletion.CASCADE, related_name='tx_sent', to=settings.AUTH_USER_MODEL)),
                 ('source_sends', models.ForeignKey(editable=False, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='+', to='trading.Commodity')),
             ],
         ),
-        migrations.CreateModel(
-            name='Tx',
-            fields=[
-                ('id', hashid_field.field.HashidAutoField(alphabet='abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890', min_length=7, primary_key=True, serialize=False)),
-                ('instant', models.DateTimeField(auto_now_add=True)),
-                ('amount', models.PositiveIntegerField(editable=False, verbose_name='amount')),
-                ('commodity', models.ForeignKey(editable=False, on_delete=django.db.models.deletion.PROTECT, to='trading.Commodity')),
-                ('dest', models.ForeignKey(editable=False, on_delete=django.db.models.deletion.PROTECT, related_name='credits', related_query_name='credit_tx', to=settings.AUTH_USER_MODEL, verbose_name='destination')),
-                ('request', models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.SET_NULL, to='trading.TxRequest')),
-                ('source', models.ForeignKey(editable=False, on_delete=django.db.models.deletion.PROTECT, related_name='debits', related_query_name='debit_tx', to=settings.AUTH_USER_MODEL, verbose_name='source')),
-            ],
-            options={
-                'verbose_name': 'transaction',
-                'verbose_name_plural': 'transactions',
-            },
-        ),
         migrations.CreateModel(
             name='Invite',
             fields=[

trading/models.py

@@ -3,6 +3,7 @@ from typing import Mapping, Optional, Sequence
 from django.conf import settings
 from django.contrib.auth.models import AbstractBaseUser, PermissionsMixin
 from django.db import models
+from django.db.models import Q
 from django.db.models.signals import post_save, pre_save
 from django.dispatch import receiver
 from django.utils.translation import gettext_lazy as _
@@ -76,38 +77,56 @@ class User(PermissionsMixin, AbstractBaseUser):
     def balances(self) -> Mapping["Commodity", int]:
         from django.db.models import Sum
 
-        credits = {
-            row["commodity"]: row["amount__sum"]
-            for row in self.credits.values("commodity").annotate(Sum("amount"))
+        # We are the source, receiving from the destination
+        credits_sent = {
+            row["dest_sends"]: row["dest_amount__sum"]
+            for row in self.tx_sent.filter(status=TxRequest.ACCEPTED)
+            .values("dest_sends")
+            .annotate(Sum("dest_amount"))
         }
-        debits = {
-            row["commodity"]: row["amount__sum"]
-            for row in self.debits.values("commodity").annotate(Sum("amount"))
+        # We are the destination, accepting from the source
+        credits_received = {
+            row["source_sends"]: row["source_amount__sum"]
+            for row in self.tx_received.filter(status=TxRequest.ACCEPTED)
+            .values("source_sends")
+            .annotate(Sum("source_amount"))
+        }
+        # We are the source, sending to the destination
+        debits_sent = {
+            row["source_sends"]: row["source_amount__sum"]
+            for row in self.tx_sent.filter(~Q(status=TxRequest.DECLINED))
+            .values("source_sends")
+            .annotate(Sum("source_amount"))
+        }
+        # We are the desination, purchasing from the source
+        debits_received = {
+            row["dest_sends"]: row["dest_amount__sum"]
+            for row in self.tx_received.filter(status=TxRequest.ACCEPTED)
+            .values("dest_sends")
+            .annotate(Sum("dest_amount"))
         }
         ipos = {
             row["pk"]: row["in_circulation"]
             for row in self.commodity_set.values("pk", "in_circulation")
         }
-        tx_requests = {
-            row["source_sends"]: row["source_amount__sum"]
-            for row in self.transaction_requests_sent.filter(status=TxRequest.OPEN)
-            .values("source_sends")
-            .annotate(Sum("source_amount"))
-        }
         keys = (
             set(ipos.keys())
-            | set(credits.keys())
-            | set(debits.keys())
-            | set(tx_requests.keys())
+            | set(credits_sent.keys())
+            | set(credits_received.keys())
+            | set(debits_sent.keys())
+            | set(debits_received.keys())
+            # | set(tx_requests.keys())
         )
         return frozendict(
             [
                 (
                     Commodity.objects.get(pk=pk),
-                    credits.get(pk, 0)
+                    credits_sent.get(pk, 0)
+                    + credits_received.get(pk, 0)
                     + ipos.get(pk, 0)
-                    - debits.get(pk, 0)
-                    - tx_requests.get(pk, 0),
+                    - debits_sent.get(pk, 0)
+                    - debits_received.get(pk, 0)
+                    # - tx_requests.get(pk, 0),
                 )
                 for pk in keys
             ]
@@ -179,40 +198,6 @@ class MaxCommodityError(Exception):
         super().__init__("maximum commodity count reached")
 
 
-class Tx(models.Model):
-    """
-    A transaction from one account to another.
-    """
-
-    id = HashidAutoField(primary_key=True)
-    instant = models.DateTimeField(auto_now_add=True, editable=False)
-    source = models.ForeignKey(
-        User,
-        on_delete=models.PROTECT,
-        related_name="debits",
-        related_query_name="debit_tx",
-        verbose_name=_("source"),
-        editable=False,
-    )
-    dest = models.ForeignKey(
-        User,
-        on_delete=models.PROTECT,
-        related_name="credits",
-        related_query_name="credit_tx",
-        verbose_name=_("destination"),
-        editable=False,
-    )
-    amount = models.PositiveIntegerField(verbose_name=_("amount"), editable=False)
-    commodity = models.ForeignKey(Commodity, on_delete=models.PROTECT, editable=False)
-    request = models.ForeignKey(
-        "TxRequest", null=True, default=None, on_delete=models.SET_NULL
-    )
-
-    class Meta:
-        verbose_name = _("transaction")
-        verbose_name_plural = _("transactions")
-
-
 class TxRequest(models.Model):
     """
     A transaction request between two users.
@@ -230,6 +215,7 @@ class TxRequest(models.Model):
         (ACCEPTED, "Accepted"),
     ]
 
+    id = HashidAutoField(primary_key=True)
     status = models.CharField(
         max_length=max([len(c) for c in STATUS_CHOICES]),
         choices=STATUS_CHOICES,
@@ -245,23 +231,25 @@ class TxRequest(models.Model):
     # )
 
     source = models.ForeignKey(
-        User,
-        on_delete=models.CASCADE,
-        related_name="transaction_requests_sent",
-        editable=False,
+        User, on_delete=models.CASCADE, related_name="tx_sent", editable=False,
     )
     dest = models.ForeignKey(
-        User,
-        on_delete=models.CASCADE,
-        related_name="transaction_requests_received",
-        editable=False,
+        User, on_delete=models.CASCADE, related_name="tx_received", editable=False,
     )
 
     source_sends = models.ForeignKey(
-        Commodity, on_delete=models.CASCADE, related_name="+", null=True, editable=False,
+        Commodity,
+        on_delete=models.CASCADE,
+        related_name="+",
+        null=True,
+        editable=False,
     )
     dest_sends = models.ForeignKey(
-        Commodity, on_delete=models.CASCADE, related_name="+", null=True, editable=False,
+        Commodity,
+        on_delete=models.CASCADE,
+        related_name="+",
+        null=True,
+        editable=False,
     )
 
     source_amount = models.PositiveIntegerField(editable=False)
@@ -304,7 +292,6 @@ class TxRequest(models.Model):
         dest_balance = self.dest.balance_of(self.dest_sends)
         return self.status == TxRequest.OPEN and dest_balance >= self.dest_amount
 
-
     def accept(self):
         """
         Accepts an open transaction request.
@@ -320,28 +307,6 @@ class TxRequest(models.Model):
 
         # Update status
         self.status = TxRequest.ACCEPTED
-
-        # Create source transaction
-        source_tx = Tx.objects.create(
-            source=self.source,
-            dest=self.dest,
-            amount=self.source_amount,
-            commodity=self.source_sends,
-            request=self,
-        )
-
-        # Create dest transaction
-        dest_tx = Tx.objects.create(
-            source=self.dest,
-            dest=self.source,
-            amount=self.dest_amount,
-            commodity=self.dest_sends,
-            request=self,
-        )
-
-        # Save everything
-        source_tx.save()
-        dest_tx.save()
         self.save()
 
     def decline(self):
@@ -353,8 +318,6 @@ class TxRequest(models.Model):
         self.save()
 
 
-
-
 class BalanceError(Exception):
     def __init__(
         self, balance: int, source: User, dest: User, amount: int, commodity: Commodity
@@ -367,22 +330,24 @@ class BalanceError(Exception):
         super().__init__("balance too low to create transaction")
 
 
-@receiver(pre_save, sender=Tx)
-def _tx_pre_save(sender, instance, *args, **kwargs):
+@receiver(pre_save, sender=TxRequest)
+def __tx_pre_save(sender, instance, *args, **kwargs):
     """
     Ensures that a transaction cannot be created if the source user doesn't have enough of the given
     commodity in their account.
     """
     if instance.pk is None:
-        balance = instance.source.balance_of(instance.commodity)
-        if instance.amount > balance:
+        source_balance = instance.source.balance_of(instance.source_sends)
+        if instance.source_amount > source_balance:
             raise BalanceError(
-                balance,
+                source_balance,
                 instance.source,
                 instance.dest,
-                instance.amount,
-                instance.commodity,
+                instance.source_amount,
+                instance.source_sends,
             )
+        # Do not check dest balance - could be a side channel for determining another account's
+        # funds which is not something we want
 
 
 @receiver(post_save, sender=TxRequest)

trading/templates/trading/t/detail.html

@@ -14,19 +14,14 @@
             <table class="table">
                 <thead>
                     <tr>
-                        <th>Commodity</th>
-                        <th>Delta</th>
+                        <th>Transaction ID</th>
+                        <th>Commodity sent</th>
+                        <th>Commodity received</th>
+                        <th>Status</th>
                     </tr>
                 </thead>
                 <tbody>
-                    <tr>
-                        <td><a href="{% url "trading:commodity_detail" object.commodity.pk %}">{{ object.commodity.name }}</a></td>
-                        <td>
-                        <span class="{% if object.source == request.user %}debit{% else %}credit{% endif %}">
-                            {{object.amount|intcomma}}
-                        </span>
-                        </td>
-                    </tr>
+                    {% include "trading/t/detail_row.html" %}
                 </tbody>
             </table>
         </div>

trading/templates/trading/t/detail_row.html

@@ -0,0 +1,12 @@
+<tr>
+    <td><a href="{% url "trading:tx_detail" object.id %}">{{object.id}}</a></td>
+    {% if object.source == request.user %}
+        <td>{{object.source_amount}}x {{object.source_sends.name}}</td>
+        <td>{{object.dest_amount}}x {{object.dest_sends.name}}</td>
+    {% else %}
+        <td>{{object.dest_amount}}x {{object.dest_sends.name}}</td>
+        <td>{{object.source_amount}}x {{object.source_sends.name}}</td>
+    {% endif %}
+    <td>{{object.status | title}}</td>
+</tr>
+

trading/templates/trading/t/list.html

@@ -14,26 +14,15 @@
             <table class="table">
                 <thead>
                     <tr>
-                        <th>Commodity</th>
-                        <th>Delta</th>
+                        <th>Transaction ID</th>
+                        <th>Commodity sent</th>
+                        <th>Commodity received</th>
                         <th>Status</th>
-                        <th>Source</th>
-                        <th>Destination</th>
                     </tr>
                 </thead>
                 <tbody>
                     {% for object in object_list %}
-                        <tr>
-                            <td><a href="{% url "trading:commodity_detail" object.commodity.pk %}">{{ object.commodity.name }}</a></td>
-                            <td>
-                            <span class="{% if object.source == request.user %}debit{% else %}credit{% endif %}">
-                                {{object.amount|intcomma}}
-                            </span>
-                            </td>
-                            <td>{{object.status}}</td>
-                            <td>{{object.source.username}}</td>
-                            <td>{{object.dest.username}}</td>
-                        </tr>
+                        {% include "trading/t/detail_row.html" %}
                     {% endfor %}
                 </tbody>
             </table>

trading/tests/test_models.py

@@ -1,38 +1,8 @@
 from django.test import TestCase
-from trading.models import User, Commodity, Tx, TxRequest, BalanceError
+from trading.models import User, Commodity, TxRequest, BalanceError
 
 
 class TestTransactions(TestCase):
-    def test_user_balances(self):
-        # Create users
-        user1 = User.objects.create_user(username="test1", email="test1@test.test")
-        user2 = User.objects.create_user(username="test2", email="test2@test.test")
-
-        # Create test commodity
-        commodity = Commodity(created_by=user1, in_circulation=1000, name="commodity")
-        commodity.save()
-
-        # Confirm balances
-        self.assertEqual(user1.balances(), {commodity: 1000})
-        self.assertEqual(user1.balance_of(commodity), 1000)
-        self.assertEqual(user2.balances(), {})
-        self.assertEqual(user2.balance_of(commodity), 0)
-
-        # Create transaction
-        Tx(source=user1, dest=user2, amount=100, commodity=commodity).save()
-
-        # Confirm balances
-        self.assertEqual(user1.balances(), {commodity: 900})
-        self.assertEqual(user2.balances(), {commodity: 100})
-
-        # Ensure we can't trade when there's missing balance
-        with self.assertRaises(BalanceError):
-            Tx(source=user2, dest=user1, amount=1000, commodity=commodity).save()
-
-        # Confirm balances
-        self.assertEqual(user1.balances(), {commodity: 900})
-        self.assertEqual(user2.balances(), {commodity: 100})
-
     def test_tx_requests(self):
         # Create users
         user1 = User.objects.create_user(username="test1", email="test1@test.test")

trading/views.py

@@ -12,7 +12,7 @@ from django.views.generic.edit import CreateView, UpdateView
 from django.views.generic.list import ListView
 from django.urls import reverse, reverse_lazy
 from django.utils.translation import gettext_lazy as _
-from trading.models import User, Commodity, MaxCommodityError, Tx, TxRequest
+from trading.models import User, Commodity, MaxCommodityError, TxRequest
 
 
 class IndexView(LoginRequiredMixin, TemplateView):
@@ -57,6 +57,21 @@ class TxRequestDetailView(DetailView):
     template_name = "trading/t/detail.html"
     model = TxRequest
 
+    def get_context_data(self, **kwargs):
+        context = super().get_context_data(**kwargs)
+        tx = context["object"]
+        if self.request.user == tx.source:
+            context["sent"] = tx.source_sends
+            context["sent_amount"] = tx.source_amount
+            context["received"] = tx.dest_sends
+            context["received_amount"] = tx.dest_amount
+        else:
+            context["sent"] = tx.dest_sends
+            context["sent_amount"] = tx.dest_amount
+            context["received"] = tx.source_sends
+            context["received_amount"] = tx.source_amount
+        return context
+
 
 class TxRequestListView(LoginRequiredMixin, ListView):
     template_name = "trading/t/list.html"