Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove price calculation #1762

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Remove price calculation #1762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e2fb35a
Remove price calculation
moisses89 Nov 16, 2023
dacb44b
Remove price clients
moisses89 Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions config/settings/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,10 +496,6 @@
"TOKENS_ERC20_GET_BALANCES_BATCH", default=2_000
) # Number of tokens to get balances from in the same request. From 2_500 some nodes raise HTTP 413

TOKEN_ETH_PRICE_TTL = env.int(
"TOKEN_ETH_PRICE_TTL", default=60 * 30 # 30 minutes
) # Expiration time for token eth price

# Notifications
# ------------------------------------------------------------------------------
SLACK_API_WEBHOOK = env("SLACK_API_WEBHOOK", default=None)
Expand Down
55 changes: 14 additions & 41 deletions safe_transaction_service/history/services/balance_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import operator
from dataclasses import dataclass
from datetime import datetime
from enum import Enum
from typing import List, Optional, Sequence

from django.conf import settings
Expand All @@ -16,13 +17,7 @@
from gnosis.eth import EthereumClient, EthereumClientProvider
from gnosis.eth.utils import fast_is_checksum_address

from safe_transaction_service.tokens.clients import CannotGetPrice
from safe_transaction_service.tokens.models import Token
from safe_transaction_service.tokens.services.price_service import (
FiatCode,
PriceService,
PriceServiceProvider,
)
from safe_transaction_service.utils.redis import get_redis
from safe_transaction_service.utils.utils import chunks

Expand Down Expand Up @@ -72,6 +67,11 @@ def get_price_address(self) -> ChecksumAddress:
return self.token_address


class FiatCode(Enum):
USD = 1
EUR = 2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this Enum class while we keep balance with price. I think that we should do this in other issue because this needs to be coordinated with clients, for example safe-client-gateway.



@dataclass
class BalanceWithFiat(Balance):
eth_value: float # Value in ether
Expand All @@ -84,9 +84,7 @@ class BalanceWithFiat(Balance):
class BalanceServiceProvider:
def __new__(cls):
if not hasattr(cls, "instance"):
cls.instance = BalanceService(
EthereumClientProvider(), PriceServiceProvider(), get_redis()
)
cls.instance = BalanceService(EthereumClientProvider(), get_redis())
return cls.instance

@classmethod
Expand All @@ -96,12 +94,9 @@ def del_singleton(cls):


