Code restructuring #50

fhenneke · 2023-06-01T14:04:12Z

This is a draft PR to show how the new structure of the EBBO code could look like.

There are multiple classes for fetching data. For now two such classes, Web3API and OrderbookAPI are implemented.
Tests are subclasses of BaseTest. They have
Tests and API use common data types. For now, Trades, OrderData, OrderExecution are implemented.

bh2smith

Hey the structure is looking really good here. I noticed that the "orderbook_api" class has a lot more than just API calls though... might want to consider separating some of that logic.

I made lots of inline suggestions and comments! but its already looking really nice!

src/apis/orderbookapi.py

src/apis/web3api.py

src/monitoring_tests/fee_test.py

src/trades.py

harisang

Looks very good so far. Did a first pass and will do another one tomorrow.

harisang · 2023-06-06T16:53:15Z

src/apis/orderbookapi.py

+    ) -> list[dict[str, Any]]:
+        """
+        This function uses a list of tx hashes to fetch and assemble competition data
+        for each of the tx hashes and returns it.


Might make sense to add a comment that links to the schema that is returned by the competition endpoint. Don't have a direct link but maybe we can add this:

https://api.cow.fi/docs/#/default/get_api_v1_solver_competition_by_tx_hash__tx_hash_

src/apis/orderbookapi.py

harisang · 2023-06-06T17:39:04Z

src/apis/web3api.py

+            transactions = []
+
+        settlement_hashes_list = []
+        # transactions may have repeating hashes, since even event logs are filtered


Just checked and it seems that trade events are appearing here, so a single settlement hash will appear N times, where N is the number of orders being executed in it. This is due to the topics filter criterion set above, which corresponds to trade events. Maybe good to add a clarifying comment here.

why not just listen for settlement events and get the trade events from the tx receipt? Are we listening for trade events?

In my tests, settlement events also happened in transactions not being settlements (but some kind of Safe approval thing). Therefore I stuck with trade events.

src/monitoring_tests/base_test.py

- implement trade, order, execution classes - split API calls into different classes: Web3 and Orderbook - define base_test class - implement fee test on top of base_test class

- use list, tuple, dict instead of List, Tuple, Dict - use SETTLEMENT_CONTRACT_ADDRESS instead of ADDRESS - use different string format in some of the logging - split fee test into smaller functions - use dataclass decorator for some classes - make BaseTest an abstract base class

- split cost coverage and quote test into two tests - implemented a modified daemon: each test has its own hash list to check - made time out a constant REQUEST_TIMEOUT = 5 (seconds) - added new constants for cost_coverage - renamed trades.py to models.py - use fstrings in logging

src/apis/orderbookapi.py

- loading json - pulling out urls for prod and barn - fixed bug in run_queue function

src/apis/orderbookapi.py

src/constants.py

src/models.py

- fix barn address - fixed typo in function doc string

checks if other solutions have settled orders from the winning settlement with more surplus - surplus difference is computed as before relative difference is computed slightly differently: price_win / price_alternative - 1 =(for fill-or-kill)= diff_surplus / surplus_token_amount instead of diff_surplus / surplus_token_limit_amount prices take fee into account: price = (sell_amount + fee_amount) / buy_amount for all order types

fhenneke · 2023-06-26T15:52:42Z

Some things still to be done:

Remove old code
Remove some code duplication

Things to do later:

add some tests
add quasimodo test

- files for old test - organized constants file - added an __init__.py for apis

- ebbo violations are logged as errors - caught connection errors are raised as warnings - if bounds are almost violated, an info message is logged - if no bounds are violated, debug info is logged

should be enabled again later

src/apis/orderbookapi.py

harisang · 2023-06-28T07:57:01Z

src/apis/orderbookapi.py

+                    return None
+        except requests.exceptions.ConnectionError as err:
+            self.logger.warning(
+                f"Connection error while fetching competition data: {err}"


Maybe it would be good to include the request that caused the exception? I am thinking mostly to include the tx hash in the error msg, in case we might need it when debugging.

harisang · 2023-06-28T08:05:04Z

src/apis/orderbookapi.py

+                json=request_dict,
+                timeout=REQUEST_TIMEOUT,
+            )
+        except ValueError as err:


Again, is it possible that we get some other exception here and the script "crashes"?

Not sure anymore why I used a different error here. Will have a look at what types of errors are possible and catch 'em all. (In the sense of 'what types of errors are typically returned by a post request'. In python, anything can happen a run time.)

I changed the error type.

src/apis/web3api.py

src/daemon.py

harisang · 2023-06-28T09:10:47Z

src/daemon.py

+            test.add_hashes_to_queue(tx_hashes)
+            web3_api.logger.debug(f"Running test ({test}) for hashes {test.tx_hashes}.")
+            test.run_queue()
+            web3_api.logger.debug("Test completed.")


Maybe have a msg Test completed for hash 0x...

Do you mean we should keep track of all the tests which where completed? Maybe I should just move those debud statements into run_queue where more info on hashes and test results is available.

- catch more requests errors - add request information to logs for connection errors - made `get_tx_hashes_by_block` return None in case of connection error - add logger to base test class, removed logger from sub classes - add debug info for checked hashes

src/monitoring_tests/partially_fillable_cost_coverage_test.py

A lot of things have changed in this PR and we would like to proceed with merging so that we finally have a working version of it which we can build on. Thus, I am dropping this review so that we can speed things up.

fhenneke requested a review from harisang June 1, 2023 14:04

fhenneke self-assigned this Jun 1, 2023

fhenneke requested a review from bh2smith June 5, 2023 08:55

bh2smith previously requested changes Jun 5, 2023

View reviewed changes

bh2smith marked this pull request as draft June 5, 2023 10:54

harisang mentioned this pull request Jun 6, 2023

Quote test ignores some errors now #52

Merged

harisang reviewed Jun 6, 2023

View reviewed changes

fhenneke added 3 commits June 9, 2023 11:14

started restructuring code

9e0eeb7

- implement trade, order, execution classes - split API calls into different classes: Web3 and Orderbook - define base_test class - implement fee test on top of base_test class

fhenneke force-pushed the full_code_restructuring branch from f1695d9 to c919afc Compare June 9, 2023 17:38

bh2smith reviewed Jun 10, 2023

View reviewed changes

src/apis/orderbookapi.py Outdated Show resolved Hide resolved

src/apis/orderbookapi.py Outdated Show resolved Hide resolved

harisang reviewed Jun 26, 2023

View reviewed changes

src/apis/orderbookapi.py Outdated Show resolved Hide resolved

addressing some comments

bf78994

- loading json - pulling out urls for prod and barn - fixed bug in run_queue function

harisang reviewed Jun 26, 2023

View reviewed changes

src/apis/orderbookapi.py Outdated Show resolved Hide resolved

harisang reviewed Jun 26, 2023

View reviewed changes

src/constants.py Outdated Show resolved Hide resolved

harisang reviewed Jun 26, 2023

View reviewed changes

src/models.py Outdated Show resolved Hide resolved

fhenneke added 2 commits June 26, 2023 17:43

more small fixes

6bfc631

- fix barn address - fixed typo in function doc string

fhenneke added 6 commits June 27, 2023 10:34

remove old code

46aeba4

- files for old test - organized constants file - added an __init__.py for apis

small fixes to surplus test

731454e

remove more unused code

3ecdcbf

change logging

7bde230

- ebbo violations are logged as errors - caught connection errors are raised as warnings - if bounds are almost violated, an info message is logged - if no bounds are violated, debug info is logged

refactor surplus test

c437ddc

disable code duplication pylint error

21cd579

should be enabled again later

fhenneke marked this pull request as ready for review June 27, 2023 14:12

fhenneke changed the title ~~[Draft] Code restructuring~~ Code restructuring Jun 27, 2023

fixed order in comparison of surplus and prices

2e9ccab

harisang reviewed Jun 28, 2023

View reviewed changes

src/apis/orderbookapi.py Outdated Show resolved Hide resolved

harisang reviewed Jun 28, 2023

View reviewed changes

src/apis/web3api.py Outdated Show resolved Hide resolved

harisang reviewed Jun 28, 2023

View reviewed changes

src/daemon.py Outdated Show resolved Hide resolved

harisang reviewed Jun 28, 2023

View reviewed changes

address some comments

29e71b6

- catch more requests errors - add request information to logs for connection errors - made `get_tx_hashes_by_block` return None in case of connection error - add logger to base test class, removed logger from sub classes - add debug info for checked hashes

fhenneke requested a review from bh2smith June 29, 2023 12:39

harisang reviewed Jun 30, 2023

View reviewed changes

src/monitoring_tests/partially_fillable_cost_coverage_test.py Outdated Show resolved Hide resolved

removed line break in error message

607850c

harisang approved these changes Jun 30, 2023

View reviewed changes

fhenneke merged commit 8be84b7 into main Jun 30, 2023

fhenneke deleted the full_code_restructuring branch June 30, 2023 12:01

github-actions bot locked and limited conversation to collaborators Jun 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code restructuring #50

Code restructuring #50

fhenneke commented Jun 1, 2023

bh2smith left a comment

harisang left a comment

harisang Jun 6, 2023

harisang Jun 6, 2023

bh2smith Jun 6, 2023

fhenneke Jun 30, 2023

fhenneke commented Jun 26, 2023 •

edited

Loading

harisang Jun 28, 2023

fhenneke Jun 28, 2023

fhenneke Jun 29, 2023

harisang Jun 28, 2023

fhenneke Jun 28, 2023

fhenneke Jun 29, 2023

harisang Jun 28, 2023

fhenneke Jun 28, 2023

Code restructuring #50

Code restructuring #50

Conversation

fhenneke commented Jun 1, 2023

bh2smith left a comment

Choose a reason for hiding this comment

harisang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhenneke commented Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhenneke commented Jun 26, 2023 •

edited

Loading