Option to change deployed agent's bet strategy #53
Conversation
|
Warning Rate Limit Exceeded@kongzii has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 14 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates enhance the prediction market agent tooling by introducing a dynamic bet amount calculation, adding probability calculations for market outcomes, and implementing methods for determining the minimum bet required to win a specified amount. These changes improve the agent's adaptability to market conditions and its ability to calculate bets more precisely, thereby refining its decision-making process in prediction markets. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (8)
- prediction_market_agent_tooling/deploy/agent.py (3 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (3 hunks)
- prediction_market_agent_tooling/markets/bets.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold/data_models.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (2 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (2 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (1 hunks)
- tests/markets/test_bets.py (1 hunks)
Additional comments: 9
prediction_market_agent_tooling/markets/bets.py (1)
- 18-24: The function
minimum_bet_to_win_manifoldcorrectly handles the currency check and usesceilto ensure the bet amount is an integer. This is a good practice for currencies that do not support fractional amounts. The function is well-implemented and adheres to the requirements specified for Manifold markets.prediction_market_agent_tooling/markets/manifold/manifold.py (1)
- 39-39: The assignment of
p_yesusingProbability(model.pool.YES)is a good practice as it encapsulates the probability value within a specific type, enhancing type safety and readability. Ensure thatmodel.pool.YESis always a valid float within the range [0, 1] to maintain the integrity of theProbabilitytype.prediction_market_agent_tooling/markets/agent_market.py (2)
- 21-25: The implementation of the
p_noproperty as1 - self.p_yesis correct and efficiently calculates the complementary probability. This approach is simple and effective for binary outcomes.- 40-46: The
get_outcome_strmethod correctly handles the retrieval of outcomes by index with appropriate error handling for index out of range errors. The error message is clear and informative, aiding in debugging if an incorrect index is used.tests/markets/test_bets.py (1)
- 47-70: The test cases for
test_minimum_bet_to_win_manifoldare correctly designed and include assertions that directly compare the calculated minimum bet against an expected value. This approach is appropriate and ensures the function's correctness across different scenarios.prediction_market_agent_tooling/markets/manifold/data_models.py (1)
- 26-26: The use of
t.ClassVarforBET_AMOUNT_CURRENCYis a good practice as it clearly indicates that this variable is intended to be a class-level constant, not an instance variable. This enhances readability and maintainability of the code.prediction_market_agent_tooling/deploy/agent.py (1)
- 130-130: The integration of
calculate_bet_amountin therunmethod to determine the bet amount dynamically is a significant improvement over a fixed bet amount. This change allows for more flexible and potentially more profitable betting strategies based on market conditions.prediction_market_agent_tooling/markets/omen/data_models.py (2)
- 42-43: The introduction of
BET_AMOUNT_CURRENCYas a class variable is a good practice, clearly indicating the currency used for bet amounts in Omen markets. This enhances code readability and maintainability.- 78-87: The properties
p_yesandp_nofor calculating probabilities are correctly implemented, withp_nobeing the complement ofp_yes. This approach is efficient and simplifies the calculation of probabilities for binary outcomes. Ensure that theoutcomeTokenProbabilitieslist is always correctly populated and that the index forOMEN_TRUE_OUTCOMEis valid.
| @@ -58,6 +58,7 @@ def from_data_model(model: OmenMarket) -> "OmenAgentMarket": | |||
| outcomes=model.outcomes, | |||
| collateral_token_contract_address_checksummed=model.collateral_token_contract_address_checksummed, | |||
| market_maker_contract_address_checksummed=model.market_maker_contract_address_checksummed, | |||
| p_yes=model.p_yes, | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process reveals that the OmenAgentMarket.from_data_model method's old signature is still being used in multiple locations within the codebase, despite the addition of the p_yes parameter. This suggests that not all parts of the codebase have been updated to accommodate the new method signature, which could potentially lead to runtime errors. Therefore, the review comment highlighting the need to ensure all calls to this method have been updated is validated by these findings.
Analysis chain
The addition of the p_yes parameter to the from_data_model method in the OmenAgentMarket class is a significant change. It's important to ensure that all calls to this method throughout the codebase have been updated to include this new parameter. Additionally, consider adding type annotations for the new parameter to improve code readability and maintainability.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search to find files that import OmenAgentMarket. Expecting list of dependent files.
rg --type py 'from .*omen import OmenAgentMarket'
# Search old signature to find omissions. Expecting no results.
ast-grep --lang python --pattern $'OmenAgentMarket.from_data_model($_)'
# Search new signature to validate usage. Expecting list of locations where the new signature is used.
ast-grep --lang python --pattern $'OmenAgentMarket.from_data_model($_, $_)'
Length of output: 825
When introducing new parameters like p_yes in the from_data_model method, it's crucial to update the corresponding documentation and examples to reflect these changes. This helps ensure that the codebase remains accessible to new developers and maintains consistency in documentation.
| outcomes=model.outcomes, | ||
| collateral_token_contract_address_checksummed=model.collateral_token_contract_address_checksummed, | ||
| market_maker_contract_address_checksummed=model.market_maker_contract_address_checksummed, | ||
| p_yes=model.p_yes, | ||
| ) | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Ensure that all external dependencies and modules imported at the beginning of the file are used within the module. Unused imports can lead to confusion and unnecessary code bloat.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [7-7]
The use of type annotations throughout the file is commendable as it improves code readability and helps with static type checking. Consider consistently applying type annotations to all function parameters and return types for enhanced maintainability.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [17-17]
The documentation comments for the OmenAgentMarket class and its methods are helpful for understanding the purpose and usage of the class. Ensure that all public methods and significant logic blocks within the class are similarly documented to maintain code readability and ease of maintenance.
| def minimum_bet_to_win( | ||
| answer: bool, amount_to_win: float, market: AgentMarket | ||
| ) -> float: | ||
| """ | ||
| Estimates the minimum bet amount to win the given amount based on the current market price. | ||
| """ | ||
| share_price = market.p_yes if answer else market.p_no | ||
| bet_amount = amount_to_win / (1 / share_price - 1) | ||
| return bet_amount |
There was a problem hiding this comment.
The function minimum_bet_to_win correctly calculates the minimum bet amount based on the market price. However, it lacks error handling for cases where share_price might be 0, leading to a division by zero error. Consider adding validation to ensure share_price is not 0 before performing the division.
+ if share_price == 0:
+ raise ValueError("Share price cannot be zero.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def minimum_bet_to_win( | |
| answer: bool, amount_to_win: float, market: AgentMarket | |
| ) -> float: | |
| """ | |
| Estimates the minimum bet amount to win the given amount based on the current market price. | |
| """ | |
| share_price = market.p_yes if answer else market.p_no | |
| bet_amount = amount_to_win / (1 / share_price - 1) | |
| return bet_amount | |
| def minimum_bet_to_win( | |
| answer: bool, amount_to_win: float, market: AgentMarket | |
| ) -> float: | |
| """ | |
| Estimates the minimum bet amount to win the given amount based on the current market price. | |
| """ | |
| share_price = market.p_yes if answer else market.p_no | |
| if share_price == 0: | |
| raise ValueError("Share price cannot be zero.") | |
| bet_amount = amount_to_win / (1 / share_price - 1) | |
| return bet_amount |
tests/markets/test_bets.py
Outdated
| @pytest.mark.parametrize( | ||
| "outcome, market_p_yes, amount_to_win", | ||
| [ | ||
| (True, 0.68, 1), | ||
| (False, 0.68, 1), | ||
| (True, 0.7, 10), | ||
| ], | ||
| ) | ||
| def test_minimum_bet_to_win( | ||
| outcome: bool, market_p_yes: Probability, amount_to_win: float | ||
| ) -> None: | ||
| min_bet = minimum_bet_to_win( | ||
| outcome, | ||
| amount_to_win, | ||
| OmenAgentMarket( | ||
| id="id", | ||
| question="question", | ||
| outcomes=["Yes", "No"], | ||
| p_yes=market_p_yes, | ||
| collateral_token_contract_address_checksummed=WXDAI_CONTRACT_ADDRESS, | ||
| market_maker_contract_address_checksummed=Web3.to_checksum_address( | ||
| "0xf3318C420e5e30C12786C4001D600e9EE1A7eBb1" | ||
| ), | ||
| ), | ||
| ) | ||
| assert ( | ||
| min_bet / (market_p_yes if outcome else 1 - market_p_yes) | ||
| >= min_bet + amount_to_win | ||
| ) |
There was a problem hiding this comment.
The test cases for test_minimum_bet_to_win are well-structured and cover different scenarios, including varying probabilities and amounts to win. However, the assertion logic seems incorrect. The formula used in the assertion does not correctly validate the expected behavior of the minimum_bet_to_win function. Consider revising the assertion to directly compare the calculated minimum bet against an expected value.
- assert (
- min_bet / (market_p_yes if outcome else 1 - market_p_yes)
- >= min_bet + amount_to_win
- )
+ # Example correction, assuming an expected_min_bet variable is defined based on the test parameters
+ assert min_bet == expected_min_betCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @pytest.mark.parametrize( | |
| "outcome, market_p_yes, amount_to_win", | |
| [ | |
| (True, 0.68, 1), | |
| (False, 0.68, 1), | |
| (True, 0.7, 10), | |
| ], | |
| ) | |
| def test_minimum_bet_to_win( | |
| outcome: bool, market_p_yes: Probability, amount_to_win: float | |
| ) -> None: | |
| min_bet = minimum_bet_to_win( | |
| outcome, | |
| amount_to_win, | |
| OmenAgentMarket( | |
| id="id", | |
| question="question", | |
| outcomes=["Yes", "No"], | |
| p_yes=market_p_yes, | |
| collateral_token_contract_address_checksummed=WXDAI_CONTRACT_ADDRESS, | |
| market_maker_contract_address_checksummed=Web3.to_checksum_address( | |
| "0xf3318C420e5e30C12786C4001D600e9EE1A7eBb1" | |
| ), | |
| ), | |
| ) | |
| assert ( | |
| min_bet / (market_p_yes if outcome else 1 - market_p_yes) | |
| >= min_bet + amount_to_win | |
| ) | |
| @pytest.mark.parametrize( | |
| "outcome, market_p_yes, amount_to_win", | |
| [ | |
| (True, 0.68, 1), | |
| (False, 0.68, 1), | |
| (True, 0.7, 10), | |
| ], | |
| ) | |
| def test_minimum_bet_to_win( | |
| outcome: bool, market_p_yes: Probability, amount_to_win: float | |
| ) -> None: | |
| min_bet = minimum_bet_to_win( | |
| outcome, | |
| amount_to_win, | |
| OmenAgentMarket( | |
| id="id", | |
| question="question", | |
| outcomes=["Yes", "No"], | |
| p_yes=market_p_yes, | |
| collateral_token_contract_address_checksummed=WXDAI_CONTRACT_ADDRESS, | |
| market_maker_contract_address_checksummed=Web3.to_checksum_address( | |
| "0xf3318C420e5e30C12786C4001D600e9EE1A7eBb1" | |
| ), | |
| ), | |
| ) | |
| # Example correction, assuming an expected_min_bet variable is defined based on the test parameters | |
| assert min_bet == expected_min_bet |
| def calculate_bet_amount(self, answer: bool, market: AgentMarket) -> BetAmount: | ||
| """ | ||
| Calculate the bet amount. By default, it returns the minimum bet amount. | ||
| """ | ||
| return market.get_tiny_bet_amount() |
There was a problem hiding this comment.
The method calculate_bet_amount provides a default implementation that returns the minimum bet amount. This is a good starting point, but it might be beneficial to document that subclasses should override this method to implement more sophisticated bet amount calculations based on specific strategies or market conditions.
+ """
+ Note: Subclasses should override this method to implement specific bet amount calculation strategies based on market conditions.
+ """Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def calculate_bet_amount(self, answer: bool, market: AgentMarket) -> BetAmount: | |
| """ | |
| Calculate the bet amount. By default, it returns the minimum bet amount. | |
| """ | |
| return market.get_tiny_bet_amount() | |
| def calculate_bet_amount(self, answer: bool, market: AgentMarket) -> BetAmount: | |
| """ | |
| Calculate the bet amount. By default, it returns the minimum bet amount. | |
| """ | |
| """ | |
| Note: Subclasses should override this method to implement specific bet amount calculation strategies based on market conditions. | |
| """ | |
| return market.get_tiny_bet_amount() |
| @@ -30,6 +37,14 @@ def place_bet(self, outcome: bool, amount: BetAmount) -> None: | |||
| def get_binary_markets(limit: int) -> list["AgentMarket"]: | |||
| raise NotImplementedError("Subclasses must implement this method") | |||
|
|
|||
| def get_outcome_str(self, outcome_index: int) -> str: | |||
There was a problem hiding this comment.
Noticed this isn't used in the other class, seems like it makes more sense here along with get_outcome_index
| if market.currency != Currency.Mana: | ||
| raise ValueError(f"Manifold bets are made in Mana. Got {market.currency}.") | ||
| # Manifold lowest bet is 1 Mana, so we need to ceil the result. | ||
| return ceil(minimum_bet_to_win(answer, amount_to_win, market)) |
There was a problem hiding this comment.
For example, on https://manifold.markets/dglid/-will-the-ncaa-establish-any-new-po, with 52% p_yes, we need to bet 2 Mana in order to win 3 Mana. It doesn't make sense (except for benchmark purposes of course), to bet 1 Mana because we would get only 1 back.
| @@ -0,0 +1,24 @@ | |||
| from math import ceil | |||
There was a problem hiding this comment.
Maybe rename file to betting_strategies.py?
| return bet_amount | ||
|
|
||
|
|
||
| def minimum_bet_to_win_manifold( |
There was a problem hiding this comment.
Could we keep manifold-specific code in the manifold/ sub-directory. This function could actually be a method on the ManifoldAgentMarket class, like get_tiny_bet_amount
There was a problem hiding this comment.
Yes, much cleaner this way, implemented!
Solves #51
Summary by CodeRabbit