class BalanceService:
def __init__(
self, ethereum_client: EthereumClient, price_service: PriceService, redis: Redis
):
def __init__(self, ethereum_client: EthereumClient, redis: Redis):
self.ethereum_client = ethereum_client
self.ethereum_network = self.ethereum_client.get_network()
self.price_service = price_service
self.redis = redis
self.cache_token_info = TTLCache(
maxsize=4096, ttl=60 * 30
Expand Down Expand Up @@ -268,50 +263,28 @@ def get_usd_balances(
exclude_spam: bool = False,
) -> List[BalanceWithFiat]:
"""
All this could be more optimal (e.g. batching requests), but as everything is cached
I think we should be alright
NOTE: PriceService was removed, this function return balances with price 0.
:param safe_address:
:param only_trusted: If True, return balance only for trusted tokens
:param exclude_spam: If True, exclude spam tokens
:return: List of BalanceWithFiat
"""
# TODO Use price service get_token_cached_usd_values
balances: List[Balance] = self.get_balances(
safe_address, only_trusted, exclude_spam
)
try:
eth_price = self.price_service.get_native_coin_usd_price()
except CannotGetPrice:
logger.warning("Cannot get network ether price", exc_info=True)
eth_price = 0
balances_with_usd = []
price_token_addresses = [balance.get_price_address() for balance in balances]
token_eth_values_with_timestamp = (
self.price_service.get_token_cached_eth_values(price_token_addresses)
)
for balance, token_eth_value_with_timestamp in zip(
balances, token_eth_values_with_timestamp
):
token_eth_value = token_eth_value_with_timestamp.eth_value
token_address = balance.token_address
if not token_address: # Ether
fiat_conversion = eth_price
fiat_balance = fiat_conversion * (balance.balance / 10**18)
else:
fiat_conversion = eth_price * token_eth_value
balance_with_decimals = balance.balance / 10**balance.token.decimals
fiat_balance = fiat_conversion * balance_with_decimals

for balance in balances:
balances_with_usd.append(
BalanceWithFiat(
balance.token_address,
balance.token,
balance.balance,
token_eth_value,
token_eth_value_with_timestamp.timestamp,
round(fiat_balance, 4),
round(fiat_conversion, 4),
0.0,
datetime.utcfromtimestamp(0),
0.0,
0.0,
FiatCode.USD.name,
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hectorgomezv just to inform you, the transaction service will return eth_value as 0, timestamp when was calculated as 1970-01... (epoch 0), fiat_balance 0 and fiat_conversion as 0.
This is affecting the endpoint /v1/safes/{address}/balances/usd/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @moisses89! No problem at all from the CGW perspective, the service is not using that endpoint anymore.

)
Expand Down
118 changes: 42 additions & 76 deletions safe_transaction_service/history/tests/test_balance_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
from typing import Optional
from unittest import mock
from unittest.mock import MagicMock
import datetime

from django.test import TestCase
from django.utils import timezone

from eth_account import Account
from eth_typing import ChecksumAddress

from gnosis.eth.tests.ethereum_test_case import EthereumTestCaseMixin
from gnosis.eth.tests.utils import deploy_erc20

from safe_transaction_service.tokens.models import Token
from safe_transaction_service.tokens.services.price_service import PriceService
from safe_transaction_service.tokens.tests.factories import TokenFactory

from ..services import BalanceServiceProvider
Expand Down Expand Up @@ -41,18 +36,8 @@ def test_get_token_info(self):
self.assertEqual(token_info.symbol, token_db.symbol)
self.assertEqual(token_info.decimals, token_db.decimals)

@mock.patch.object(
PriceService, "get_token_eth_value", return_value=0.4, autospec=True
)
@mock.patch.object(
PriceService, "get_native_coin_usd_price", return_value=123.4, autospec=True
)
@mock.patch.object(timezone, "now", return_value=timezone.now())
def test_get_usd_balances(
self,
timezone_now_mock: MagicMock,
get_native_coin_usd_price_mock: MagicMock,
get_token_eth_value_mock: MagicMock,
):
balance_service = self.balance_service

Expand Down Expand Up @@ -85,16 +70,22 @@ def test_get_usd_balances(
balances,
[
BalanceWithFiat(
None, None, value, 1.0, timezone_now_mock.return_value, 0.0, 123.4
None,
None,
value,
0.0,
datetime.datetime.utcfromtimestamp(0),
0.0,
0.0,
),
BalanceWithFiat(
erc20.address,
token_info,
tokens_value,
0.4,
timezone_now_mock.return_value,
round(123.4 * 0.4 * (tokens_value / 1e18), 4),
round(123.4 * 0.4, 4),
0.0,
datetime.datetime.utcfromtimestamp(0),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From clients atre expected a date, how we cannot say when was calculated I decided to put epoch 0.

0.0,
0.0,
),
],
)
Expand All @@ -104,7 +95,7 @@ def test_get_usd_balances(
balances,
[
BalanceWithFiat(
None, None, value, 1.0, timezone_now_mock.return_value, 0.0, 123.4
None, None, value, 0.0, datetime.datetime.utcfromtimestamp(0), 0, 0
),
],
)
Expand All @@ -115,16 +106,16 @@ def test_get_usd_balances(
balances,
[
BalanceWithFiat(
None, None, value, 1.0, timezone_now_mock.return_value, 0.0, 123.4
None, None, value, 0.0, datetime.datetime.utcfromtimestamp(0), 0, 0
),
BalanceWithFiat(
erc20.address,
token_info,
tokens_value,
0.4,
timezone_now_mock.return_value,
round(123.4 * 0.4 * (tokens_value / 1e18), 4),
round(123.4 * 0.4, 4),
0,
datetime.datetime.utcfromtimestamp(0),
0,
0,
),
],
)
Expand Down Expand Up @@ -167,57 +158,46 @@ def test_get_usd_balances(
None,
None,
value,
1.0,
timezone_now_mock.return_value,
0.0,
123.4,
datetime.datetime.utcfromtimestamp(0),
0.0,
0.0,
),
BalanceWithFiat(
erc20_3.address,
token_info_3,
tokens_value,
0.4,
timezone_now_mock.return_value,
round(123.4 * 0.4 * (tokens_value / 1e18), 4),
round(123.4 * 0.4, 4),
0.0,
datetime.datetime.utcfromtimestamp(0),
0.0,
0.0,
),
BalanceWithFiat(
erc20.address,
token_info,
tokens_value,
0.4,
timezone_now_mock.return_value,
round(123.4 * 0.4 * (tokens_value / 1e18), 4),
round(123.4 * 0.4, 4),
0.0,
datetime.datetime.utcfromtimestamp(0),
0.0,
0.0,
),
BalanceWithFiat(
erc20_2.address,
token_info_2,
tokens_value,
0.4,
timezone_now_mock.return_value,
round(123.4 * 0.4 * (tokens_value / 1e18), 4),
round(123.4 * 0.4, 4),
0.0,
datetime.datetime.utcfromtimestamp(0),
0.0,
0.0,
),
],
)

