Integrate AgentMarket.outcome_token_pool into betting strategies #389
Conversation
evangriffiths
commented
Sep 9, 2024
•
evangriffiths
commented
- Adding a 'full' kelly bet based using the olas code
- Using this in the KellyBettingStrategy if supported
- Refactoring the market-moving-bet function to not be OmenMarket-specific
WalkthroughThe changes in this pull request modify the betting strategy logic within the Changes
Possibly related PRs
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (3)
prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (1)
Line range hint
9-55: Refactor suggestion forget_kelly_bet_simplified.The function
get_kelly_bet_simplifiedhas been correctly updated to return aSimpleBetobject. The logic within the function appears sound, adhering to the Kelly Criterion formula. However, consider refactoring the handling ofmarket_prob == 0to avoid potential division by zero in a more explicit manner.Consider adding a small epsilon to
market_probdirectly when it's calculated to ensure it never hits zero:- if market_prob == 0: - market_prob = 1e-10 + market_prob = max(market_prob, 1e-10)tests/markets/test_betting_strategies.py (2)
162-190: Review oftest_get_market_moving_bet.The test has been updated to reflect the new parameter names and expected outcomes. The assertions are appropriate for verifying the functionality of
get_market_moving_bet. However, consider adding a comment explaining the tolerance used in thenp.isclosefunction to clarify why a specific tolerance level was chosen.Add a comment to explain the choice of tolerance in the
np.isclosefunction:# Tolerance set to 2.0 to allow for minor deviations in floating-point calculations
219-241: Limited Test Coverage forget_kelly_bet_full.The current test file
tests/markets/test_betting_strategies.pycontains only one test case forget_kelly_bet_full, with no visible variations in parameters. To ensure comprehensive validation of the function, consider adding more test cases that cover a diverse range of scenarios and parameter variations.
- Add additional test cases for
get_kelly_bet_fullwith varied parameters to ensure robust testing.Analysis chain
Review of
test_kelly_bet.The test has been updated to include both
get_kelly_bet_simplifiedandget_kelly_bet_full. The logic checks the direction of the bet based on the estimated probability, which is a good practice for verifying the correctness of the betting strategy. Ensure that the tests cover a range of scenarios to fully validate the new and more complexget_kelly_bet_fullfunction.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the coverage of test cases for `get_kelly_bet_full`. # Test: Search for test cases related to `get_kelly_bet_full`. Expect: A diverse range of test scenarios. rg --type python -A 5 $'get_kelly_bet_full'Length of output: 73
Script:
#!/bin/bash # Description: Verify the coverage of test cases for `get_kelly_bet_full`. # Corrected Test: Search for test cases related to `get_kelly_bet_full`. Expect: A diverse range of test scenarios. rg --type py -A 5 'get_kelly_bet_full'Length of output: 2742
Script:
#!/bin/bash # Description: Extract detailed information about test cases for `get_kelly_bet_full` in `tests/markets/test_betting_strategies.py`. # Extract the relevant test cases and their context to assess coverage and diversity. rg --type py -A 20 'get_kelly_bet_full' tests/markets/test_betting_strategies.pyLength of output: 1235
Script:
#!/bin/bash # Description: Search for all occurrences of `get_kelly_bet_full` in the test file to assess the diversity of test scenarios. # Search for all instances of `get_kelly_bet_full` in the test file to identify multiple test cases or parameter variations. rg --type py 'get_kelly_bet_full' tests/markets/test_betting_strategies.pyLength of output: 126
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- prediction_market_agent_tooling/deploy/betting_strategy.py (2 hunks)
- prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (2 hunks)
- prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (2 hunks)
- prediction_market_agent_tooling/tools/betting_strategies/utils.py (1 hunks)
- tests/markets/test_betting_strategies.py (5 hunks)
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Ruff
prediction_market_agent_tooling/tools/betting_strategies/market_moving.py
37-37: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
Additional comments not posted (7)
prediction_market_agent_tooling/tools/betting_strategies/utils.py (1)
4-6: LGTM!The
SimpleBetclass is well-defined with appropriate field types and uses Pydantic for data validation.prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (2)
5-6: Approved import changes.The imports are correctly updated to reflect the new dependencies and usage within the file.
82-82: Approved return statement.The function correctly returns a
SimpleBetobject, enhancing the clarity and usability of the function's output.prediction_market_agent_tooling/deploy/betting_strategy.py (1)
8-11: Approved import changes.The imports are correctly updated to include the new functions and constants necessary for the updated betting strategy logic.
prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (2)
1-1: Approved import statement.The import of
SimpleBetfrom theutilsmodule is correct and necessary due to the removal ofKellyBetand the introduction ofSimpleBet.
3-7: Ensure probability validation is robust.The function
check_is_valid_probabilitycorrectly checks if a probability value is within the acceptable range [0, 1]. This is crucial for the integrity of the betting calculations that follow.tests/markets/test_betting_strategies.py (1)
32-33: Approved import updates.The updated imports for
get_kelly_bet_fullandget_kelly_bet_simplifiedare correct and reflect the changes in the betting strategy logic.
prediction_market_agent_tooling/tools/betting_strategies/market_moving.py
Show resolved
Hide resolved
prediction_market_agent_tooling/tools/betting_strategies/market_moving.py
Outdated
Show resolved
Hide resolved
prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (2 hunks)
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Ruff
prediction_market_agent_tooling/tools/betting_strategies/market_moving.py
37-37: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
Additional comments not posted (2)
prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (2)
10-16: LGTM!The function signature changes are approved. The new parameters and return type improve the flexibility and usability of the function.
36-42: The previous review comment on this code segment is still valid. Refer to the comment below:"""
coderabbitai[bot]: Refactor suggestion: Set a more realistic upper bound.The TODO comment acknowledges the need for a better upper bound for
max_bet_amount. Consider using market data or historical bet sizes to set this dynamically.Tools
Ruff
37-37: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
"""
Tools
Ruff
37-37: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
prediction_market_agent_tooling/tools/betting_strategies/market_moving.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- prediction_market_agent_tooling/deploy/betting_strategy.py (2 hunks)
- prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (2 hunks)
- prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (2 hunks)
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Additional comments not posted (5)
prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (1)
Line range hint
10-82: LGTM!The changes to the
get_market_moving_betfunction are approved. The refactoring has improved the function's flexibility and usability by:
- Modifying the function signature to accept pool sizes, market probabilities, and other parameters directly, instead of relying on an
OmenMarketobject.- Returning a
SimpleBetobject, which encapsulates the bet's direction and size, enhancing clarity.- Retaining the binary search mechanism for determining the optimal bet amount while adjusting the calculations to reflect the new parameters.
- Maintaining the assertions and checks for market invariants based on the new structure of the amounts.
The function is well-documented with a clear explanation of the underlying mathematical formula, and it correctly handles potential issues like length mismatches between vectors.
prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (3)
Line range hint
9-55: LGTM!The changes to the
get_kelly_bet_simplifiedfunction (previouslyget_kelly_bet) are approved. The refactoring has improved the function's clarity and consistency by:
- Renaming the function to
get_kelly_bet_simplified, which accurately reflects its purpose and distinguishes it from the newget_kelly_bet_fullfunction.- Changing the return type from
KellyBettoSimpleBet, aligning with the streamlined representation of bets introduced in other parts of the codebase.The function remains well-documented with a clear explanation of the underlying mathematical formula, and it includes appropriate validation checks for the input probabilities and confidence levels.
58-137: Approve the addition ofget_kelly_bet_fullwith a suggestion to verify the mathematical formula.The new
get_kelly_bet_fullfunction is a valuable addition to the betting strategy logic, as it introduces a more comprehensive calculation for the Kelly Criterion, accounting for additional market dynamics such as outcome pool sizes and fees.The function is well-documented with a clear explanation of the underlying mathematical formula and its source. It includes appropriate validation checks for the input probabilities, confidence level, and fee, and correctly handles edge cases where
max_betor the denominator is 0.The use of the
SimpleBetreturn type is consistent with the streamlined representation of bets introduced in other parts of the codebase.However, given the mathematical complexity of the formula, it is crucial to ensure its correctness. Consider the following suggestions:
- Implement comprehensive unit tests to verify the function's behavior under various input scenarios, including edge cases.
- Have the mathematical formula reviewed by a domain expert to confirm its accuracy and applicability to the specific use case.
Would you like assistance in setting up additional unit tests or facilitating a review of the mathematical formula by a domain expert?
1-1: Approve the removal of theKellyBetclass.The removal of the
KellyBetclass is a positive change that aligns with the shift towards a more streamlined representation of bets using theSimpleBetclass throughout the codebase.The
SimpleBetclass likely provides similar functionality to the removedKellyBetclass, making the latter redundant. The use of theSimpleBetclass in the return types ofget_kelly_bet_simplifiedandget_kelly_bet_fullensures consistency with other parts of the codebase.prediction_market_agent_tooling/deploy/betting_strategy.py (1)
173-192: LGTM!The changes in the
calculate_tradesmethod of theKellyBettingStrategyclass are approved. The refactoring enhances the flexibility of the betting strategy by:
- Allowing the method to choose between the full and simplified Kelly Criterion calculations based on the availability of a token pool in the market.
- Using
get_kelly_bet_fullwhen a token pool is available, enabling a more comprehensive calculation that accounts for additional market dynamics such as outcome pool sizes and fees.- Using
get_kelly_bet_simplifiedwhen a token pool is not available, providing a fallback option that uses fewer parameters and relies on the current market probability.The
check_not_nonefunction is used appropriately to ensure that theoutcome_token_poolis notNonebefore accessing its values, preventing potential errors.The method correctly constructs the
amountsdictionary andtarget_positionbased on the direction and size of thekelly_bet, ensuring consistency with the rest of the betting strategy logic.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (2 hunks)
- tests/markets/test_betting_strategies.py (5 hunks)
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Additional comments not posted (5)
prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (2)
12-18: LGTM!The changes to the
get_market_moving_betfunction are approved. The refactoring improves the function's interface and logic by:
- Modifying the function signature to accept pool sizes and other parameters directly, instead of relying on an
OmenMarketobject.- Returning a
SimpleBetobject to encapsulate the bet's direction and size, enhancing clarity and usability.- Refactoring the internal logic to use the provided pool sizes for calculations, eliminating the dependency on the
OmenMarketclass.These changes make the function more flexible and easier to use with different market configurations.
Also applies to: 38-44, 48-63, 84-84
87-134: LGTM!The addition of the
_sanity_check_omen_market_moving_betfunction is approved. This utility function is a useful addition for sanity checking the market moving bets by:
- Calling the market's calcBuyAmount method from the smart contract.
- Using the adjusted outcome pool sizes to calculate the new p_yes.
- Checking that the market's new p_yes is equal to the target_p_yes.
This function helps ensure the correctness of the market moving bets.
tests/markets/test_betting_strategies.py (3)
164-193: LGTM!The changes to the
test_get_market_moving_betfunction are approved. The updates align with the modifications made to theget_market_moving_betfunction by:
- Altering the parameterization of the function to use the new parameter names and types.
- Updating the logic to accommodate the new parameter names and types.
- Adjusting the call to
get_market_moving_betto pass the new parameters.- Modifying the assertions to check against the new expected values.
These changes ensure that the test function accurately evaluates the updated
get_market_moving_betfunction.
195-216: LGTM!The addition of the
test_sanity_check_market_moving_betfunction is approved. This function is a valid test for the_sanity_check_omen_market_moving_betfunction added in theprediction_market_agent_tooling/tools/betting_strategies/market_moving.pyfile. It:
- Retrieves a binary market using the
OmenAgentMarket.get_binary_marketsmethod.- Calls the
get_market_moving_betfunction with the market's pool sizes, current p_yes, target p_yes, and fee.- Calls the
_sanity_check_omen_market_moving_betfunction with the market moving bet, market, and target p_yes.This test function helps ensure the correctness of the
_sanity_check_omen_market_moving_betfunction.
246-268: LGTM!The changes to the
test_kelly_betfunction are approved. The modifications enhance the testing of the Kelly criterion betting strategy by:
- Replacing the call to
get_kelly_betwithget_kelly_bet_simplified.- Introducing a new assertion that utilizes
get_kelly_bet_full.These changes allow for a more comprehensive evaluation of the bet direction based on different calculation methods, improving the robustness of the test function.
Co-authored-by: Peter Jung <peter@jung.ninja>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- prediction_market_agent_tooling/deploy/betting_strategy.py (2 hunks)
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Additional comments not posted (3)
prediction_market_agent_tooling/deploy/betting_strategy.py (3)
14-15: LGTM!The import statements are approved.
17-17: LGTM!The import statement is approved.
173-191: Skip the previous review comment.The previous review comment suggesting the use of
OMEN_FALSE_OUTCOMEforno_outcome_pool_sizeis not applicable to the current code changes. The code correctly usesmarket.get_outcome_str_from_bool(True)andmarket.get_outcome_str_from_bool(False)for accessing the yes and no outcome pools respectively.
|
Trying to verify the calculation in ``, and it looks like there's a bug somewhere. You would expect the kelly bet to asymptote to the market-moving bet, but see from the plots (for two different markets) that this isn't the case: 
Since I've added a test that checks the market-moving-bet vs the buy amount calculated by the smart contract, I'm convinced the bug is in the kelly bet code. To reproduce: |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
prediction_market_agent_tooling/deploy/betting_strategy.py (1)
173-192: LGTM, but consider simplifying the conditional logic.The changes to introduce the conditional logic for choosing between
get_kelly_bet_fullandget_kelly_bet_simplifiedbased on the availability of a token pool in the market are approved. This enhances the flexibility of the betting strategy.The use of
check_not_noneis a good practice to ensure that the outcome pool sizes are validated before use.Consider simplifying the conditional logic by directly returning the result inside the conditional branches, reducing the need for the temporary variable
kelly_bet:return ( get_kelly_bet_full( yes_outcome_pool_size=check_not_none(market.outcome_token_pool)[ market.get_outcome_str_from_bool(True) ], no_outcome_pool_size=check_not_none(market.outcome_token_pool)[ market.get_outcome_str_from_bool(False) ], estimated_p_yes=answer.p_yes, max_bet=self.max_bet_amount, confidence=answer.confidence, ) if market.has_token_pool() else get_kelly_bet_simplified( self.max_bet_amount, market.current_p_yes, answer.p_yes, answer.confidence, ) )
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- prediction_market_agent_tooling/deploy/betting_strategy.py (2 hunks)
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Additional comments not posted (3)
prediction_market_agent_tooling/deploy/betting_strategy.py (3)
14-15: LGTM!The imports are necessary for the changes introduced later in the file.
17-17: LGTM!The import is necessary for the changes introduced later in the file.
176-181: Refactor Required: Use Correct Indices for Outcome PoolsThe code incorrectly uses
OMEN_TRUE_OUTCOMEfor bothyes_outcome_pool_sizeandno_outcome_pool_size. It should useOMEN_TRUE_OUTCOMEforyes_outcome_pool_sizeandOMEN_FALSE_OUTCOMEforno_outcome_pool_sizeto ensure correct functionality.Run the following script to verify the correct indices for outcome pools:
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
prediction_market_agent_tooling/deploy/betting_strategy.py (1)
173-192: LGTM, but consider simplifying the conditional logic.The changes to introduce the conditional logic for choosing between
get_kelly_bet_fullandget_kelly_bet_simplifiedbased on the availability of a token pool in the market are approved. This enhances the flexibility of the betting strategy.The use of
check_not_noneis a good practice to ensure that the outcome pool sizes are validated before use.Consider simplifying the conditional logic by directly returning the result inside the conditional branches, reducing the need for the temporary variable
kelly_bet:return ( get_kelly_bet_full( yes_outcome_pool_size=check_not_none(market.outcome_token_pool)[ market.get_outcome_str_from_bool(True) ], no_outcome_pool_size=check_not_none(market.outcome_token_pool)[ market.get_outcome_str_from_bool(False) ], estimated_p_yes=answer.p_yes, max_bet=self.max_bet_amount, confidence=answer.confidence, ) if market.has_token_pool() else get_kelly_bet_simplified( self.max_bet_amount, market.current_p_yes, answer.p_yes, answer.confidence, ) )
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- prediction_market_agent_tooling/deploy/betting_strategy.py (2 hunks)
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Additional comments not posted (4)
prediction_market_agent_tooling/deploy/betting_strategy.py (4)
14-15: LGTM!The import statements are correctly updated to include the new functions
get_kelly_bet_fullandget_kelly_bet_simplifiedfromkelly_criterion.py.
17-17: LGTM!The import statement for
check_not_noneis correctly added to validate outcome pool sizes before use.
173-192: The suggestion to refactor the conditional logic for choosing betweenget_kelly_bet_fullandget_kelly_bet_simplifiedto simplify it is still valid and applicable to the current code segment.
173-192: The potential issue with using the same index for both yes and no outcome pool sizes is not applicable to the current code segment, as it now usesmarket.get_outcome_str_from_bool(True)andmarket.get_outcome_str_from_bool(False)to get the correct indices.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
prediction_market_agent_tooling/deploy/betting_strategy.py (1)
173-192: LGTM, but consider simplifying the conditional logic.The changes to introduce the conditional logic for choosing between
get_kelly_bet_fullandget_kelly_bet_simplifiedbased on the availability of a token pool in the market are approved. This enhances the flexibility of the betting strategy.The use of
check_not_noneis a good practice to ensure that the outcome pool sizes are validated before use.Consider simplifying the conditional logic by directly returning the result inside the conditional branches, reducing the need for the temporary variable
kelly_bet:return ( get_kelly_bet_full( yes_outcome_pool_size=check_not_none(market.outcome_token_pool)[ market.get_outcome_str_from_bool(True) ], no_outcome_pool_size=check_not_none(market.outcome_token_pool)[ market.get_outcome_str_from_bool(False) ], estimated_p_yes=answer.p_yes, max_bet=self.max_bet_amount, confidence=answer.confidence, ) if market.has_token_pool() else get_kelly_bet_simplified( self.max_bet_amount, market.current_p_yes, answer.p_yes, answer.confidence, ) )
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- prediction_market_agent_tooling/deploy/betting_strategy.py (2 hunks)
Additional comments not posted (3)
prediction_market_agent_tooling/deploy/betting_strategy.py (3)
14-15: LGTM!The import changes are approved. They are consistent with the alterations to the declarations of exported or public entities.
Also applies to: 17-17
173-192: The refactoring suggestion for the conditional logic is still valid and applicable to the current code segment. Please refer to the previous comment for the suggested refactoring.
173-192: The issue with using the same index for both outcome pool sizes is not applicable to the current code segment as it usesmarket.get_outcome_str_from_bool(True)andmarket.get_outcome_str_from_bool(False)to get the correct indices.
| denominator = 2 * (x**2 * f - y**2 * f) | ||
| if denominator == 0: | ||
| return SimpleBet(direction=True, size=0) |
There was a problem hiding this comment.
Quick question about the formula: if (x=yes_outcome_pool_size = 10, y=no_outcome_pool_size=10), this always goes to 0 thus bet_amount is hard-set to 0.
Is that correct? This would e.g. prevent us from placing bets in balanced markets using Kelly - but maybe I'm mistaken.
There was a problem hiding this comment.
Yes, but that doesn't seem like desirable behaviour! Have made an issue to fix: #401
|
Script to reproduce 
on this PR #389 |
