Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
max_price_impact
parameter to KellyBettingStrategy #433Add
max_price_impact
parameter to KellyBettingStrategy #433Changes from 10 commits
4bfc702
2c8389a
c9c198a
5333d1d
195f019
fd741d4
f7dc54b
9b6aaa7
337df5e
c17790d
a6804a3
ac99ce1
bda388b
752c3be
6c67f15
f6a7499
0a01259
7a04ea6
d13f235
55d90a6
4b1a6d2
297a0bf
5ba66b5
572b943
ba2afcd
dabcc23
1a3f362
625d012
5e04adf
3ccb430
b7cd215
321cff8
efa1909
06be0d6
79fca0c
04ec90d
b8884de
6b7b6a7
83349d4
22e4028
6f014a6
d3f5c04
74d524e
8cd4818
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
That's interesting, what about cutting it out to some helper function like
get_cached_httpx_client
?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.
Abstracted it away (see class HttpxCachedClient)
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.
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.
You didn't commit this
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.
If these two terms have the same meaning in this case, can we just stick to using one of them?
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.
Thanks, fixed
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.
Hey, there are still uses of the term 'slippage' (in KellyBettingStrategy and the tests)
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.
Thanks. Renamed.
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.
It sounds pretty bad, yet it's logged as
info
, and the code continues. Can it be just ignored?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 this happen a lot if I run the script on this branch. If this is ever hit, does that mean there's a bug in
calculate_bet_amount_for_price_impact
?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.
Indeed the
minimize_scalar
was complaining - I adjusted bounds and now it looks better. Please give it a go and let me know if you find issues!Please note that this is now a ValueError since I want to enforce these boundaries.
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.
Include market fees in price impact calculations
The
fee
parameter is currently set to0
when callingcalculate_bet_amount_for_price_impact
. If the market has a non-zero fee, this could lead to inaccurate calculations. Pass the actual market fee to ensure precise price impact estimation.Apply this diff to include the market fee:
📝 Committable suggestion
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.
So it's possible it's not ok, but it just continues?
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.
Please check revised version - now it's a ValueError.
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.
Why does this need to be a function within the function?
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.
It doesn't have to be, it just felt simpler to have this here since it's not needed anywhere else in the 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.
Hmm, that's almost never done in Python. Can you just make it a normal function, please?
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 dunno, I think it's fine in this context, especially as it's being used as a Callable, passed as an arg to
minimize_scalar
ChatGPT agrees:
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.
Also I did the same thing here
prediction-market-agent-tooling/prediction_market_agent_tooling/tools/utils.py
Lines 231 to 238 in 29224f8
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.
Okay then, I'm standing overvoted 😄 For sure it's not a blocker.
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.
In PMAT, all
scripts
have been helpful in executing some actions so far.This feels more for the
examples
folder. 🤔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.
Removed
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.
These test are in the
omen
directory but they don't have anything specific about OmenThere 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.
Agreed - moved.
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.
Isn't this
p_no
?prediction-market-agent-tooling/prediction_market_agent_tooling/markets/omen/data_models.py
Line 346 in 75e9310
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 changed
yes, no = no, yes
if buy_direction is False.Tests taken from https://docs.gnosis.io/conditionaltokens/docs/introduction3/#an-example-with-cpmm pass.