Conversation
# Conflicts: # hydradx/model/amm/omnipool_amm.py
There was a problem hiding this comment.
Pull Request Overview
This PR updates oracle behavior and test expectations to match production implementations. Key changes include:
- Updating the oracle’s update mechanism to incorporate per-asset update intervals.
- Adjusting test tolerances using pytest.approx in oracle-related tests.
- Enhancing swap strategy documentation and handling in trade strategies.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| hydradx/tests/test_oracle.py | Added tests for multi-block oracle updates. |
| hydradx/tests/test_omnipool_amm.py | Updated oracle update and tolerance values in test assertions. |
| hydradx/model/amm/trade_strategies.py | Improved docstring and swap handling in scheduled swaps. |
| hydradx/model/amm/oracle.py | Modified update method to support per-asset update intervals. |
| hydradx/model/amm/omnipool_amm.py | Updated liquidity tracking in add/remove liquidity methods. |
| hydradx/model/amm/exchange.py | Added time_step initialization to Exchange. |
Comments suppressed due to low confidence (1)
hydradx/tests/test_omnipool_amm.py:1532
- The new relative tolerance of 1e-12 in the pytest.approx call is very strict for floating point comparisons. Consider verifying that this tolerance accommodates minor rounding variations in production.
if omnipool_oracle_1.volume_out[tkn] != pytest.approx(expected_vol_out, 1e-12):
| self_attr = getattr(self, attr) | ||
| for tkn in assets: | ||
| self_attr[tkn] = ( | ||
| update_factor[tkn] * self_attr.get(tkn, block_attr[tkn]) |
There was a problem hiding this comment.
Using self_attr.get(tkn, block_attr[tkn]) as a fallback depends on block_attr guarantees. It would be clearer to explicitly initialize self_attr for all asset tokens during object construction to avoid any unexpected fallbacks.
| update_factor[tkn] * self_attr.get(tkn, block_attr[tkn]) | |
| update_factor[tkn] * self_attr[tkn] |
poliwop
left a comment
poliwop
left a comment
There was a problem hiding this comment.
We should still enable oracle updates every block, for modeling purposes, though the new multi-block updates should be the default as that matches PROD. Also, having the old "every block" update will allow us to test the new multi-block update logic against it.
| assets = self.asset_list | ||
| update_steps = {tkn: max(block.time_step - self.last_updated[tkn], 1) for tkn in assets} | ||
| update_factor = {tkn: (1 - self.decay_factor) ** update_steps[tkn] for tkn in assets} | ||
| for attr in ['liquidity', 'price', 'volume_in', 'volume_out']: |
There was a problem hiding this comment.
I'm not requesting any change here... This is maybe a level of abstraction that makes it a little less clear what is going on for little benefit? I realize that on some level it saves doing the same thing 4 times, but there is cost to abstraction too...
| if assets is None: | ||
| assets = self.asset_list | ||
| update_steps = {tkn: max(block.time_step - self.last_updated[tkn], 1) for tkn in assets} | ||
| update_factor = {tkn: (1 - self.decay_factor) ** update_steps[tkn] for tkn in assets} |
There was a problem hiding this comment.
we have sometimes used math.pow() instead of "**", I don't exactly remember if it was for accuracy or what?
There was a problem hiding this comment.
I looked it up, and it seems that the difference is that while ** is slightly faster, math.pow will always return a float (regardless of input type, including int or mpf). In this case it seems like ** is best.
| raise AssertionError('Price is not correct.') | ||
|
|
||
|
|
||
| def test_oracle_multi_block(): |
There was a problem hiding this comment.
Can we fuzz the number of blocks?
There was a problem hiding this comment.
Not on this one, because it's testing for exact values. I do fuzz time steps on other tests.
| events = run.run(initial_state=initial_state, time_steps=10, silent=True) | ||
| final_omnipool = events[-1].pools['omnipool'].update() | ||
| omnipool_oracle = final_omnipool.oracles['price'] | ||
| expected_vol_in = {'DOT': 232.21719930388855, 'HDX': 622.8414188596074, 'USD': 0} |
There was a problem hiding this comment.
I know this is harder to set up, but I think it'd be much better to test against our old setup of updating oracles every block, rather than test against hardcoded values? We should test the 1-block oracle update in other ways (potentially against hardcoded values), but those tests should already be in place I think?
There was a problem hiding this comment.
That's what this one does:
test_update_every_block
Changes to oracle update to bring it in line with how it's done in production.