@mock.patch.object(
PriceService, "get_token_eth_value", return_value=0.4, autospec=True
)
@mock.patch.object(
PriceService, "get_native_coin_usd_price", return_value=123.4, autospec=True
)
@mock.patch.object(timezone, "now", return_value=timezone.now())
def test_get_usd_balances_copy_price(
self,
timezone_now_mock: MagicMock,
get_native_coin_usd_price_mock: MagicMock,
get_token_eth_value_mock: MagicMock,
):
def test_get_usd_balances_copy_price(self):
balance_service = self.balance_service
safe_address = SafeContractFactory().address
random_address = Account.create().address
timestamp_str = "1970-01-01T00:00:00Z"

balances = balance_service.get_usd_balances(safe_address)
self.assertEqual(len(balances), 1)
Expand All @@ -235,16 +215,7 @@ def test_get_usd_balances_copy_price(
)
ERC20TransferFactory(address=erc20.address, to=safe_address)

def get_token_eth_value(
self, token_address: ChecksumAddress
) -> Optional[float]:
if token_address == erc20.address:
return 0.4
elif token_address == random_address:
return 0.1

get_token_eth_value_mock.side_effect = get_token_eth_value
for expected_token_eth_value in (0.4, 0.1):
for expected_token_eth_value in (0, 0):
with self.subTest(expected_token_eth_value=expected_token_eth_value):
balances = balance_service.get_usd_balances(safe_address)
self.assertEqual(len(balances), 2)
Expand All @@ -255,24 +226,19 @@ def get_token_eth_value(
None,
None,
0,
1.0,
timezone_now_mock.return_value,
expected_token_eth_value,
datetime.datetime.utcfromtimestamp(0),
0.0,
0.0,
123.4,
),
BalanceWithFiat(
erc20.address,
balance_service.get_token_info(erc20.address),
tokens_value,
expected_token_eth_value,
timezone_now_mock.return_value,
round(
123.4
* expected_token_eth_value
* (tokens_value / 1e18),
4,
),
round(123.4 * expected_token_eth_value, 4),
datetime.datetime.utcfromtimestamp(0),
0.0,
0.0,
),
],
)
Expand Down
Loading