-
Notifications
You must be signed in to change notification settings - Fork 19
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
Feat/liquidity info #281
Feat/liquidity info #281
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Need to address the CI failures though.
if old_amounts is not None: | ||
l0_start, l0_end = old_amounts[0], new_amounts[0] | ||
l1_start, l1_end = old_amounts[1], new_amounts[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's convert this to a structure, for example, a dataclass called Liquidity
that has the l0s and l1s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly what I had in mind. Old amounts and new amounts lists should be completely replaced by a single object of the liquidity info. That way we do not deal with indexes etc at all, we enhance readability and protect against index errors.
def update_liquidity_amounts(self, old_amounts: List[int], new_amounts: List[int]): | ||
"""Function to update the liquidity amounts""" | ||
self.l0_start, self.l0_end = old_amounts[0], new_amounts[0] | ||
self.l1_start, self.l1_end = old_amounts[1], new_amounts[1] | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't be necessary if the old_amounts
and new_amounts
get replaced with an instance of this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected
if self.shared_state.mock_data is not None: | ||
question_id = self.shared_state.mock_data.id | ||
else: | ||
raise ValueError("No mocked data information") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicated below, you may create a property for safely accessing the self.shared_state.mock_data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the BenchmarkingMockData there is no benchmarking mode so I guess this use-case is already covered in the code even before. No extra checks needed here.
if liquidity_info is None: | ||
self.context.logger.info("No market liquidity information.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can instead check if all the attributes are None
. You can create a method in the dataclass for that.
liquidity_info.l0_start if liquidity_info is not None else None, | ||
liquidity_info.l1_start if liquidity_info is not None else None, | ||
liquidity_info.l0_end if liquidity_info is not None else None, | ||
liquidity_info.l1_end if liquidity_info is not None else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
liquidity_info.l0_start if liquidity_info is not None else None, | |
liquidity_info.l1_start if liquidity_info is not None else None, | |
liquidity_info.l0_end if liquidity_info is not None else None, | |
liquidity_info.l1_end if liquidity_info is not None else None, | |
liquidity_info.l0_start, | |
liquidity_info.l1_start, | |
liquidity_info.l0_end, | |
liquidity_info.l1_end, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
# Liquidity of tokens for option 0, before placing the bet | ||
l0_start: int | ||
# Liquidity of tokens for option 1, before placing the bet | ||
l1_start: int | ||
# Liquidity of tokens for option 0, after placing the bet | ||
l0_end: int | ||
# Liquidity of tokens for option 1, after placing the bet | ||
l1_end: int | ||
|
||
def __init__(self, l0_start, l1_start, l0_end, l1_end): | ||
"""Function to update the liquidity amounts""" | ||
self.l0_start, self.l0_end = l0_start, l0_end | ||
self.l1_start, self.l1_end = l1_start, l1_end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Liquidity of tokens for option 0, before placing the bet | |
l0_start: int | |
# Liquidity of tokens for option 1, before placing the bet | |
l1_start: int | |
# Liquidity of tokens for option 0, after placing the bet | |
l0_end: int | |
# Liquidity of tokens for option 1, after placing the bet | |
l1_end: int | |
def __init__(self, l0_start, l1_start, l0_end, l1_end): | |
"""Function to update the liquidity amounts""" | |
self.l0_start, self.l0_end = l0_start, l0_end | |
self.l1_start, self.l1_end = l1_start, l1_end | |
# Liquidity of tokens for option 0, before placing the bet | |
l0_start: Optional[int] | |
# Liquidity of tokens for option 1, before placing the bet | |
l1_start: Optional[int] | |
# Liquidity of tokens for option 0, after placing the bet | |
l0_end: Optional[int] | |
# Liquidity of tokens for option 1, after placing the bet | |
l1_end: Optional[int] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Co-authored-by: Adamantios Zaras <[email protected]>
@@ -618,6 +624,9 @@ def _write_benchmark_results( | |||
) | |||
return | |||
|
|||
if liquidity_info.l0_start is None: | |||
self.context.logger.info("No market liquidity information.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you are using liquidity_info on line 663-666. If this is None, shouldnt we handle this case differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
…iquidity_info # Conflicts: # packages/packages.json
Feature to include liquidity information at the benchmarking results output file including the initial and final liquidity values of the two options in the pool: option 0 and option 1.