Triangle mode refactor with pstart #676
Conversation
| self.ConfigObj.logger.debug(f"[bot.calculate_profit sorted_price_curves] {sorted_price_curves}") | ||
| # self.ConfigObj.logger.debug(f"[bot.calculate_profit sort_sequence] {sort_sequence}") | ||
| # self.ConfigObj.logger.debug(f"[bot.calculate_profit price_curves] {price_curves}") | ||
| # self.ConfigObj.logger.debug(f"[bot.calculate_profit sorted_price_curves] {sorted_price_curves}") |
There was a problem hiding this comment.
Entire code section replaced with calling ArbitrageFinderBase.calculate_profit.
Need to pull from the base branch.
| self.ConfigObj.logger.debug(f"[bot.calculate_profit price_curves_usd] {price_curves_usd}") | ||
| self.ConfigObj.logger.debug(f"[bot.calculate_profit sorted_price_curves_usd] {sorted_price_curves_usd}") | ||
| # self.ConfigObj.logger.debug(f"[bot.calculate_profit price_curves_usd] {price_curves_usd}") | ||
| # self.ConfigObj.logger.debug(f"[bot.calculate_profit sorted_price_curves_usd] {sorted_price_curves_usd}") |
There was a problem hiding this comment.
Entire code section replaced with calling ArbitrageFinderBase.calculate_profit.
Need to pull from the base branch.
| @@ -906,7 +906,6 @@ def init_bot(mgr: Any) -> CarbonBot: | |||
| mgr=mgr, | |||
| ConfigObj=mgr.cfg, | |||
| state=mgr.pool_data, | |||
| uniswap_v2_event_mappings=mgr.uniswap_v2_event_mappings, | |||
There was a problem hiding this comment.
This may yield a functional (behavioral) change outside the scope of this PR.
Do you know the exact implications of this change?
If not, then we should leave it out of this PR (and probably open an issue on this).
| self.ConfigObj.logger.debug(f"[modes.base.calculate_profit sorted_price_curves] {sorted_price_curves}") | ||
| # self.ConfigObj.logger.debug(f"[modes.base.calculate_profit sort_sequence] {sort_sequence}") | ||
| # self.ConfigObj.logger.debug(f"[modes.base.calculate_profit price_curves] {price_curves}") | ||
| # self.ConfigObj.logger.debug(f"[modes.base.calculate_profit sorted_price_curves] {sorted_price_curves}") |
There was a problem hiding this comment.
Entire function section revised.
Need to pull from the base branch.
| # Since Carbon orders can contain diverse prices independent of external market prices, and | ||
| # we require that the pstart be on the Carbon curve to get successful optimizer runs, | ||
| # then for Carbon orders only we must randomize the pstart from the list of available Carbon curves. | ||
| # Random selection chosen as opposed to iterating over all possible combinations. |
There was a problem hiding this comment.
Why (and since when) do we "require that the pstart be on the Carbon curve to get successful optimizer runs"?
The code below actually opts for a pstart which is NOT on a Crabon curve, as it literally sorts the list of curves such that all Carbon curves are at the end of that list, and it then picks the price of the first curve on that list.
What exactly does choosing a random carbon strategy to get the price from achieve here "as opposed to iterating over all possible combinations"?
First of all, It's not like we were iterating these curves prior to this change.
We simply chose the first curve on the list (CC[0]), and then used its price for pstart.
Second, even if for some reason we need a random Carbon curve here, the first curve is just as random as any other curve on that list, since you have no knowledge of how this list was generated to begin with.
Third, adding randomness in a system requires a really (REALLY) good reason, as it turns the behavior of that system inconsistent, unexpected, non-deterministic, hard to test and hard to reproduce any reported issues.
For example, reducing the probability of collisions between different bot operators is a good reason for choosing a random arbitrage-opportunity (at the top layer). Such reason does not appear to be viable in the case at hand.
|
|
||
| def custom_sort(data, sort_sequence, carbon_v1_forks): | ||
| sort_order = {key: index for index, key in enumerate(sort_sequence) if key not in carbon_v1_forks} | ||
| return sorted(data, key=lambda item: float('inf') if item.params['exchange'] in carbon_v1_forks else sort_order.get(item.params['exchange'], float('inf'))) |
There was a problem hiding this comment.
The code for sorting the list of curves by order of exchange-significance (B2, B3, U2, U3, Carbon) is already implemented in the base class (please pull from the base branch first because it has been slightly modified since you posted this PR).
But even more importantly - you are using it here only for the triangle modes.
If it does indeed the improve the outcome, then it should most likely be implemented for all the other modes as well (probably in the base class, where this code is already implemented anyway).
And as explained in a previous comment - the randomization of Carbon curve prices seems completely out of place. A new list of curves is generated on every iteration of the bot, and the first Carbon curve is as random as any other Carbon curve.
| @@ -32,11 +35,11 @@ def get_combos(self) -> List[Any]: | |||
|
|
|||
| # Note the relevant pairs | |||
| all_relevant_pairs = flt_x_pairs + x_y_pairs | |||
| self.ConfigObj.logger.debug(f"len(all_relevant_pairs) {len(all_relevant_pairs)}") | |||
| self.ConfigObj.logger.debug(f"[triangle_multi_complete.get_combos] all_relevant_pairs: {len(all_relevant_pairs)}") | |||
There was a problem hiding this comment.
No need to go wild, especially in the case of debug loggings.
This convention generates a lot more occurrences of every function name in the code, making it harder to track down when searching for them in actual code (i.e., not within quoted strings).
New pstart handling (in triangle_compete mode)
Bugfix token handling for NATIVE/WRAPPED
Log clean up
Remove duplicate iterations for univ2