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

Model il #138

Merged
merged 89 commits into from
Jul 11, 2023
Merged

Model il #138

merged 89 commits into from
Jul 11, 2023

Conversation

jepidoptera
Copy link
Contributor

No description provided.

poliwop and others added 30 commits April 20, 2023 12:57
* moved test_cash_out_accuracy from omnipool_amm to omnipool_state

* in test_cash_out_accuracy, give agent shares and holdings in every asset
* Added limit for add liquidity in test_add_manipulation

* Excluding price_move of 0 in test_withdraw_manipulation

* lift trade cap so arb trades don't fail

---------

Co-authored-by: jepidoptera <jepidoptera@live.com>
* Removed cadCAD

* Removed cadCAD directory

* Removed cadCAD from requirements.txt
remove extra balance check from execute_swap
* Adding tests - WIP

* Adding test for minimum withdrawal fee

* Added nonzero minimums to price_move and lp_percent so that test would not fail with these in the range of 1e-300

* Added test for dynamic withdrawal fee

* Added test of remove_liquidity when the LP price and the current price are different

* Removed redundant setting of oracle values in omnipool_config

* Added error message to test_remove_liquidity_min_fee

* Simplified code to set last oracle values in test_remove_liquidity_dynamic_fee

* Removed unused import

* Added error messages

* Formatting
@jepidoptera jepidoptera requested a review from poliwop June 13, 2023 19:35
hydradx/model/amm/trade_strategies.py Outdated Show resolved Hide resolved
"# price_list = processing.import_binance_prices(['BTC', 'ETH', 'DOT'], start_date='Jan 1 2023', days = 120)\n",
"\n",
"assets = {\n",
" 'HDX': {'usd price': 0.05, 'weight': 0.7999},\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

HDX plays a special role in the Omnipool currently, with LRNA fees being directed to the HDX pool whenever imbalance = 0. I'd thus prefer HDX weight to be realistic (5% or so?) and for there to be some other generic token to make up the additional 65% of the pool.
Would this wind up being identical?

Copy link
Contributor Author

@jepidoptera jepidoptera Jun 14, 2023

Choose a reason for hiding this comment

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

In this notebook, it's the weight of USD and not HDX that changes, and the only thing that changes in price is TKN. I changed it to make that clearer and also updated the weights so USD is the majority.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The HDX/USD price shouldn't change in this notebook, right? Is that what we see? I would expect there needs to be some trade involving HDX to make this happen, which would change its weight?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So my question above stands... and I see that we have TKN and TKN2 in both notebooks? That's what I believe is appropriate.

@jepidoptera jepidoptera requested a review from poliwop June 15, 2023 17:35
Copy link
Collaborator

@poliwop poliwop left a comment

Choose a reason for hiding this comment

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

I think something is off in ImpermanentLossAnalysis.ipynb between the initial instantiation of the Omnipool and the instantiation in the for look. TKN2 is initially in the pool, but isn't present in the for loop. It's not clear to me which is correct.

In ImpermanentLossAnalysis2.ipynb, I think we need HDX to move with the market too. This may mean that I was wrong to suggest that we need HDX and TKN2 to be split out separately, although I still think this is actually clearer. I would simply add HDX to the "other_assets" so that it moves with the market.

"# price_list = processing.import_binance_prices(['BTC', 'ETH', 'DOT'], start_date='Jan 1 2023', days = 120)\n",
"\n",
"assets = {\n",
" 'HDX': {'usd price': 0.05, 'weight': 0.7999},\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

So my question above stands... and I see that we have TKN and TKN2 in both notebooks? That's what I believe is appropriate.

Copy link
Collaborator

@poliwop poliwop left a comment

Choose a reason for hiding this comment

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

I think one more change is needed in ImpermanentLossAnalysis2.ipynb

@jepidoptera jepidoptera requested a review from poliwop July 10, 2023 23:30
@poliwop poliwop merged commit acbd1dc into main Jul 11, 2023
2 checks passed
@poliwop poliwop deleted the model-IL branch July 11, 2023 18:00
poliwop added a commit that referenced this pull request Jul 11, 2023
* Notebooks which illustrate IL exposure in Omnipool

---------

Co-authored-by: poliwop <grove.colin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants