Split DeployableTraderAgent into DeployablePredictionAgent
#525
Conversation
WalkthroughThis pull request introduces significant changes to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)
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: 5
🧹 Outside diff range and nitpick comments (5)
tests/markets/test_markets.py (1)
95-100: Consider adding documentation and strengthening the test.The test effectively ensures market type coverage, but could be enhanced with:
- A docstring explaining the test's purpose
- Additional assertion to verify that market types aren't supported by both agents
Here's a suggested improvement:
@pytest.mark.parametrize("market_type", list(MarketType)) def test_market_is_covered(market_type: MarketType) -> None: + """ + Verify that each market type is supported by exactly one agent type, + either DeployablePredictionAgent or DeployableTraderAgent, ensuring + complete coverage and clear separation of responsibilities. + """ + in_prediction = market_type in DeployablePredictionAgent.supported_markets + in_trader = market_type in DeployableTraderAgent.supported_markets + + assert in_prediction != in_trader, ( + f"Market {market_type} must be supported by exactly one agent type. " + f"Currently supported by: {'both' if in_prediction and in_trader else 'neither'}" + )prediction_market_agent_tooling/markets/agent_market.py (1)
241-248: Enhance docstring with implementation guidance.While the method structure is good, the docstring could be more informative by including:
- Expected behavior when
traded_marketis None- Examples of where trades might be uploaded (e.g., external systems, databases)
- Any error handling expectations
Consider expanding the docstring:
def store_trades( self, traded_market: ProcessedTradedMarket | None, keys: APIKeys ) -> None: """ - If market allows to upload trades somewhere, implement it in this method. + Upload trades to the market's storage system if supported. + + Args: + traded_market: The processed market with trades to store. If None, implementation + should handle gracefully (e.g., by logging or no-op). + keys: API keys for authentication with the storage system. + + Implementation should handle any storage errors appropriately and may upload to: + - Market platform's API + - External analytics systems + - Logging/monitoring systems """ raise NotImplementedError("Subclasses must implement this method")prediction_market_agent_tooling/markets/omen/omen.py (1)
Line range hint
438-445: Add error handling for IPFS storage.The IPFS storage operation lacks error handling. Consider wrapping it in a try-catch block to handle potential storage failures gracefully.
Apply this diff to add error handling:
if keys.enable_ipfs_upload: logger.info("Storing prediction on IPFS.") - ipfs_hash = IPFSHandler(keys).store_agent_result( - IPFSAgentResult(reasoning=reasoning) - ) - ipfs_hash_decoded = ipfscidv0_to_byte32(ipfs_hash) + try: + ipfs_hash = IPFSHandler(keys).store_agent_result( + IPFSAgentResult(reasoning=reasoning) + ) + ipfs_hash_decoded = ipfscidv0_to_byte32(ipfs_hash) + except Exception as e: + logger.error(f"Failed to store prediction on IPFS: {e}") + # Fall back to zero hash if IPFS storage fails + ipfs_hash_decoded = HexBytes(HASH_ZERO)prediction_market_agent_tooling/deploy/agent.py (2)
283-284: Ensure extensibility ofsupported_marketsattribute.The
supported_marketsattribute restricts theDeployablePredictionAgentto use only Metaculus markets. If there's a possibility of supporting additional markets for predictions without trading in the future, consider making this attribute configurable or documenting the rationale.
466-469: Include unsupportedmarket_typein the exception message for clarity.When raising the
ValueError, including the unsupportedmarket_typein the message provides better context for debugging.Apply this diff to enhance the exception message:
if market_type not in self.supported_markets: raise ValueError( - f"Only {self.supported_markets} are supported by this agent." + f"Market type {market_type} is not supported. Only {self.supported_markets} are supported by this agent." )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- prediction_market_agent_tooling/deploy/agent.py (5 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (2 hunks)
- prediction_market_agent_tooling/markets/metaculus/metaculus.py (3 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (3 hunks)
- tests/markets/test_markets.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (12)
tests/markets/test_markets.py (2)
3-6: LGTM! Import changes align with the agent split.The new imports correctly reflect the architectural changes of splitting the agent functionality into prediction and trading components.
95-100: Verify market type coverage is complete.Let's ensure we're testing against all available market types.
✅ Verification successful
Based on the results, I can now generate the final response since I have all the necessary information to verify the market type coverage.
Market type coverage is complete and correctly tested
The test is correctly parametrized with all available market types from the
MarketTypeenum:
OMENMANIFOLDPOLYMARKETMETACULUSThe test verifies that each market type is supported by either
DeployablePredictionAgent(which supports OMEN, MANIFOLD, and POLYMARKET) orDeployableTraderAgent. This ensures complete coverage and will fail if any market type is left unsupported, which is exactly what we want.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that we're testing all available market types # and that they're properly assigned to agents # Check MarketType enum definition echo "Available MarketTypes:" rg -A 10 "class MarketType" # Check agent market type assignments echo -e "\nAgent market type assignments:" rg "supported_markets\s*=" prediction_market_agent_tooling/deploy/agent.pyLength of output: 1273
prediction_market_agent_tooling/markets/metaculus/metaculus.py (2)
8-8: LGTM: Import changes are appropriate.The new imports are properly organized and directly support the new store_prediction implementation.
Also applies to: 20-20
106-108: Verify method signature changes across the codebase.Let's ensure all callers have been updated to use the new store_prediction method and no references to submit_prediction remain.
✅ Verification successful
Let me try a different approach to verify the method signature changes by using
ast-grepto find method calls and definitions more accurately.
Based on the results, I can now generate the final response as we have sufficient information about the method signature changes and their impact across the codebase.
Method signature change has been properly implemented
The verification shows:
- No references to the old
submit_predictionmethod remain in the codebase (neither definitions nor calls)- The new
store_predictionmethod is called correctly inprediction_market_agent_tooling/deploy/agent.pywith the expected parameters (processed_marketandkeys)- The change appears to be consistently implemented across the Metaculus-related classes and their usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to submit_prediction and verify store_prediction usage echo "Checking for any remaining submit_prediction references..." rg "submit_prediction" -l echo -e "\nVerifying store_prediction usage pattern..." rg "store_prediction.*ProcessedMarket" -A 2Length of output: 301
Script:
#!/bin/bash # Search for method definitions and calls using AST patterns echo "Searching for store_prediction method definitions..." ast-grep --pattern 'def store_prediction($$$)' echo -e "\nSearching for store_prediction method calls..." ast-grep --pattern '$_.store_prediction($$$)' echo -e "\nSearching for submit_prediction method definitions..." ast-grep --pattern 'def submit_prediction($$$)' echo -e "\nSearching for submit_prediction method calls..." ast-grep --pattern '$_.submit_prediction($$$)' # Also check imports and class definitions for context echo -e "\nChecking related files..." rg "class.*Metaculus" -A 5Length of output: 2832
prediction_market_agent_tooling/markets/agent_market.py (2)
32-34: LGTM! Clean class design for trade processing.The
ProcessedTradedMarketclass is well-structured, extendingProcessedMarketwith trade-specific functionality. This aligns with the PR objective of separating trading capabilities.
234-234: Verify implementations handle nullable processed_market parameter.The signature change from
ProcessedMarkettoProcessedMarket | Noneis a breaking change that requires verification in all implementing classes.✅ Verification successful
Let me gather more specific information about the implementations to verify how they handle the
processed_marketparameter.
Let me check the Polymarket implementation specifically since it wasn't shown in the previous results.
Based on the gathered information, I can now provide the final response:
Implementations correctly handle nullable processed_market parameter
The verification shows that implementing classes properly handle the nullable parameter:
- Metaculus: Implements null check with
if processed_market is not None- Omen: Has empty implementation with comment explaining prediction storage is handled in
store_trades- Omen's
store_trades: Properly handles null case with warning message- Base class: Documents the nullable parameter in the abstract method
- Polymarket: No implementation found, likely relies on
store_tradeslike OmenThe signature change is safe and all implementations either handle the null case or defer to
store_trades.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all implementations of store_prediction to verify they handle the nullable parameter # Search for classes that inherit from AgentMarket echo "Finding AgentMarket implementations..." rg -l "class \w+\((?:\w+,\s*)*AgentMarket(?:,\s*\w+)*\)" # Search for store_prediction implementations echo "Finding store_prediction implementations..." ast-grep --pattern 'def store_prediction(self, processed_market$_: $_, $_) { $$$ }'Length of output: 622
Script:
#!/bin/bash # Search for store_prediction implementations with context echo "=== Manifold implementation ===" rg "def store_prediction" prediction_market_agent_tooling/markets/manifold/manifold.py -B 1 -A 10 echo -e "\n=== Metaculus implementation ===" rg "def store_prediction" prediction_market_agent_tooling/markets/metaculus/metaculus.py -B 1 -A 10 echo -e "\n=== Omen implementation ===" rg "def store_prediction" prediction_market_agent_tooling/markets/omen/omen.py -B 1 -A 10 echo -e "\n=== Polymarket implementation ===" rg "def store_prediction" prediction_market_agent_tooling/markets/polymarket/polymarket.py -B 1 -A 10 echo -e "\n=== Jobs implementation ===" rg "def store_prediction" prediction_market_agent_tooling/jobs/jobs_models.py -B 1 -A 10Length of output: 1775
Script:
#!/bin/bash # Search for store_prediction in Polymarket with more context and different pattern echo "=== Polymarket implementation (detailed) ===" rg "store_prediction" prediction_market_agent_tooling/markets/polymarket/polymarket.py -B 2 -A 15 # Also check for any abstract method definition in the base class echo -e "\n=== Base AgentMarket class ===" rg "store_prediction" prediction_market_agent_tooling/markets/agent_market.py -B 2 -A 15Length of output: 1028
prediction_market_agent_tooling/markets/omen/omen.py (3)
28-28: LGTM!The import of
ProcessedTradedMarketis correctly added and properly organized within the import group.
Line range hint
425-462: LGTM for the overall implementation.The new
store_tradesmethod effectively combines prediction and trade storage with proper null checks and logging. The implementation is well-structured and maintains a clear separation of concerns.
421-424: Verify the impact of moving prediction storage tostore_trades.The method signature has been updated to accept nullable
processed_market, and the implementation has been moved tostore_trades. This change suggests that predictions are now stored along with trades.Let's verify that all callers of
store_predictionhave been updated to handle the nullable parameter:✅ Verification successful
Let me check the
store_tradesmethod in the Omen class to verify if the prediction storage logic has been properly moved there.
Let me try a different approach using ripgrep to find the
store_tradesimplementation.
Let me get more context of the
store_tradesimplementation to verify if it handles the prediction storage as mentioned in the docstring.
Based on the code analysis, I can now generate the final response:
The prediction storage refactoring is correctly implemented
The changes are verified and properly implemented:
- The
store_predictionmethod is correctly updated to accept nullableprocessed_market- All callers of
store_predictionhave been updated to handle the nullable parameter- The prediction storage logic is handled in
store_tradeswhich:
- Properly handles null cases with appropriate logging
- Stores the prediction reasoning in IPFS when enabled
- Uses the same nullable pattern with
traded_marketparameter🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to store_prediction to ensure they handle the nullable parameter rg -A 5 'store_prediction\('Length of output: 2295
Script:
#!/bin/bash # Search for store_trades implementation in Omen class ast-grep --pattern 'def store_trades($$$) { $$$ }'Length of output: 54
Script:
#!/bin/bash # Search for store_trades implementation in Omen class with context rg -A 10 'def store_trades' prediction_market_agent_tooling/markets/omen/omen.pyLength of output: 491
Script:
#!/bin/bash # Search for store_trades implementation with more context rg -B 5 -A 20 'def store_trades' prediction_market_agent_tooling/markets/omen/omen.pyLength of output: 1042
prediction_market_agent_tooling/deploy/agent.py (3)
39-39: ImportProcessedTradedMarketis appropriate.The addition of
ProcessedTradedMarketto the import statement ensures that the class is available for use later in the code.
411-412: Correctly initializeProcessedMarketwith the provided answer.The
ProcessedMarketobject is properly instantiated when an answer is available, ensuring that the processed market data is correctly encapsulated.
415-416: Log statement is correctly formatted using f-strings.The logging statement effectively logs the market question, URL, and answer using the enhanced f-string syntax, which aids in debugging.
| def store_prediction( | ||
| self, processed_market: ProcessedMarket | None, keys: APIKeys | ||
| ) -> None: | ||
| if processed_market is not None: | ||
| make_prediction(self.id, processed_market.answer.p_yes) | ||
| post_question_comment( | ||
| self.id, | ||
| check_not_none( | ||
| processed_market.answer.reasoning, | ||
| "Reasoning must be provided for Metaculus.", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Add explicit error handling.
The implementation could benefit from two improvements:
-
Explicit handling of the None case:
- Currently silently returns when processed_market is None
- Consider logging or raising an appropriate exception
-
Error handling for API calls:
- make_prediction and post_question_comment could fail
- Consider wrapping in try-except to handle potential API errors
Here's a suggested improvement:
def store_prediction(
self, processed_market: ProcessedMarket | None, keys: APIKeys
) -> None:
if processed_market is not None:
- make_prediction(self.id, processed_market.answer.p_yes)
- post_question_comment(
- self.id,
- check_not_none(
- processed_market.answer.reasoning,
- "Reasoning must be provided for Metaculus.",
- ),
- )
+ try:
+ make_prediction(self.id, processed_market.answer.p_yes)
+ post_question_comment(
+ self.id,
+ check_not_none(
+ processed_market.answer.reasoning,
+ "Reasoning must be provided for Metaculus.",
+ ),
+ )
+ except Exception as e:
+ raise RuntimeError(f"Failed to store prediction for market {self.id}") from e
+ else:
+ raise ValueError(f"Cannot store prediction: processed_market is None for market {self.id}")📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def store_prediction( | |
| self, processed_market: ProcessedMarket | None, keys: APIKeys | |
| ) -> None: | |
| if processed_market is not None: | |
| make_prediction(self.id, processed_market.answer.p_yes) | |
| post_question_comment( | |
| self.id, | |
| check_not_none( | |
| processed_market.answer.reasoning, | |
| "Reasoning must be provided for Metaculus.", | |
| ), | |
| ) | |
| def store_prediction( | |
| self, processed_market: ProcessedMarket | None, keys: APIKeys | |
| ) -> None: | |
| if processed_market is not None: | |
| try: | |
| make_prediction(self.id, processed_market.answer.p_yes) | |
| post_question_comment( | |
| self.id, | |
| check_not_none( | |
| processed_market.answer.reasoning, | |
| "Reasoning must be provided for Metaculus.", | |
| ), | |
| ) | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to store prediction for market {self.id}") from e | |
| else: | |
| raise ValueError(f"Cannot store prediction: processed_market is None for market {self.id}") |
| HexBytes(HexStr(i.id)) for i in traded_market.trades if i.id is not None | ||
| ] |
There was a problem hiding this comment.
Add null check for trade transaction IDs.
The list comprehension assumes that trade IDs are not None. Consider adding a filter to handle potential null IDs.
Apply this diff to safely handle null trade IDs:
- HexBytes(HexStr(i.id)) for i in traded_market.trades if i.id is not None
+ HexBytes(HexStr(trade.id))
+ for trade in traded_market.trades
+ if trade and trade.id is not None📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HexBytes(HexStr(i.id)) for i in traded_market.trades if i.id is not None | |
| ] | |
| HexBytes(HexStr(trade.id)) | |
| for trade in traded_market.trades | |
| if trade and trade.id is not None | |
| ] |
| ) -> None: | ||
| keys = APIKeys() | ||
| self.update_langfuse_trace_by_processed_market(market_type, processed_market) |
There was a problem hiding this comment.
Handle None value for processed_market when storing prediction.
Ensure that market.store_prediction can handle a None value for processed_market without raising exceptions. If it cannot, consider adding a conditional check before calling this method to prevent potential errors.
| def initialize_langfuse(self) -> None: | ||
| super().initialize_langfuse() | ||
| # Auto-observe all the methods where it makes sense, so that subclassses don't need to do it manually. | ||
| self.get_betting_strategy = observe()(self.get_betting_strategy) # type: ignore[method-assign] | ||
| self.build_trades = observe()(self.build_trades) # type: ignore[method-assign] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider alternative approaches to method observation for better maintainability.
Reassigning methods in initialize_langfuse by wrapping them with observe() may lead to maintainability issues, especially with inheritance. Consider using decorators at the method definitions or a metaclass to automatically apply observe() to the desired methods.
| match trade.trade_type: | ||
| case TradeType.BUY: | ||
| id = market.buy_tokens( | ||
| outcome=trade.outcome, amount=trade.amount | ||
| ) | ||
| case TradeType.SELL: | ||
| id = market.sell_tokens( | ||
| outcome=trade.outcome, amount=trade.amount | ||
| ) | ||
| case _: | ||
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") |
There was a problem hiding this comment.
Ensure compatibility with Python 3.10 for match statements.
The use of match statements requires Python 3.10 or later. Verify that the deployment environment supports Python 3.10+. If backward compatibility is needed, consider replacing match statements with equivalent if-elif-else constructs.
Apply this diff to replace the match statement for compatibility:
# Replace `match` statement with `if-elif-else`
- match trade.trade_type:
- case TradeType.BUY:
+ if trade.trade_type == TradeType.BUY:
id = market.buy_tokens(
outcome=trade.outcome, amount=trade.amount
)
- case TradeType.SELL:
+ elif trade.trade_type == TradeType.SELL:
id = market.sell_tokens(
outcome=trade.outcome, amount=trade.amount
)
- case _:
+ else:
raise ValueError(f"Unexpected trade type {trade.trade_type}.")📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| match trade.trade_type: | |
| case TradeType.BUY: | |
| id = market.buy_tokens( | |
| outcome=trade.outcome, amount=trade.amount | |
| ) | |
| case TradeType.SELL: | |
| id = market.sell_tokens( | |
| outcome=trade.outcome, amount=trade.amount | |
| ) | |
| case _: | |
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") | |
| if trade.trade_type == TradeType.BUY: | |
| id = market.buy_tokens( | |
| outcome=trade.outcome, amount=trade.amount | |
| ) | |
| elif trade.trade_type == TradeType.SELL: | |
| id = market.sell_tokens( | |
| outcome=trade.outcome, amount=trade.amount | |
| ) | |
| else: | |
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") |
| @@ -288,23 +291,13 @@ def __init__( | |||
| super().__init__(enable_langfuse=enable_langfuse) | |||
| self.place_bet = place_bet | |||
|
|
|||
| def get_betting_strategy(self, market: AgentMarket) -> BettingStrategy: | |||
There was a problem hiding this comment.
Everything around trading was moved to DeployableTraderAgent. DeployablePredictionAgent only does the prediction.
| @@ -436,62 +403,28 @@ def process_market( | |||
| ) -> ProcessedMarket | None: | |||
| logger.info(f"Processing market {market.question=} from {market.url=}.") | |||
|
|
|||
| self.before_process_market(market_type, market) | |||
There was a problem hiding this comment.
Moved to process_markets function, so that it will be called correctly in the DeployableTraderAgent subclass.
| if verify_market and not self.verify_market(market_type, market): | ||
| logger.info(f"Market '{market.question}' doesn't meet the criteria.") | ||
| self.update_langfuse_trace_by_processed_market(market_type, None) |
There was a problem hiding this comment.
Moved to after_process_market hook.
There was a problem hiding this comment.
Taking back, it needs to be in process_market method otherwise it won't be assigned to the correct trace in Langfuse
| @@ -528,6 +463,115 @@ def after_process_markets(self, market_type: MarketType) -> None: | |||
| "Executes actions that occur after bets are placed." | |||
|
|
|||
| def run(self, market_type: MarketType) -> None: | |||
| if market_type not in self.supported_markets: | |||
| raise ValueError( | |||
| f"Only {self.supported_markets} are supported by this agent." | |||
There was a problem hiding this comment.
I already had this in mind here #519
But now it's required here as well, because only METACULUS can be used with DeployablePredictionAgent, and rest of the markets is for DeployableTraderAgent.
| ) -> None: | ||
| api_keys = APIKeys() | ||
| super().after_process_market(market_type, market, processed_market) | ||
| if isinstance(processed_market, ProcessedTradedMarket): |
There was a problem hiding this comment.
Not nice, but idk how to do it better. after_process_market hook in this subclass will always receive ProcessedTradedMarket, but we need to accept processed_market: ProcessedMarket | ProcessedTradedMarket | None in the arguments because otherwise, mypy will it complain about subclass not being compatible with parent classs.
There was a problem hiding this comment.
Could you do:
processed_market: ProcessedMarket | None,
and then do:
assert isinstance(processed_market, ProcessedTradedMarket)
?
There was a problem hiding this comment.
Sure, that should work. | ProcessedTradedMarket in the argument typing is just to be explicit. Removed.
| ) -> None: | ||
| """ | ||
| If market allows to upload predictions somewhere, implement it in this method. | ||
| """ | ||
| raise NotImplementedError("Subclasses must implement this method") | ||
|
|
||
| def store_trades( |
There was a problem hiding this comment.
Now store_prediction is implemented on MetaculusAgentMarket and store_trades on the others.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
prediction_market_agent_tooling/deploy/agent.py (2)
413-419: Consider adding error handling for answer processing.While the code correctly handles the None case, consider adding error handling for potential exceptions during answer processing to ensure graceful failure.
- answer = self.answer_binary_market(market) - processed_market = ( - ProcessedMarket(answer=answer) if answer is not None else None - ) + try: + answer = self.answer_binary_market(market) + processed_market = ( + ProcessedMarket(answer=answer) if answer is not None else None + ) + except Exception as e: + logger.error(f"Failed to process market answer: {e}") + return None
546-565: Consider implementing transaction batching for multiple trades.When executing multiple trades, consider implementing transaction batching to reduce gas costs and improve efficiency for blockchain markets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (1)
- prediction_market_agent_tooling/deploy/agent.py (6 hunks)
🧰 Additional context used
🔇 Additional comments (3)
prediction_market_agent_tooling/deploy/agent.py (3)
285-286: LGTM! Good practice for restricting market types.The explicit declaration of supported markets enhances type safety and provides clear API boundaries.
500-508: LGTM! Well-structured betting strategy implementation.The betting strategy implementation is clean and follows good practices:
- Retrieves user position
- Calculates total amount including existing position
- Uses a clear strategy pattern
572-581: LGTM! Type-safe market processing implementation.The implementation correctly handles different types of processed markets and ensures type safety through proper type checking.
| def check_min_required_balance_to_trade(self, market: AgentMarket) -> None: | ||
| api_keys = APIKeys() | ||
|
|
||
| # Get the strategy to know how much it will bet. | ||
| strategy = self.get_betting_strategy(market) | ||
| # Have a little bandwidth after the bet. | ||
| min_required_balance_to_trade = strategy.maximum_possible_bet_amount * 1.01 | ||
|
|
||
| if market.get_trade_balance(api_keys) < min_required_balance_to_trade: | ||
| raise OutOfFundsError( | ||
| f"Minimum required balance {min_required_balance_to_trade} for agent is not met." | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding retry mechanism for balance checks.
The balance check is crucial for trading operations. Consider implementing a retry mechanism with exponential backoff for balance checks to handle temporary network issues or blockchain delays.
+ @retry(
+ stop_max_attempt_number=3,
+ wait_exponential_multiplier=1000,
+ wait_exponential_max=10000
+ )
def check_min_required_balance_to_trade(self, market: AgentMarket) -> None:
api_keys = APIKeys()Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- prediction_market_agent_tooling/deploy/agent.py (6 hunks)
🧰 Additional context used
🔇 Additional comments (1)
prediction_market_agent_tooling/deploy/agent.py (1)
491-503: 🛠️ Refactor suggestionConsider adding retry mechanism for balance checks.
The balance check is crucial for trading operations. Network issues or blockchain delays could cause temporary failures.
+ @retry( + stop_max_attempt_number=3, + wait_exponential_multiplier=1000, + wait_exponential_max=10000 + ) def check_min_required_balance_to_trade(self, market: AgentMarket) -> None:Likely invalid or redundant comment.
| processed_market = ( | ||
| ProcessedMarket(answer=answer) if answer is not None else None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider early return for None answer.
Creating a ProcessedMarket instance only to return None could be simplified.
- processed_market = (
- ProcessedMarket(answer=answer) if answer is not None else None
- )
+ if answer is None:
+ return None
+ return ProcessedMarket(answer=answer)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| processed_market = ( | |
| ProcessedMarket(answer=answer) if answer is not None else None | |
| if answer is None: | |
| return None | |
| return ProcessedMarket(answer=answer) |
| if ( | ||
| "DeployableAgent" not in str(cls.__init__) | ||
| and "DeployableTraderAgent" not in str(cls.__init__) | ||
| and "DeployablePredictionAgent" not in str(cls.__init__) | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more maintainable approach for class name checks.
The string-based class name checks could become fragile if class names change. Consider using cls.__bases__ to check inheritance directly.
- if (
- "DeployableAgent" not in str(cls.__init__)
- and "DeployableTraderAgent" not in str(cls.__init__)
- and "DeployablePredictionAgent" not in str(cls.__init__)
- ):
+ base_classes = [base.__name__ for base in cls.__mro__]
+ allowed_classes = {"DeployableAgent", "DeployableTraderAgent", "DeployablePredictionAgent"}
+ if not any(base in allowed_classes for base in base_classes):📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| "DeployableAgent" not in str(cls.__init__) | |
| and "DeployableTraderAgent" not in str(cls.__init__) | |
| and "DeployablePredictionAgent" not in str(cls.__init__) | |
| ): | |
| base_classes = [base.__name__ for base in cls.__mro__] | |
| allowed_classes = {"DeployableAgent", "DeployableTraderAgent", "DeployablePredictionAgent"} | |
| if not any(base in allowed_classes for base in base_classes): |
| placed_trades = [] | ||
| for trade in trades: | ||
| logger.info(f"Executing trade {trade} on market {market.id} ({market.url})") | ||
|
|
||
| if self.place_bet: | ||
| match trade.trade_type: | ||
| case TradeType.BUY: | ||
| id = market.buy_tokens( | ||
| outcome=trade.outcome, amount=trade.amount | ||
| ) | ||
| case TradeType.SELL: | ||
| id = market.sell_tokens( | ||
| outcome=trade.outcome, amount=trade.amount | ||
| ) | ||
| case _: | ||
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") | ||
| placed_trades.append(PlacedTrade.from_trade(trade, id)) | ||
| else: | ||
| logger.info(f"Trade execution skipped because {self.place_bet=}.") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting trade execution logic to a separate method.
The trade execution logic is complex and could benefit from being extracted into a dedicated method for better maintainability.
+ def _execute_trade(self, trade: Trade, market: AgentMarket) -> PlacedTrade | None:
+ if not self.place_bet:
+ logger.info(f"Trade execution skipped because {self.place_bet=}.")
+ return None
+
+ trade_id = None
+ if trade.trade_type == TradeType.BUY:
+ trade_id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount)
+ elif trade.trade_type == TradeType.SELL:
+ trade_id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount)
+ else:
+ raise ValueError(f"Unexpected trade type {trade.trade_type}.")
+
+ return PlacedTrade.from_trade(trade, trade_id) if trade_id else None
def process_market(...):
# ...
placed_trades = []
for trade in trades:
logger.info(f"Executing trade {trade} on market {market.id} ({market.url})")
- if self.place_bet:
- match trade.trade_type:
- case TradeType.BUY:
- id = market.buy_tokens(
- outcome=trade.outcome, amount=trade.amount
- )
- case TradeType.SELL:
- id = market.sell_tokens(
- outcome=trade.outcome, amount=trade.amount
- )
- case _:
- raise ValueError(f"Unexpected trade type {trade.trade_type}.")
- placed_trades.append(PlacedTrade.from_trade(trade, id))
- else:
- logger.info(f"Trade execution skipped because {self.place_bet=}.")
+ if placed_trade := self._execute_trade(trade, market):
+ placed_trades.append(placed_trade)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| placed_trades = [] | |
| for trade in trades: | |
| logger.info(f"Executing trade {trade} on market {market.id} ({market.url})") | |
| if self.place_bet: | |
| match trade.trade_type: | |
| case TradeType.BUY: | |
| id = market.buy_tokens( | |
| outcome=trade.outcome, amount=trade.amount | |
| ) | |
| case TradeType.SELL: | |
| id = market.sell_tokens( | |
| outcome=trade.outcome, amount=trade.amount | |
| ) | |
| case _: | |
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") | |
| placed_trades.append(PlacedTrade.from_trade(trade, id)) | |
| else: | |
| logger.info(f"Trade execution skipped because {self.place_bet=}.") | |
| def _execute_trade(self, trade: Trade, market: AgentMarket) -> PlacedTrade | None: | |
| if not self.place_bet: | |
| logger.info(f"Trade execution skipped because {self.place_bet=}.") | |
| return None | |
| trade_id = None | |
| if trade.trade_type == TradeType.BUY: | |
| trade_id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount) | |
| elif trade.trade_type == TradeType.SELL: | |
| trade_id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount) | |
| else: | |
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") | |
| return PlacedTrade.from_trade(trade, trade_id) if trade_id else None | |
| placed_trades = [] | |
| for trade in trades: | |
| logger.info(f"Executing trade {trade} on market {market.id} ({market.url})") | |
| if placed_trade := self._execute_trade(trade, market): | |
| placed_trades.append(placed_trade) |
| def after_process_market( | ||
| self, | ||
| market_type: MarketType, | ||
| market: AgentMarket, | ||
| processed_market: ProcessedMarket | ProcessedTradedMarket | None, | ||
| ) -> None: | ||
| api_keys = APIKeys() | ||
| super().after_process_market(market_type, market, processed_market) | ||
| if isinstance(processed_market, ProcessedTradedMarket): | ||
| market.store_trades(processed_market, api_keys) |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the code analysis, I can now provide a final response.
Error handling needed for IPFS upload and contract interactions
The review comment is valid. The store_trades method in Omen market performs several operations that could fail:
- IPFS file upload through
IPFSHandler - Smart contract interaction via
OmenAgentResultMappingContract().add_prediction - JSON operations and file handling
None of these operations are wrapped in try-catch blocks, which could lead to unhandled exceptions. The suggested error handling in the review comment would help gracefully handle failures in:
- Network issues during IPFS uploads
- Smart contract transaction failures
- File system operations
- JSON serialization errors
The original refactor suggestion is appropriate and should be implemented.
🔗 Analysis chain
Consider adding error handling for trade storage.
The trade storage operation could fail due to various reasons (network issues, API failures, etc.). Consider adding error handling.
def after_process_market(
self,
market_type: MarketType,
market: AgentMarket,
processed_market: ProcessedMarket | ProcessedTradedMarket | None,
) -> None:
api_keys = APIKeys()
super().after_process_market(market_type, market, processed_market)
if isinstance(processed_market, ProcessedTradedMarket):
- market.store_trades(processed_market, api_keys)
+ try:
+ market.store_trades(processed_market, api_keys)
+ except Exception as e:
+ logger.error(f"Failed to store trades: {e}")
+ # Consider if we should re-raise or handle differently
+ raiseLet me try a different approach to check the implementation of store_trades and any existing error handling.
Let me check the complete implementation of the Omen market's store_trades method to see if it has any error handling.
Let me check one more thing to see if there's any error handling in the IPFS and contract interaction parts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if store_trades has error handling in its implementation
ast-grep --pattern 'def store_trades($$$) { $$ try { $$ } catch { $$ } $$ }'
Length of output: 78
Script:
#!/bin/bash
# Search for store_trades method implementation
rg "def store_trades" -A 10
# Also search for any calls to store_trades to understand the context
rg "store_trades\(" -A 3
Length of output: 2887
Script:
#!/bin/bash
# Get the full implementation of store_trades in Omen market
rg "def store_trades" -A 30 prediction_market_agent_tooling/markets/omen/omen.py
Length of output: 1264
Script:
#!/bin/bash
# Check for try-catch blocks in the Omen market implementation
rg "try:" -A 5 -B 2 prediction_market_agent_tooling/markets/omen/omen.py
# Also check the IPFSHandler implementation
rg "def store_agent_result" -A 10 prediction_market_agent_tooling/
Length of output: 1154
| @@ -29,6 +29,9 @@ | |||
|
|
|||
| class ProcessedMarket(BaseModel): | |||
| answer: ProbabilisticAnswer | |||
|
|
|||
There was a problem hiding this comment.
Could remove ProcessedMarket class now, as it is just a wrapper around ProbabilisticAnswer without any additional functionality
There was a problem hiding this comment.
Yes, but then we have ProcessedTradedMarket which adds trades field.
For all to come nicely together, we need this ProcessedMarket, because:
DeployablePredictionAgent's process_market returns ProcessedMarket
DeployableTraderAgent's process_market returns ProcessedTradedMarket
If ProcessedTradedMarket is subclass of ProcessedMarket, then process_market methods are compatible.
If DeployablePredictionAgent's process_market starts to return only ProbabilisticAnswer, then we'd need to add trades into ProbabilisticAnswer.
But not sure if I explain this properly.. 😄 If you have some other idea how to structure it, show me please. 🙏
There was a problem hiding this comment.
Hmm I had a look, so you could alias:
ProcessedMarket = ProbabilisticAnswer
but mypy
or subclass
class ProcessedMarket(ProbabilisticAnswer):
pass
but then constructing a ProcessedTradedMarket instance from a ProcessedMarket becomes more messy if you don't want to do **processed_market.dict().
soooo okay, i retract my objection!
|
|
||
|
|
||
| @pytest.mark.parametrize("market_type", list(MarketType)) | ||
| def test_market_is_covered(market_type: MarketType) -> None: |
There was a problem hiding this comment.
I was concerned that the supported_markets check would get out of date if we added a new market type - but this test prevents that - nice!
| ) -> None: | ||
| api_keys = APIKeys() | ||
| super().after_process_market(market_type, market, processed_market) | ||
| if isinstance(processed_market, ProcessedTradedMarket): |
There was a problem hiding this comment.
Could you do:
processed_market: ProcessedMarket | None,
and then do:
assert isinstance(processed_market, ProcessedTradedMarket)
?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
prediction_market_agent_tooling/deploy/agent.py (3)
468-471: Enhance error message with more context.The error message could be more informative by explaining why only certain markets are supported.
- f"Only {self.supported_markets} are supported by this agent." + f"Only {self.supported_markets} are supported by this agent as it only handles predictions without trading capabilities."
576-581: Improve type hint specificity in after_process_market.The method only handles
ProcessedTradedMarket, but the type hint allows anyProcessedMarket. Consider making it more specific to prevent potential type-related issues.def after_process_market( self, market_type: MarketType, market: AgentMarket, - processed_market: ProcessedMarket | None, + processed_market: ProcessedTradedMarket | None, ) -> None:
280-286: Well-structured separation of prediction and trading concerns!The split between
DeployablePredictionAgentandDeployableTraderAgentwith their respective supported markets is a good architectural decision that:
- Improves separation of concerns
- Makes the code more maintainable
- Clearly defines the responsibilities of each agent type
Also applies to: 477-483
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent_tooling/markets/metaculus/metaculus.py (1)
123-131: Add docstrings to utility methods.While the implementation is correct, adding docstrings would help explain why these methods are no-ops for Metaculus:
@staticmethod def verify_operational_balance(api_keys: APIKeys) -> bool: + """ + Verify if there's sufficient balance for operations. + Always returns True as Metaculus doesn't use balance for predictions. + """ # No operational balance for Metaculus. return True @staticmethod def redeem_winnings(api_keys: APIKeys) -> None: + """ + Redeem any available winnings. + No-op as Metaculus doesn't have a concept of redeemable winnings. + """ # Nothing to redeem on Metaculus. pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prediction_market_agent_tooling/deploy/agent.py (6 hunks)
- prediction_market_agent_tooling/markets/metaculus/metaculus.py (3 hunks)
🧰 Additional context used
🔇 Additional comments (9)
prediction_market_agent_tooling/markets/metaculus/metaculus.py (3)
8-8: LGTM: Import addition is correct.The ProcessedMarket import aligns with the new store_prediction method signature.
106-117: Previous review comment about error handling is still applicable.The implementation still needs the suggested error handling improvements from the previous review.
23-23: Verify implementation of prediction interface.Let's ensure this class implements all required prediction methods from the parent class.
✅ Verification successful
Let me gather more information about the base class to properly verify the implementation.
Let me try a different approach to find the base class definition.
Based on the base class definition and the implementation shown earlier, I can now generate the final response.
Implementation correctly follows prediction-only interface
The
MetaculusAgentMarketclass properly implements the required prediction-related abstract methods fromAgentMarket:
store_prediction(): Implemented to make predictions and post comments on Metaculusget_binary_markets(): Implemented with Metaculus-specific filtering and sortingget_user_id(): Implemented to return Metaculus user IDverify_operational_balance(): Implemented to return True (no balance needed for Metaculus)redeem_winnings(): Implemented as no-op (not applicable for Metaculus)Trading-related methods like
place_bet(),buy_tokens(),sell_tokens()are not implemented, which is correct since Metaculus is a prediction-only platform.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find the base class definition to check required methods ast-grep --pattern 'class AgentMarket: $$$ ' # Find all implementations in this class ast-grep --pattern 'class MetaculusAgentMarket(AgentMarket): $$$ 'Length of output: 11008
Script:
#!/bin/bash # Find the AgentMarket base class definition and its abstract methods rg -U "class AgentMarket.*?\n\n" -A 100Length of output: 41
Script:
#!/bin/bash # Find the AgentMarket class file first fd AgentMarket # Then look for the class definition and abstract methods rg -l "class AgentMarket" | xargs catLength of output: 12873
prediction_market_agent_tooling/deploy/agent.py (6)
285-286: LGTM! Clear market support restriction.The explicit restriction to Metaculus markets for prediction-only operations is well-documented and enforced.
602-608: LGTM! Proper type checking and error handling.The type checking and conditional storing of trades is well implemented.
573-583: 🛠️ Refactor suggestionConsider Python 3.10 compatibility for match statements.
The use of match statements requires Python 3.10+. Consider using if-elif for broader compatibility.
- match trade.trade_type: - case TradeType.BUY: - id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount) - case TradeType.SELL: - id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount) - case _: - raise ValueError(f"Unexpected trade type {trade.trade_type}.") + if trade.trade_type == TradeType.BUY: + id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount) + elif trade.trade_type == TradeType.SELL: + id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount) + else: + raise ValueError(f"Unexpected trade type {trade.trade_type}.")
412-413: 🛠️ Refactor suggestionConsider early return pattern for better readability.
The creation of
processed_marketcould be simplified using an early return pattern.- processed_market = ( - ProcessedMarket(answer=answer) if answer is not None else None - ) + if answer is None: + return None + return ProcessedMarket(answer=answer)Likely invalid or redundant comment.
509-521: 🛠️ Refactor suggestionConsider adding retry mechanism for balance checks.
The balance check is crucial for trading operations. Consider implementing a retry mechanism with exponential backoff for balance checks to handle temporary network issues.
+ @retry( + stop_max_attempt_number=3, + wait_exponential_multiplier=1000, + wait_exponential_max=10000 + ) def check_min_required_balance_to_trade(self, market: AgentMarket) -> None: api_keys = APIKeys()Likely invalid or redundant comment.
169-173: 🛠️ Refactor suggestionConsider using type checking instead of string comparison.
The current implementation using string comparison is fragile and could break if class names change. Consider using Python's type system for a more robust solution.
- if ( - "DeployableAgent" not in str(cls.__init__) - and "DeployableTraderAgent" not in str(cls.__init__) - and "DeployablePredictionAgent" not in str(cls.__init__) - ): + allowed_bases = (DeployableAgent, DeployableTraderAgent, DeployablePredictionAgent) + if not any(issubclass(cls, base) for base in allowed_bases):Likely invalid or redundant comment.
| if self.place_trades: | ||
| match trade.trade_type: | ||
| case TradeType.BUY: | ||
| id = market.buy_tokens( | ||
| outcome=trade.outcome, amount=trade.amount | ||
| ) | ||
| case TradeType.SELL: | ||
| id = market.sell_tokens( | ||
| outcome=trade.outcome, amount=trade.amount | ||
| ) | ||
| case _: | ||
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") | ||
| placed_trades.append(PlacedTrade.from_trade(trade, id)) | ||
| else: | ||
| logger.info(f"Trade execution skipped because {self.place_trades=}.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract trade execution logic to a separate method.
The trade execution logic is complex and could be extracted to improve readability and maintainability.
+ def _execute_trade(self, trade: Trade, market: AgentMarket) -> PlacedTrade | None:
+ if not self.place_trades:
+ logger.info(f"Trade execution skipped because {self.place_trades=}.")
+ return None
+
+ match trade.trade_type:
+ case TradeType.BUY:
+ id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount)
+ case TradeType.SELL:
+ id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount)
+ case _:
+ raise ValueError(f"Unexpected trade type {trade.trade_type}.")
+
+ return PlacedTrade.from_trade(trade, id)
def process_market(self, market_type: MarketType, market: AgentMarket, verify_market: bool = True) -> ProcessedTradedMarket | None:
# ...
placed_trades = []
for trade in trades:
logger.info(f"Executing trade {trade} on market {market.id} ({market.url})")
- if self.place_trades:
- match trade.trade_type:
- case TradeType.BUY:
- id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount)
- case TradeType.SELL:
- id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount)
- case _:
- raise ValueError(f"Unexpected trade type {trade.trade_type}.")
- placed_trades.append(PlacedTrade.from_trade(trade, id))
- else:
- logger.info(f"Trade execution skipped because {self.place_trades=}.")
+ if placed_trade := self._execute_trade(trade, market):
+ placed_trades.append(placed_trade)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.place_trades: | |
| match trade.trade_type: | |
| case TradeType.BUY: | |
| id = market.buy_tokens( | |
| outcome=trade.outcome, amount=trade.amount | |
| ) | |
| case TradeType.SELL: | |
| id = market.sell_tokens( | |
| outcome=trade.outcome, amount=trade.amount | |
| ) | |
| case _: | |
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") | |
| placed_trades.append(PlacedTrade.from_trade(trade, id)) | |
| else: | |
| logger.info(f"Trade execution skipped because {self.place_trades=}.") | |
| if self.place_trades: | |
| if placed_trade := self._execute_trade(trade, market): | |
| placed_trades.append(placed_trade) | |
| else: | |
| logger.info(f"Trade execution skipped because {self.place_trades=}.") | |
| def _execute_trade(self, trade: Trade, market: AgentMarket) -> PlacedTrade | None: | |
| if not self.place_trades: | |
| logger.info(f"Trade execution skipped because {self.place_trades=}.") | |
| return None | |
| match trade.trade_type: | |
| case TradeType.BUY: | |
| id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount) | |
| case TradeType.SELL: | |
| id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount) | |
| case _: | |
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") | |
| return PlacedTrade.from_trade(trade, id) |
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/agent.py (6 hunks)
🧰 Additional context used
🔇 Additional comments (3)
prediction_market_agent_tooling/deploy/agent.py (3)
414-415: 🛠️ Refactor suggestionConsider early return for None answer.
The creation of ProcessedMarket instance can be simplified with an early return pattern.
- processed_market = ( - ProcessedMarket(answer=answer) if answer is not None else None - ) + if answer is None: + return None + return ProcessedMarket(answer=answer)Likely invalid or redundant comment.
574-588: 🛠️ Refactor suggestionExtract trade execution logic to a separate method.
The trade execution logic is complex and could be extracted to improve readability and maintainability.
+ def _execute_trade(self, trade: Trade, market: AgentMarket) -> PlacedTrade | None: + if not self.place_trades: + logger.info(f"Trade execution skipped because {self.place_trades=}.") + return None + + match trade.trade_type: + case TradeType.BUY: + id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount) + case TradeType.SELL: + id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount) + case _: + raise ValueError(f"Unexpected trade type {trade.trade_type}.") + + return PlacedTrade.from_trade(trade, id) def process_market(...): # ... placed_trades = [] for trade in trades: logger.info(f"Executing trade {trade} on market {market.id} ({market.url})") - if self.place_trades: - match trade.trade_type: - case TradeType.BUY: - id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount) - case TradeType.SELL: - id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount) - case _: - raise ValueError(f"Unexpected trade type {trade.trade_type}.") - placed_trades.append(PlacedTrade.from_trade(trade, id)) - else: - logger.info(f"Trade execution skipped because {self.place_trades=}.") + if placed_trade := self._execute_trade(trade, market): + placed_trades.append(placed_trade)Likely invalid or redundant comment.
169-173: 🛠️ Refactor suggestionConsider using type checking instead of string comparison.
The current implementation using string comparison is fragile and could break if class names change. Consider using Python's type system for a more robust solution.
- if ( - "DeployableAgent" not in str(cls.__init__) - and "DeployableTraderAgent" not in str(cls.__init__) - and "DeployablePredictionAgent" not in str(cls.__init__) - ): + base_classes = [base.__name__ for base in cls.__mro__] + allowed_classes = {"DeployableAgent", "DeployableTraderAgent", "DeployablePredictionAgent"} + if not any(base in allowed_classes for base in base_classes):Likely invalid or redundant comment.
| if self.store_trades: | ||
| market.store_trades(processed_market, api_keys) | ||
| else: | ||
| logger.info( | ||
| f"Trades {processed_market.trades} not stored because {self.store_trades=}." | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for trade storage operations.
The trade storage operation could fail due to various reasons (network issues, API failures, etc.). Consider adding error handling.
if isinstance(processed_market, ProcessedTradedMarket):
if self.store_trades:
- market.store_trades(processed_market, api_keys)
+ try:
+ market.store_trades(processed_market, api_keys)
+ except Exception as e:
+ logger.error(f"Failed to store trades: {e}")
+ # Consider if we should re-raise or handle differently
+ raise
else:
logger.info(
f"Trades {processed_market.trades} not stored because {self.store_trades=}."
)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.store_trades: | |
| market.store_trades(processed_market, api_keys) | |
| else: | |
| logger.info( | |
| f"Trades {processed_market.trades} not stored because {self.store_trades=}." | |
| ) | |
| if self.store_trades: | |
| try: | |
| market.store_trades(processed_market, api_keys) | |
| except Exception as e: | |
| logger.error(f"Failed to store trades: {e}") | |
| # Consider if we should re-raise or handle differently | |
| raise | |
| else: | |
| logger.info( | |
| f"Trades {processed_market.trades} not stored because {self.store_trades=}." | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- prediction_market_agent_tooling/deploy/agent.py (6 hunks)
🔇 Additional comments (4)
prediction_market_agent_tooling/deploy/agent.py (4)
474-477: LGTM! Clear market type validation.The validation ensures that only supported market types are processed, with a clear error message.
603-609: 🛠️ Refactor suggestionAdd error handling for trade storage.
The trade storage operation could fail due to various reasons (network issues, API failures, etc.).
if isinstance(processed_market, ProcessedTradedMarket): if self.store_trades: + try: market.store_trades(processed_market, api_keys) + except Exception as e: + logger.error(f"Failed to store trades: {e}") + raise else: logger.info( f"Trades {processed_market.trades} not stored because {self.store_trades=}." )Likely invalid or redundant comment.
169-173: 🛠️ Refactor suggestionConsider using
__mro__for class inheritance checks.The current string-based class name checks are fragile and could break if class names change. Using Python's Method Resolution Order (MRO) would be more robust.
- if ( - "DeployableAgent" not in str(cls.__init__) - and "DeployableTraderAgent" not in str(cls.__init__) - and "DeployablePredictionAgent" not in str(cls.__init__) - ): + base_classes = [base.__name__ for base in cls.__mro__] + allowed_classes = {"DeployableAgent", "DeployableTraderAgent", "DeployablePredictionAgent"} + if not any(base in allowed_classes for base in base_classes):Likely invalid or redundant comment.
573-587: 🛠️ Refactor suggestionExtract trade execution logic to a separate method.
The trade execution logic is complex and could be extracted to improve readability and maintainability.
+ def _execute_trade(self, trade: Trade, market: AgentMarket) -> PlacedTrade | None: + if not self.place_trades: + logger.info(f"Trade execution skipped because {self.place_trades=}.") + return None + + match trade.trade_type: + case TradeType.BUY: + id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount) + case TradeType.SELL: + id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount) + case _: + raise ValueError(f"Unexpected trade type {trade.trade_type}.") + + return PlacedTrade.from_trade(trade, id) def process_market(...): # ... for trade in trades: logger.info(f"Executing trade {trade} on market {market.id} ({market.url})") - if self.place_trades: - match trade.trade_type: - case TradeType.BUY: - id = market.buy_tokens(outcome=trade.outcome, amount=trade.amount) - case TradeType.SELL: - id = market.sell_tokens(outcome=trade.outcome, amount=trade.amount) - case _: - raise ValueError(f"Unexpected trade type {trade.trade_type}.") - placed_trades.append(PlacedTrade.from_trade(trade, id)) - else: - logger.info(f"Trade execution skipped because {self.place_trades=}.") + if placed_trade := self._execute_trade(trade, market): + placed_trades.append(placed_trade)Likely invalid or redundant comment.
| processed_market: ProcessedMarket | None, | ||
| ) -> None: | ||
| keys = APIKeys() | ||
| market.store_prediction(processed_market=processed_market, keys=keys) | ||
| if self.store_prediction: | ||
| market.store_prediction(processed_market=processed_market, keys=keys) | ||
| else: | ||
| logger.info( | ||
| f"Prediction {processed_market} not stored because {self.store_prediction=}." | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for prediction storage.
The prediction storage operation could fail due to various reasons (network issues, API failures, etc.).
if self.store_prediction:
+ try:
market.store_prediction(processed_market=processed_market, keys=keys)
+ except Exception as e:
+ logger.error(f"Failed to store prediction: {e}")
+ raise
else:
logger.info(
f"Prediction {processed_market} not stored because {self.store_prediction=}."
)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| processed_market: ProcessedMarket | None, | |
| ) -> None: | |
| keys = APIKeys() | |
| market.store_prediction(processed_market=processed_market, keys=keys) | |
| if self.store_prediction: | |
| market.store_prediction(processed_market=processed_market, keys=keys) | |
| else: | |
| logger.info( | |
| f"Prediction {processed_market} not stored because {self.store_prediction=}." | |
| ) | |
| processed_market: ProcessedMarket | None, | |
| ) -> None: | |
| keys = APIKeys() | |
| if self.store_prediction: | |
| try: | |
| market.store_prediction(processed_market=processed_market, keys=keys) | |
| except Exception as e: | |
| logger.error(f"Failed to store prediction: {e}") | |
| raise | |
| else: | |
| logger.info( | |
| f"Prediction {processed_market} not stored because {self.store_prediction=}." | |
| ) |
| self.update_langfuse_trace_by_market(market_type, market) | ||
| logger.info(f"Processing market {market.question=} from {market.url=}.") | ||
|
|
||
| self.before_process_market(market_type, market) | ||
|
|
||
| answer: ProbabilisticAnswer | None | ||
| if verify_market and not self.verify_market(market_type, market): | ||
| logger.info(f"Market '{market.question}' doesn't meet the criteria.") | ||
| self.update_langfuse_trace_by_processed_market(market_type, None) | ||
| return None | ||
|
|
||
| answer = self.answer_binary_market(market) | ||
|
|
||
| if answer is None: | ||
| logger.info(f"No answer for market '{market.question}'.") | ||
| self.update_langfuse_trace_by_processed_market(market_type, None) | ||
| return None | ||
| answer = None | ||
| else: | ||
| answer = self.answer_binary_market(market) | ||
|
|
||
| existing_position = market.get_position(user_id=APIKeys().bet_from_address) | ||
| trades = self.build_trades( | ||
| market=market, | ||
| answer=answer, | ||
| existing_position=existing_position, | ||
| processed_market = ( | ||
| ProcessedMarket(answer=answer) if answer is not None else None | ||
| ) | ||
|
|
||
| placed_trades = [] | ||
| for trade in trades: | ||
| logger.info(f"Executing trade {trade} on market {market.id} ({market.url})") | ||
|
|
||
| if self.place_bet: | ||
| match trade.trade_type: | ||
| case TradeType.BUY: | ||
| id = market.buy_tokens( | ||
| outcome=trade.outcome, amount=trade.amount | ||
| ) | ||
| case TradeType.SELL: | ||
| id = market.sell_tokens( | ||
| outcome=trade.outcome, amount=trade.amount | ||
| ) | ||
| case _: | ||
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") | ||
| placed_trades.append(PlacedTrade.from_trade(trade, id)) | ||
| else: | ||
| logger.info(f"Trade execution skipped because {self.place_bet=}.") | ||
|
|
||
| processed_market = ProcessedMarket(answer=answer, trades=placed_trades) | ||
| self.update_langfuse_trace_by_processed_market(market_type, processed_market) | ||
|
|
||
| self.after_process_market( | ||
| market_type, market, processed_market=processed_market | ||
| logger.info( | ||
| f"Processed market {market.question=} from {market.url=} with {answer=}." |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for market processing.
The market processing logic should handle potential exceptions from answer_binary_market to ensure graceful failure.
self.update_langfuse_trace_by_market(market_type, market)
logger.info(f"Processing market {market.question=} from {market.url=}.")
answer: ProbabilisticAnswer | None
if verify_market and not self.verify_market(market_type, market):
logger.info(f"Market '{market.question}' doesn't meet the criteria.")
answer = None
else:
+ try:
answer = self.answer_binary_market(market)
+ except Exception as e:
+ logger.error(f"Failed to answer market: {e}")
+ answer = None📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.update_langfuse_trace_by_market(market_type, market) | |
| logger.info(f"Processing market {market.question=} from {market.url=}.") | |
| self.before_process_market(market_type, market) | |
| answer: ProbabilisticAnswer | None | |
| if verify_market and not self.verify_market(market_type, market): | |
| logger.info(f"Market '{market.question}' doesn't meet the criteria.") | |
| self.update_langfuse_trace_by_processed_market(market_type, None) | |
| return None | |
| answer = self.answer_binary_market(market) | |
| if answer is None: | |
| logger.info(f"No answer for market '{market.question}'.") | |
| self.update_langfuse_trace_by_processed_market(market_type, None) | |
| return None | |
| answer = None | |
| else: | |
| answer = self.answer_binary_market(market) | |
| existing_position = market.get_position(user_id=APIKeys().bet_from_address) | |
| trades = self.build_trades( | |
| market=market, | |
| answer=answer, | |
| existing_position=existing_position, | |
| processed_market = ( | |
| ProcessedMarket(answer=answer) if answer is not None else None | |
| ) | |
| placed_trades = [] | |
| for trade in trades: | |
| logger.info(f"Executing trade {trade} on market {market.id} ({market.url})") | |
| if self.place_bet: | |
| match trade.trade_type: | |
| case TradeType.BUY: | |
| id = market.buy_tokens( | |
| outcome=trade.outcome, amount=trade.amount | |
| ) | |
| case TradeType.SELL: | |
| id = market.sell_tokens( | |
| outcome=trade.outcome, amount=trade.amount | |
| ) | |
| case _: | |
| raise ValueError(f"Unexpected trade type {trade.trade_type}.") | |
| placed_trades.append(PlacedTrade.from_trade(trade, id)) | |
| else: | |
| logger.info(f"Trade execution skipped because {self.place_bet=}.") | |
| processed_market = ProcessedMarket(answer=answer, trades=placed_trades) | |
| self.update_langfuse_trace_by_processed_market(market_type, processed_market) | |
| self.after_process_market( | |
| market_type, market, processed_market=processed_market | |
| logger.info( | |
| f"Processed market {market.question=} from {market.url=} with {answer=}." | |
| self.update_langfuse_trace_by_market(market_type, market) | |
| logger.info(f"Processing market {market.question=} from {market.url=}.") | |
| answer: ProbabilisticAnswer | None | |
| if verify_market and not self.verify_market(market_type, market): | |
| logger.info(f"Market '{market.question}' doesn't meet the criteria.") | |
| answer = None | |
| else: | |
| try: | |
| answer = self.answer_binary_market(market) | |
| except Exception as e: | |
| logger.error(f"Failed to answer market: {e}") | |
| answer = None | |
| processed_market = ( | |
| ProcessedMarket(answer=answer) if answer is not None else None | |
| ) | |
| self.update_langfuse_trace_by_processed_market(market_type, processed_market) | |
| logger.info( | |
| f"Processed market {market.question=} from {market.url=} with {answer=}." |
No description provided.