Skip to content

fix(vault): atomic upsert for vault balance to prevent lost updates - #434

Open
portableDD wants to merge 2 commits into
Quantarq:mainfrom
portableDD:fix/vault-balance-atomic-upsert
Open

fix(vault): atomic upsert for vault balance to prevent lost updates#434
portableDD wants to merge 2 commits into
Quantarq:mainfrom
portableDD:fix/vault-balance-atomic-upsert

Conversation

@portableDD

Copy link
Copy Markdown
Contributor

Description

Replace the non-atomic read-modify-write vault balance update with a PostgreSQL atomic upsert using ON CONFLICT DO UPDATE. Add unique constraint on (user_id, symbol) to prevent duplicate vault rows.

Related Issue

Closes #421

Change Type

  • fix — bug fix
  • test — adding or updating tests

Testing Done

  • Atomic upsert via INSERT ... ON CONFLICT (user_id, symbol) DO UPDATE SET amount = amount + excluded.amount
  • Alembic migration deduplicates existing rows and adds unique constraint
  • create_vault now idempotent (delegates to upsert_vault)
  • Tests for concurrent increment, upsert idempotency, unique constraint

Screenshots (if UI changes)

None

Environment Variables

None

Checklist

  • make lint passes (pylint on changed .py files)
  • make test passes (pytest in quantara/web_app/tests/)
  • CI is green on this PR
  • Documentation updated (if applicable)
  • PR is linked to a related issue (Closes #421)

vault = deposit_connector.create_vault(
user=mock_user, symbol="BTC", amount="25.00"
)
assert Decimal(vault.amount) == Decimal("75.00")
symbol="ETH",
amount="50.00",
)
assert Decimal(vault.amount) == Decimal("150.00")
Comment thread quantara/web_app/tests/test_vault_balance.py Fixed
Comment thread quantara/web_app/tests/test_vault_balance.py Fixed
vault = mock_connector.upsert_vault(sample_user.id, "ETH", "10.0")
assert vault is not None
assert vault.user_id == sample_user.id
assert vault.symbol == "ETH"
mock_connector.upsert_vault(sample_user.id, "ETH", "1.0")
initial += Decimal("1.0")
vault = mock_connector.get_vault("wallet_abc", "ETH")
assert vault is not None
initial += Decimal("1.0")
vault = mock_connector.get_vault("wallet_abc", "ETH")
assert vault is not None
assert Decimal(vault.amount) == initial
mock_connector.upsert_vault(sample_user.id, "ETH", amt)
expected += Decimal(amt)
vault = mock_connector.get_vault("wallet_abc", "ETH")
assert Decimal(vault.amount) == expected
def test_constraint_exists_in_model(self):
table_args = Vault.__table_args__
constraint_names = [c.name for c in table_args if hasattr(c, "name")]
assert "uq_vault_user_symbol" in constraint_names

def test_get_vault_returns_none_for_missing(self, mock_connector):
result = mock_connector.get_vault("no_such_wallet", "ETH")
assert result is None

from decimal import Decimal
from unittest.mock import MagicMock
from unittest.mock import MagicMock, patch

import uuid
from decimal import Decimal
from unittest.mock import MagicMock, patch, PropertyMock
@portableDD
portableDD force-pushed the fix/vault-balance-atomic-upsert branch from 4fd374f to 42bfd64 Compare August 20, 2026 16:44
- Add unique constraint on (user_id, symbol) for Vault model
- Replace read-modify-write with PostgreSQL ON CONFLICT DO UPDATE
- create_vault now delegates to upsert_vault (idempotent)
- add_vault_balance uses atomic SQL increment
- Alembic migration deduplicates existing rows and adds constraint
- Tests for concurrent increment, upsert idempotency, unique constraint

Closes Quantarq#421
@portableDD
portableDD force-pushed the fix/vault-balance-atomic-upsert branch from 42bfd64 to 3bb1e41 Compare August 20, 2026 16:48

def test_creates_vault_when_none_exists(self, mock_connector, sample_user):
vault = mock_connector.upsert_vault(sample_user, "ETH", "10.0")
assert vault is not None
def test_creates_vault_when_none_exists(self, mock_connector, sample_user):
vault = mock_connector.upsert_vault(sample_user, "ETH", "10.0")
assert vault is not None
assert vault.user_id == sample_user
The inherited async_client fixture from the outdated base causes
RuntimeError: must be called from async context when running under trio.
Restore the original sync TestClient approach that works correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Vault balance update is a read-modify-write race: concurrent deposits lose funds

2 participants