From b6b9927722c14792488e0c6a11ef0414deaf7f05 Mon Sep 17 00:00:00 2001 From: Felix Henneke Date: Wed, 6 Nov 2024 08:58:30 +0100 Subject: [PATCH] [Easy] Remove option to consolidate transfers (#416) This PR removes a command line argument to merge transfers and all code associated to it. With our current code, switching on this option would result in transfers merging transfers which have not been merged on purpose. Examples for this are transfers for quote and batch rewards, as well as transfers for protocol fees and partner fees. Removing the option makes it more difficult to use the script in a wrong way. At the end of the restructuring of the solver accounting, we might also want to merge transfers again. This will, however, not be compatible with the simple logic currently implemented. Thus, keeping the current code has little benefits. Removing the command line option allows to also remove the associated code and tests. This makes the rest of the code easier to change. This change is a breaking change of the command line API. We currently do not use the option anywhere, so the change should not affect current operations. --- src/fetch/transfer_file.py | 3 - src/models/transfer.py | 67 ------- src/utils/script_args.py | 9 - tests/unit/test_models.py | 348 ------------------------------------- 4 files changed, 427 deletions(-) diff --git a/src/fetch/transfer_file.py b/src/fetch/transfer_file.py index dcf776e8..96aaf0c6 100644 --- a/src/fetch/transfer_file.py +++ b/src/fetch/transfer_file.py @@ -125,9 +125,6 @@ def auto_propose( if tr.amount_wei >= args.min_transfer_amount_cow_atoms: payout_transfers.append(tr) - if args.consolidate_transfers: - payout_transfers = Transfer.consolidate(payout_transfers) - if args.post_tx: ssl_context = ssl.create_default_context(cafile=certifi.where()) ssl_context.verify_mode = ssl.CERT_REQUIRED diff --git a/src/models/transfer.py b/src/models/transfer.py index 48de2164..9bcde752 100644 --- a/src/models/transfer.py +++ b/src/models/transfer.py @@ -7,8 +7,6 @@ from dataclasses import dataclass from typing import Optional -import pandas as pd - from dune_client.types import Address from eth_typing.encoding import HexStr from gnosis.safe.multi_send import MultiSendOperation, MultiSendTx @@ -58,28 +56,6 @@ def __init__(self, token: Optional[Token], recipient: Address, amount_wei: int): self._recipient = recipient self.amount_wei = amount_wei - @classmethod - def from_dict(cls, obj: dict[str, str]) -> Transfer: - """Converts Dune data dict to object with types""" - token_address = obj.get("token_address", None) - return cls( - token=Token(token_address) if token_address else None, - recipient=Address(obj["receiver"]), - amount_wei=int(obj["amount"]), - ) - - @classmethod - def from_dataframe(cls, pdf: pd.DataFrame) -> list[Transfer]: - """Converts Pandas Dataframe into list of Transfers""" - return [ - cls( - token=Token(row["token_address"]) if row["token_address"] else None, - recipient=Address(row["receiver"]), - amount_wei=int(row["amount"]), - ) - for _, row in pdf.iterrows() - ] - @staticmethod def summarize(transfers: list[Transfer]) -> str: """Summarizes transfers with totals""" @@ -99,26 +75,6 @@ def recipient(self) -> Address: """Read access to the recipient of a transfer""" return self._recipient - @staticmethod - def consolidate(transfer_list: list[Transfer]) -> list[Transfer]: - """ - Removes redundancy of a transfer list by consolidating _duplicate transfers_. - Duplicates defined as transferring the same token to one recipient multiple times. - This optimizes gas cost of multiple transfers. - """ - - transfer_dict: dict[tuple, Transfer] = {} - for transfer in transfer_list: - key = (transfer.recipient, transfer.token) - if key in transfer_dict: - transfer_dict[key] = transfer_dict[key].merge(transfer) - else: - transfer_dict[key] = transfer - return sorted( - transfer_dict.values(), - key=lambda t: (-t.amount, t.recipient, t.token), - ) - @property def token_type(self) -> TokenType: """Returns the type of transfer (Native or ERC20)""" @@ -135,29 +91,6 @@ def amount(self) -> float: assert self.token is not None return self.amount_wei / int(10**self.token.decimals) - def merge(self, other: Transfer) -> Transfer: - """ - Merge two transfers (acts like addition) if all fields except amount are equal, - returns a transfer who amount is the sum. - Merges incur information loss (particularly in the category of redirects). - Note that two transfers of the same token with different receivers, - but redirected to the same address, will get merged and original receivers will be forgotten - """ - merge_requirements = [ - self.recipient == other.recipient, - self.token == other.token, - ] - if all(merge_requirements): - return Transfer( - token=self.token, - recipient=self.recipient, - amount_wei=self.amount_wei + other.amount_wei, - ) - raise ValueError( - f"Can't merge transfers {self}, {other}. " - f"Requirements met {merge_requirements}" - ) - def as_multisend_tx(self) -> MultiSendTx: """Converts Transfer into encoded MultiSendTx bytes""" receiver = Web3.to_checksum_address(self.recipient.address) diff --git a/src/utils/script_args.py b/src/utils/script_args.py index 1b582a30..a00933af 100644 --- a/src/utils/script_args.py +++ b/src/utils/script_args.py @@ -18,7 +18,6 @@ class ScriptArgs: dune: DuneFetcher post_tx: bool dry_run: bool - consolidate_transfers: bool min_transfer_amount_wei: int min_transfer_amount_cow_atoms: int ignore_slippage: bool @@ -44,13 +43,6 @@ def generic_script_init(description: str) -> ScriptArgs: "(requires valid env var `PROPOSER_PK`)", default=False, ) - parser.add_argument( - "--consolidate-transfers", - type=bool, - help="Flag to indicate whether payout transfer file should be optimized " - "(i.e. squash transfers having same receiver-token pair) ", - default=False, - ) parser.add_argument( "--dry-run", type=bool, @@ -85,7 +77,6 @@ def generic_script_init(description: str) -> ScriptArgs: ), post_tx=args.post_tx, dry_run=args.dry_run, - consolidate_transfers=args.consolidate_transfers, min_transfer_amount_wei=args.min_transfer_amount_wei, min_transfer_amount_cow_atoms=args.min_transfer_amount_cow_atoms, ignore_slippage=args.ignore_slippage, diff --git a/tests/unit/test_models.py b/tests/unit/test_models.py index f8be7463..070cf46f 100644 --- a/tests/unit/test_models.py +++ b/tests/unit/test_models.py @@ -22,354 +22,6 @@ def setUp(self) -> None: self.token_1 = Token(Address.from_int(1), 18) self.token_2 = Token(Address.from_int(2), 18) - def test_basic_consolidation(self): - recipients = [ - Address.from_int(0), - Address.from_int(1), - ] - tokens = [ - Token(Address.from_int(2), 18), - Token(Address.from_int(3), 18), - ] - transfer_list = [ - Transfer( - token=tokens[0], - recipient=recipients[0], - amount_wei=1 * ONE_ETH, - ), - Transfer( - token=tokens[0], - recipient=recipients[0], - amount_wei=2 * ONE_ETH, - ), - Transfer( - token=tokens[1], - recipient=recipients[0], - amount_wei=3 * ONE_ETH, - ), - Transfer( - token=tokens[0], - recipient=recipients[1], - amount_wei=4 * ONE_ETH, - ), - Transfer( - token=None, - recipient=recipients[0], - amount_wei=5 * ONE_ETH, - ), - Transfer( - token=None, - recipient=recipients[0], - amount_wei=6 * ONE_ETH, - ), - Transfer( - token=None, - recipient=recipients[1], - amount_wei=7 * ONE_ETH, - ), - Transfer( - token=None, - recipient=recipients[1], - amount_wei=8 * ONE_ETH, - ), - ] - - expected = [ - Transfer( - token=None, - recipient=recipients[1], - amount_wei=15 * ONE_ETH, - ), - Transfer( - token=None, - recipient=recipients[0], - amount_wei=11 * ONE_ETH, - ), - Transfer( - token=tokens[0], - recipient=recipients[1], - amount_wei=4 * ONE_ETH, - ), - Transfer( - token=tokens[0], - recipient=recipients[0], - amount_wei=3 * ONE_ETH, - ), - Transfer( - token=tokens[1], - recipient=recipients[0], - amount_wei=3 * ONE_ETH, - ), - ] - self.assertEqual(expected, Transfer.consolidate(transfer_list)) - - def test_consolidation_with_redirect(self): - receiver = Address.from_int(0) - redirect = Address.from_int(1) - - transfer_list = [ - Transfer( - token=None, - recipient=receiver, - amount_wei=1 * ONE_ETH, - ), - Transfer( - token=None, - recipient=receiver, - amount_wei=2 * ONE_ETH, - ), - redirected_transfer( - token=None, - recipient=receiver, - amount_wei=3 * ONE_ETH, - redirect=redirect, - ), - ] - - expected = [ - Transfer( - token=None, - recipient=receiver, - amount_wei=3 * ONE_ETH, - ), - redirected_transfer( - token=None, - recipient=receiver, - amount_wei=3 * ONE_ETH, - redirect=redirect, - ), - ] - results = Transfer.consolidate(transfer_list) - self.assertEqual(expected, results) - - transfer_list = [ - Transfer( - token=None, - recipient=receiver, - amount_wei=1 * ONE_ETH, - ), - redirected_transfer( - token=None, - recipient=receiver, - amount_wei=2 * ONE_ETH, - redirect=redirect, - ), - redirected_transfer( - token=None, - recipient=receiver, - amount_wei=3 * ONE_ETH, - redirect=redirect, - ), - ] - expected = [ - Transfer( - token=None, - recipient=redirect, - amount_wei=5 * ONE_ETH, - ), - Transfer( - token=None, - recipient=receiver, - amount_wei=1 * ONE_ETH, - ), - ] - results = Transfer.consolidate(transfer_list) - self.assertEqual(expected, results) - - def test_basic_merge_without_redirects(self): - receiver = Address.zero() - # Native Transfer Merge - native_transfer1 = Transfer( - token=None, - recipient=receiver, - amount_wei=ONE_ETH, - ) - native_transfer2 = Transfer( - token=None, - recipient=receiver, - amount_wei=ONE_ETH, - ) - self.assertEqual( - native_transfer1.merge(native_transfer2), - Transfer( - token=None, - recipient=receiver, - amount_wei=2 * ONE_ETH, - ), - ) - # ERC20 Transfer Merge - erc20_transfer1 = Transfer( - token=self.token_1, - recipient=receiver, - amount_wei=ONE_ETH, - ) - erc20_transfer2 = Transfer( - token=self.token_1, - recipient=receiver, - amount_wei=ONE_ETH, - ) - self.assertEqual( - erc20_transfer1.merge(erc20_transfer2), - Transfer( - token=self.token_1, - recipient=receiver, - amount_wei=2 * ONE_ETH, - ), - ) - - with self.assertRaises(ValueError) as err: - native_transfer1.merge(erc20_transfer1) - self.assertEqual( - f"Can't merge transfers {native_transfer1}, {erc20_transfer1}. " - f"Requirements met [True, False]", - str(err.exception), - ) - - with self.assertRaises(ValueError) as err: - # Different recipients - t1 = Transfer( - token=self.token_1, - recipient=Address.from_int(1), - amount_wei=2 * ONE_ETH, - ) - t2 = Transfer( - token=self.token_1, - recipient=Address.from_int(2), - amount_wei=2 * ONE_ETH, - ) - t1.merge(t2) - self.assertEqual( - f"Can't merge transfers {t1}, {t2}. Requirements met [False, True]", - str(err.exception), - ) - - with self.assertRaises(ValueError) as err: - # Different Token Addresses - t1 = Transfer( - token=self.token_1, - recipient=receiver, - amount_wei=2 * ONE_ETH, - ) - t2 = Transfer( - token=self.token_2, - recipient=receiver, - amount_wei=2 * ONE_ETH, - ) - t1.merge(t2) - self.assertEqual( - f"Can't merge transfers {t1}, {t2}. Requirements met [True, False]", - str(err.exception), - ) - - def test_merge_with_redirects(self): - receiver_1 = Address.from_int(1) - receiver_2 = Address.from_int(2) - redirect = Address.from_int(3) - - transfer = redirected_transfer( - token=None, recipient=receiver_1, amount_wei=ONE_ETH, redirect=redirect - ) - expected = Transfer( - token=None, - recipient=redirect, - amount_wei=2 * ONE_ETH, - ) - self.assertEqual( - transfer.merge( - redirected_transfer( - token=None, - recipient=receiver_2, - amount_wei=ONE_ETH, - redirect=redirect, - ) - ), - # Both redirected to same address get merged. - expected, - ) - self.assertEqual( - transfer.merge( - Transfer( - token=None, - recipient=redirect, - amount_wei=ONE_ETH, - ) - ), - # one redirected and the other with initial recipient as redirect address get merged. - expected, - ) - - with self.assertRaises(ValueError) as err: - # Fail to merge redirected transfer with one whose recipient - # is the original receiver of the redirected transfer - other = Transfer( - token=None, - recipient=receiver_1, - amount_wei=ONE_ETH, - ) - transfer.merge(other) - self.assertEqual( - f"Can't merge transfers {transfer}, {other}. Requirements met [False, True]", - str(err.exception), - ) - - def test_from_dict(self): - receiver = Address.from_int(1) - self.assertEqual( - Transfer.from_dict( - { - "token_address": None, - "receiver": receiver.address, - "amount": "1234000000000000000", - } - ), - Transfer( - token=None, - recipient=receiver, - amount_wei=1234 * 10**15, - ), - ) - - self.assertEqual( - Transfer.from_dict( - { - "token_address": COW_TOKEN_ADDRESS.address, - "receiver": Address.from_int(1).address, - "amount": "1234000000000000000", - } - ), - Transfer( - token=Token(COW_TOKEN_ADDRESS), - recipient=Address.from_int(1), - amount_wei=1234 * 10**15, - ), - ) - - def test_from_dataframe(self): - receiver = Address.from_int(1) - token_address = COW_TOKEN_ADDRESS.address - transfer_df = pd.DataFrame( - { - "token_address": [None, token_address], - "receiver": [str(receiver.address), str(receiver.address)], - "amount": ["12345", "678910"], - "other_useless_column": [True, False], - } - ) - expected = [ - Transfer( - token=None, - recipient=receiver, - amount_wei=12345, - ), - Transfer( - token=Token(COW_TOKEN_ADDRESS), - recipient=receiver, - amount_wei=678910, - ), - ] - - self.assertEqual(expected, Transfer.from_dataframe(transfer_df)) - def test_basic_as_multisend_tx(self): receiver = Web3.to_checksum_address( "0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1"