Allow dynamic bet amounts based on market - remove 'adjust_bet_amount' #430
Conversation
WalkthroughThe pull request introduces modifications across several components of the prediction market agent tooling. Key changes include the addition of a Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
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: 2
Outside diff range and nitpick comments (2)
tests/test_betting_strategy.py (1)
29-29: LGTM. Consider adding a test for dynamic bet amounts.The change aligns with the PR objective of allowing dynamic bet amounts. However, to ensure comprehensive test coverage:
Consider adding a test case that explicitly verifies the dynamic bet amount functionality. This could involve creating multiple instances of
MaxAccuracyBettingStrategywith different bet amounts and asserting that they behave correctly.It might be beneficial to parameterize this test to cover various bet amounts, ensuring the strategy works correctly across different scenarios.
prediction_market_agent_tooling/markets/omen/omen.py (1)
636-639: LGTM! Consider adding a return type hint.The new
get_user_balancestatic method is well-implemented and serves its purpose of retrieving a user's balance. It correctly converts the user_id to a checksum address and uses theget_balancesfunction to fetch the total balance.Consider adding a return type hint for better code readability and type checking:
@staticmethod - def get_user_balance(user_id: str) -> float: + def get_user_balance(user_id: str) -> float: return float(get_balances(Web3.to_checksum_address(user_id)).total)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- prediction_market_agent_tooling/deploy/agent.py (3 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (4 hunks)
- prediction_market_agent_tooling/jobs/omen/omen_jobs.py (1 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (1 hunks)
- tests/test_betting_strategy.py (1 hunks)
Additional comments not posted (11)
tests/test_betting_strategy.py (2)
Line range hint
1-62: Overall, the changes look good but consider enhancing test coverage.The modifications to both
test_answer_decisionandtest_rebalancefunctions align well with the PR objective of allowing dynamic bet amounts. However, to further improve the test suite:
- Consider adding explicit tests for the dynamic bet amount functionality across various scenarios.
- Document the reasoning behind the chosen bet amounts, especially the very small amount in
test_rebalance.- Ensure that all assertions remain valid with the new bet amounts.
These enhancements will provide more robust test coverage for the new dynamic bet amount feature.
35-35: LGTM. Consider documenting the tiny bet amount and verifying assertions.The change aligns with the PR objective of allowing dynamic bet amounts. To improve the test:
Consider adding a comment explaining the rationale behind using such a small bet amount (0.0001). This helps other developers understand if it's testing a specific edge case or precision handling.
Verify if the assertions later in the test (lines 56-62) need adjustment due to the tiny bet amount. Ensure that the expected behavior is still correctly captured with this change.
To verify the impact of the tiny bet amount on the test assertions, you can run the following script:
This will help ensure that the test logic remains correct with the new bet amount.
prediction_market_agent_tooling/jobs/omen/omen_jobs.py (1)
86-87: LGTM! Consider clarifying the PR objective.The change improves code readability by separating the instantiation of the
KellyBettingStrategyfrom the method call. This is a good practice and makes the code easier to understand and maintain.However, the PR title mentions removing 'adjust_bet_amount', which is not directly visible in this change. Could you please clarify where this removal occurred or how the functionality has been adjusted? This will help ensure that the PR objectives are fully met and documented.
To verify the removal of 'adjust_bet_amount', let's search for its occurrences in the codebase:
If this search returns no results, it confirms that 'adjust_bet_amount' has been removed from the codebase as per the PR objective.
Verification successful
[/run_scripts]
'adjust_bet_amount' Successfully Removed
The search across all Python files returned no occurrences of
adjust_bet_amount, confirming its removal from the codebase as per the PR objective.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for 'adjust_bet_amount' in the codebase # Expected result: No occurrences of 'adjust_bet_amount' should be found rg --type python 'adjust_bet_amount'Length of output: 67
Script:
#!/bin/bash # Description: Search for 'adjust_bet_amount' in all Python files using fd and rg # Expected result: No occurrences of 'adjust_bet_amount' should be found fd --extension py --type file | xargs rg 'adjust_bet_amount'Length of output: 60
prediction_market_agent_tooling/deploy/betting_strategy.py (4)
96-97: LGTM: Constructor signature update improves type safetyThe change to require a non-optional
bet_amountparameter in the constructor aligns with the PR objective of allowing dynamic bet amounts. This update also improves type safety by eliminating the possibility ofNonevalues forbet_amount.
109-109: LGTM: Simplified betting logic with improved flexibilityThe direct use of
self.bet_amountsimplifies the betting logic and aligns with the removal of theadjust_bet_amountmethod. This change allows for more flexible betting strategies as the bet amount can be set dynamically when initializing the strategy.
134-135: LGTM: Consistent implementation of dynamic bet amountsThe changes in
KellyBettingStrategyare consistent with the PR objective and the modifications made toMaxAccuracyBettingStrategy:
- The constructor now requires a non-optional
max_bet_amount, improving type safety.- The
calculate_tradesmethod usesself.max_bet_amountdirectly in Kelly criterion calculations, allowing for dynamic bet amounts.These changes successfully implement the goal of allowing dynamic bet amounts based on the market.
Also applies to: 153-153, 158-158
Line range hint
1-174: Summary: PR successfully implements dynamic bet amountsThis PR successfully achieves its objective of allowing dynamic bet amounts based on the market. The key changes include:
- Removal of the
adjust_bet_amountmethod from theBettingStrategyclass.- Updated constructors in
MaxAccuracyBettingStrategyandKellyBettingStrategyto require non-optional bet amount parameters.- Simplified betting logic in both strategies to use the provided bet amounts directly.
These changes improve type safety, simplify the betting logic, and provide more flexibility in setting bet amounts. The implementation is consistent across the affected classes and aligns well with the PR objectives.
prediction_market_agent_tooling/markets/agent_market.py (2)
285-287: LGTM: New methodget_user_balanceadded.The new static method
get_user_balanceis a good addition to theAgentMarketclass. It follows the existing pattern of abstract methods in the class, providing a consistent interface for subclasses to implement user balance retrieval.
Line range hint
1-287: Verify PR objectives: Removal of 'adjust_bet_amount'.The PR title mentions removing 'adjust_bet_amount', but this change is not visible in the provided code. Please ensure that this removal has been implemented as intended.
To verify the removal of 'adjust_bet_amount', run the following script:
Verification successful
Verified PR Objective: Removal of
adjust_bet_amountimplemented successfully.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of 'adjust_bet_amount' in the codebase # Test: Search for 'adjust_bet_amount' in Python files rg --type python 'adjust_bet_amount' # If the above command returns results, the method might not have been removed as intendedLength of output: 67
Script:
#!/bin/bash # Description: Check for the presence of 'adjust_bet_amount' in Python files rg --glob '*.py' 'adjust_bet_amount'Length of output: 36
prediction_market_agent_tooling/deploy/agent.py (2)
Line range hint
420-430: LGTMThe changes to the
build_tradesmethod correctly update the signature to includemarket_type, and the implementation is consistent.
459-462: LGTMThe updated call to
build_tradesinprocess_marketnow includesmarket_type, aligning with the new method signature.
| def get_betting_strategy( | ||
| self, market_type: MarketType, market: AgentMarket | ||
| ) -> BettingStrategy: | ||
| user_id = APIKeys().bet_from_address |
There was a problem hiding this comment.
bet_from_address is Omen-only, it would be None and error out for other markets
Maybe new property on APIKeys that would return the correct user id (bet_from_address or manifold_user_id or something else) based on the MarketType?
There was a problem hiding this comment.
Good spot! It's a bit of a major change fixing circular import issues after adding that to APIKeys, so I've put it as (another! 😢) method on AgentMarket and its subclasses
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
prediction_market_agent_tooling/deploy/betting_strategy.py (4)
Line range hint
96-109: LGTM! Consider adding type hinting forself.bet_amount.The changes to the
MaxAccuracyBettingStrategyclass align well with the PR objective of allowing dynamic bet amounts based on market conditions. The removal of the optional parameter in the constructor enforces that a bet amount must be provided, which is a good practice.Consider adding type hinting for
self.bet_amountin the constructor:class MaxAccuracyBettingStrategy(BettingStrategy): - def __init__(self, bet_amount: float): + def __init__(self, bet_amount: float): + self.bet_amount: float = bet_amount - self.bet_amount = bet_amountThis will improve code readability and help with static type checking.
Line range hint
137-161: LGTM! Consider adding type hinting forself.max_bet_amount.The changes to the
KellyBettingStrategyclass are in line with the PR objective of allowing dynamic bet amounts based on market conditions. Removing the default value formax_bet_amountin the constructor is a good practice as it ensures that a maximum bet amount is always provided.Consider adding type hinting for
self.max_bet_amountin the constructor:class KellyBettingStrategy(BettingStrategy): - def __init__(self, max_bet_amount: float): + def __init__(self, max_bet_amount: float): + self.max_bet_amount: float = max_bet_amount - self.max_bet_amount = max_bet_amountThis will improve code readability and help with static type checking.
Line range hint
180-238: UpdateMaxAccuracyWithKellyScaledBetsStrategyto align with other classes.The
MaxAccuracyWithKellyScaledBetsStrategyclass still contains theadjust_bet_amountmethod and usesadjusted_bet_amountin thecalculate_tradesmethod. This is inconsistent with the changes made to other classes and doesn't align with the PR objective of removing theadjust_bet_amountmethod.Consider updating this class to follow the same pattern as
MaxAccuracyBettingStrategyandKellyBettingStrategy:
- Remove the
adjust_bet_amountmethod.- Update the constructor to take a non-optional
max_bet_amountparameter.- Use
self.max_bet_amountdirectly in thecalculate_tradesmethod.Here's a suggested implementation:
class MaxAccuracyWithKellyScaledBetsStrategy(BettingStrategy): def __init__(self, max_bet_amount: float): self.max_bet_amount: float = max_bet_amount def calculate_trades( self, existing_position: Position | None, answer: ProbabilisticAnswer, market: AgentMarket, ) -> list[Trade]: outcome_token_pool = check_not_none(market.outcome_token_pool) # Fixed direction of bet, only use Kelly to adjust the bet size based on market's outcome pool size. estimated_p_yes = float(answer.p_yes > 0.5) confidence = 1.0 kelly_bet = ( get_kelly_bet_full( yes_outcome_pool_size=outcome_token_pool[ market.get_outcome_str_from_bool(True) ], no_outcome_pool_size=outcome_token_pool[ market.get_outcome_str_from_bool(False) ], estimated_p_yes=estimated_p_yes, max_bet=self.max_bet_amount, confidence=confidence, ) if market.has_token_pool() else get_kelly_bet_simplified( self.max_bet_amount, market.current_p_yes, estimated_p_yes, confidence, ) ) amounts = { market.get_outcome_str_from_bool(kelly_bet.direction): TokenAmount( amount=kelly_bet.size, currency=market.currency ), } target_position = Position(market_id=market.id, amounts=amounts) trades = self._build_rebalance_trades_from_positions( existing_position, target_position, market=market ) return trades def __repr__(self) -> str: return f"{self.__class__.__name__}(max_bet_amount={self.max_bet_amount})"This change will ensure consistency across all betting strategy classes and align with the PR objectives.
Line range hint
1-238: Summary of review findings and recommendationsThe changes made to
MaxAccuracyBettingStrategyandKellyBettingStrategyclasses align well with the PR objective of allowing dynamic bet amounts based on market conditions. The removal of theadjust_bet_amountmethod and the updates to the constructors enforce better practices for setting bet amounts.However, the
MaxAccuracyWithKellyScaledBetsStrategyclass needs to be updated to maintain consistency with the other classes and fully align with the PR objectives.To improve the overall architecture and consistency of the betting strategies:
- Update the
MaxAccuracyWithKellyScaledBetsStrategyclass as suggested in the previous comment.- Consider creating a base class for strategies that use a maximum bet amount, which could be inherited by both
KellyBettingStrategyandMaxAccuracyWithKellyScaledBetsStrategy. This would help reduce code duplication and enforce consistency in how maximum bet amounts are handled across different strategies.- Add comprehensive unit tests for each betting strategy to ensure they behave correctly with the new dynamic bet amount calculations.
These changes will result in a more consistent and maintainable codebase that fully achieves the objectives of this pull request.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- prediction_market_agent_tooling/deploy/betting_strategy.py (4 hunks)
- tests/test_betting_strategy.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/test_betting_strategy.py
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
prediction_market_agent_tooling/config.py (1)
163-171: LGTM: New get_user_id method with minor suggestions.The
get_user_idmethod is well-implemented and aligns with the PR objectives. It correctly handles different market types and includes proper error handling for unknown types.Suggestions for improvement:
- Consider using a dictionary mapping or match-case statement for better scalability as new market types are added.
- Add a docstring to explain the method's purpose and parameters.
Here's an example of how you could refactor the method using a dictionary mapping:
def get_user_id(self, market_type: MarketType) -> str: """ Get the user ID for the specified market type. Args: market_type (MarketType): The type of market to get the user ID for. Returns: str: The user ID for the specified market type. Raises: ValueError: If an unknown market type is provided. """ user_id_map = { MarketType.MANIFOLD: self.manifold_user_id, MarketType.METACULUS: str(self.metaculus_user_id), MarketType.OMEN: self.bet_from_address, MarketType.POLYMARKET: self.bet_from_address, } try: return user_id_map[market_type] except KeyError: raise ValueError(f"Unknown market type: {market_type}")This refactored version improves readability and makes it easier to add new market types in the future.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- prediction_market_agent_tooling/config.py (2 hunks)
- prediction_market_agent_tooling/deploy/agent.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/deploy/agent.py
Additional comments not posted (2)
prediction_market_agent_tooling/config.py (2)
15-15: LGTM: Import statement for MarketType.The new import statement for
MarketTypeis correctly placed and necessary for the newly addedget_user_idmethod.
15-15: Summary: Changes align with PR objectives and enhance flexibility.The introduced changes, including the new import and
get_user_idmethod, align well with the PR objectives of enabling dynamic bet amounts based on market conditions. Theget_user_idmethod provides a flexible way to retrieve user IDs for different market types, which is crucial for implementing dynamic betting strategies.These changes lay the groundwork for agents to adapt their bet sizes based on market conditions and their current balances, addressing the limitations mentioned in issue #377. The implementation is solid, with only minor suggestions for improvement in terms of scalability and documentation.
Overall, this is a well-executed step towards achieving the goal of more flexible and adaptive betting strategies.
Also applies to: 163-171
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
prediction_market_agent_tooling/monitor/markets/polymarket.py (2)
47-49: LGTM! Consider adding input validation.The new
get_user_idstatic method is a good addition that aligns with the PR objectives. It provides a clean way to retrieve the user's ID from the API keys, which can be useful for implementing dynamic bet amounts based on the user's balance.Consider adding input validation to ensure
bet_from_addressis present in theAPIKeysinstance. This could prevent potentialAttributeErrors:@staticmethod def get_user_id(api_keys: APIKeys) -> str: + if not hasattr(api_keys, 'bet_from_address'): + raise ValueError("APIKeys instance does not have 'bet_from_address' attribute") return api_keys.bet_from_address
Line range hint
1-49: Summary and Next StepsThe addition of the
get_user_idmethod is a good first step towards implementing dynamic bet amounts. However, to fully achieve the PR objectives, consider the following next steps:
- Implement the changes to
BettingStrategy.calculate_bet_amount_and_directionto accept awallet_balanceargument.- Create a mechanism to retrieve the user's current balance using the ID obtained from
get_user_id.- Update the betting logic to calculate bet amounts based on the user's current balance and market conditions.
- Add unit tests to ensure the new functionality works as expected.
- Update documentation to reflect these changes and explain how to use the new dynamic betting feature.
These steps will help ensure that the PR fully addresses the objectives outlined in issue #377 and provides a comprehensive solution for dynamic bet amounts.
prediction_market_agent_tooling/markets/manifold/manifold.py (1)
123-125: Approve with suggestions for improvementThe new
get_user_idmethod is a good addition to theManifoldAgentMarketclass. It provides a clean way to retrieve the user ID from the API keys. However, I have a few suggestions to enhance it:
- Add a return type hint for consistency:
@staticmethod def get_user_id(api_keys: APIKeys) -> str: return api_keys.manifold_user_id
- Consider adding basic error handling in case
manifold_user_idis not present in theapi_keysobject:@staticmethod def get_user_id(api_keys: APIKeys) -> str: if not hasattr(api_keys, 'manifold_user_id'): raise AttributeError("manifold_user_id not found in APIKeys") return api_keys.manifold_user_id
- Could you please clarify how this method supports the PR objectives of allowing dynamic bet amounts based on market conditions? It would be helpful to understand the broader context of this addition.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- prediction_market_agent_tooling/deploy/agent.py (3 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (1 hunks)
- prediction_market_agent_tooling/markets/metaculus/metaculus.py (2 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (1 hunks)
- prediction_market_agent_tooling/monitor/markets/polymarket.py (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- prediction_market_agent_tooling/deploy/agent.py
- prediction_market_agent_tooling/markets/agent_market.py
- prediction_market_agent_tooling/markets/omen/omen.py
Additional comments not posted (2)
prediction_market_agent_tooling/markets/metaculus/metaculus.py (2)
4-4: LGTM: New import statement for APIKeysThe addition of the import statement for
APIKeysis correct and necessary for the newget_user_idmethod. It's properly placed with other imports from the same package.
109-111: Approve new get_user_id method, but clarification neededThe new
get_user_idstatic method is implemented correctly. It provides a clean way to retrieve the Metaculus user ID from the API keys.However, could you please clarify how this method contributes to the PR's objective of allowing dynamic bet amounts based on market conditions? While it doesn't directly implement this functionality, it might be a prerequisite for user-specific operations. Some context on its intended use would be helpful.
To ensure this method is used appropriately, let's check for its usage:
An alternative to #425
Fixes #377
How does it look to use? e.g.
Pros: max flexibility
Cons: less pretty, more lines to create new agents