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.

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

Add settlement contract buffers value monitoring test #75

Merged
merged 19 commits into from
Nov 9, 2023

Conversation

harisang
Copy link
Contributor

@harisang harisang commented Oct 6, 2023

This PR adds a test that monitors the value of buffers by using price feeds from ethplorer and coingecko, while restricting the tokens that are checked to the ones included in one of the following lists (whichever can be recovered at the time of the check):

It checks the value of buffers every BUFFER_INTERVAL settlements, a constant currently set to 150, and if the value is above BUFFER_VALUE_THRESHOLD USD, a constant currently set to 200,000, it generates an alert.

The purpose of this PR is to get a better understanding of how quickly our buffers' value increases, since this might be relevant if we want to reduce the bonding pool size requirements.

@harisang harisang requested a review from fhenneke October 6, 2023 22:09
@harisang harisang changed the title Add buffer value monitoring test Add settlement contract buffers value monitoring test Oct 6, 2023
@harisang
Copy link
Contributor Author

harisang commented Oct 6, 2023

One comment here is that there are illiquid/random tokens whose value might look high but which can never be effectively be traded at the price reported by coingecko or ethplorer. To mitigate that, we could only focus on a list of "reasonable" tokens. See, e.g., here: https://tokenlists.org/, the Kleros token list being a candidate one.

src/constants.py Outdated Show resolved Hide resolved
Comment on lines 34 to 42
resp = requests.get(
"https://api.ethplorer.io/\
getAddressInfo/\
0x9008D19f58AAbD9eD0D60971565AA8510560ab41?\
apiKey=freekey",
headers=header,
timeout=REQUEST_TIMEOUT,
)
rsp = resp.json()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be moved to some EthplorerAPI. Not sure what part of the parsing of the response can be abstracted away into the API as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you mind if I keep this as is? I know this introduces an inconsistency but APIs that are very unlikely to be reused in any other way, such as this one, might not worth the effort properly incorporating them in the project. If you insist on this though, I can take care of it.

src/monitoring_tests/buffers_monitoring_test.py Outdated Show resolved Hide resolved
src/monitoring_tests/buffers_monitoring_test.py Outdated Show resolved Hide resolved
@harisang
Copy link
Contributor Author

After a loong time, I got around to implementing some of the suggested changes. Could you have a look @fhenneke ? Thanks!

Copy link
Contributor

@fhenneke fhenneke left a comment

Choose a reason for hiding this comment

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

I added a few comments.
I was not able to get all API requests to return meaningful results. So a final buffer accounting was not shown to me yet. Does it work for you already?

def __init__(self) -> None:
self.logger = get_logger()

def get_token_list(self) -> list[str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def get_token_list(self) -> list[str]:
def get_token_list(self) -> Optional[list[str]]:

Returns the Kleros token list.
"""
kleros_url = "http://t2crtokens.eth.link"

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
kleros_list: Optional[list[str]] = None

src/apis/klerosapi.py Outdated Show resolved Hide resolved
self.logger.warning(
f"Connection error while fetching the Kleros token list, error: {err}"
)
return kleros_list
Copy link
Contributor

Choose a reason for hiding this comment

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

Without the changes on setting a default, this sometimes crashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added what is now L36-37, to address the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

It still crashes for we when the request times out. Our current convention for that case is to use None as result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And all these since I cannot catch a general exception here due to the mypy,pylint etc.... Will have another look.

timeout=REQUEST_TIMEOUT,
)
ethplorer_rsp = ethplorer_data.json()
kleros_list = self.kleros_api.get_token_list()
Copy link
Contributor

Choose a reason for hiding this comment

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

Handle the case of empty kleros_list here somehow. For example

Suggested change
kleros_list = self.kleros_api.get_token_list()
kleros_list = self.kleros_api.get_token_list()
if kleros_list is None:
kleros_list = []

or maybe just skip the hash?

@harisang harisang force-pushed the add_buffers_monitoring_test branch from a509276 to f58aac0 Compare November 6, 2023 10:50
@harisang harisang requested a review from fhenneke November 6, 2023 14:55
Copy link
Contributor

@fhenneke fhenneke left a comment

Choose a reason for hiding this comment

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

I still cannot get the call to kleros to work.

src/apis/klerosapi.py Outdated Show resolved Hide resolved
@harisang
Copy link
Contributor Author

harisang commented Nov 8, 2023

Did some edits, mainly adding 2 more lists and incorporating some comments. Could you have another look @fhenneke ? (sorry for the back and forth and thanks for checking in this in detail)

Copy link
Contributor

@fhenneke fhenneke left a comment

Choose a reason for hiding this comment

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

Except for the small comments this looks good to me.

Running the code

from src.monitoring_tests.buffers_monitoring_test import (
    BuffersMonitoringTest,
)
buffer_test = BuffersMonitoringTest()
buffer_test.counter = 150
tx_hash = "0x65d07be85b9e067466259079e77e8fe72ac897535fcbda549a526c80e2ba09b4"

buffer_test.run(tx_hash)

returns

WARNING - Connection error while fetching a token list, error: HTTPSConnectionPool(host='t2crtokens.eth.link', port=443): Read timed out. (read timeout=5)
INFO - Buffer value is 97862.06651828821 USD
True

src/apis/tokenlistapi.py Outdated Show resolved Hide resolved
src/apis/tokenlistapi.py Outdated Show resolved Hide resolved
Co-authored-by: Felix Henneke <felix.henneke@protonmail.com>
Co-authored-by: Felix Henneke <felix.henneke@protonmail.com>
@harisang harisang merged commit 831ec31 into main Nov 9, 2023
3 checks passed
@harisang harisang deleted the add_buffers_monitoring_test branch November 9, 2023 15:50
@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2023
@harisang harisang restored the add_buffers_monitoring_test branch November 9, 2023 16:03
@harisang harisang deleted the add_buffers_monitoring_test branch November 9, 2023 16:04
@harisang harisang restored the add_buffers_monitoring_test branch November 9, 2023 16:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants