From c320f81181413d0bfb293c8f01250af8e4323f69 Mon Sep 17 00:00:00 2001 From: Alek Ratzloff Date: Thu, 26 Mar 2020 12:49:46 -0400 Subject: [PATCH] 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 --- trading/migrations/0001_initial.py | 24 +-- trading/models.py | 153 ++++++++------------ trading/templates/trading/t/detail.html | 15 +- trading/templates/trading/t/detail_row.html | 12 ++ trading/templates/trading/t/list.html | 19 +-- trading/tests/test_models.py | 32 +--- trading/views.py | 17 ++- 7 files changed, 101 insertions(+), 171 deletions(-) create mode 100644 trading/templates/trading/t/detail_row.html diff --git a/trading/migrations/0001_initial.py b/trading/migrations/0001_initial.py index d654117..e2e6ea2 100644 --- a/trading/migrations/0001_initial.py +++ b/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=[ diff --git a/trading/models.py b/trading/models.py index f274ff6..efdf61c 100644 --- a/trading/models.py +++ b/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) diff --git a/trading/templates/trading/t/detail.html b/trading/templates/trading/t/detail.html index a0aec43..eebee8b 100644 --- a/trading/templates/trading/t/detail.html +++ b/trading/templates/trading/t/detail.html @@ -14,19 +14,14 @@ - - + + + + - - - - + {% include "trading/t/detail_row.html" %}
CommodityDeltaTransaction IDCommodity sentCommodity receivedStatus
{{ object.commodity.name }} - - {{object.amount|intcomma}} - -
diff --git a/trading/templates/trading/t/detail_row.html b/trading/templates/trading/t/detail_row.html new file mode 100644 index 0000000..8931655 --- /dev/null +++ b/trading/templates/trading/t/detail_row.html @@ -0,0 +1,12 @@ + + {{object.id}} + {% if object.source == request.user %} + {{object.source_amount}}x {{object.source_sends.name}} + {{object.dest_amount}}x {{object.dest_sends.name}} + {% else %} + {{object.dest_amount}}x {{object.dest_sends.name}} + {{object.source_amount}}x {{object.source_sends.name}} + {% endif %} + {{object.status | title}} + + diff --git a/trading/templates/trading/t/list.html b/trading/templates/trading/t/list.html index d48ec91..0a43a96 100644 --- a/trading/templates/trading/t/list.html +++ b/trading/templates/trading/t/list.html @@ -14,26 +14,15 @@ - - + + + - - {% for object in object_list %} - - - - - - - + {% include "trading/t/detail_row.html" %} {% endfor %}
CommodityDeltaTransaction IDCommodity sentCommodity received StatusSourceDestination
{{ object.commodity.name }} - - {{object.amount|intcomma}} - - {{object.status}}{{object.source.username}}{{object.dest.username}}
diff --git a/trading/tests/test_models.py b/trading/tests/test_models.py index 6d49ded..5d26f2b 100644 --- a/trading/tests/test_models.py +++ b/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") diff --git a/trading/views.py b/trading/views.py index 7c383c2..abb92c5 100644 --- a/trading/views.py +++ b/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"