Add get_open_and_resolved_markets function #57
Conversation
WalkthroughThe update enhances the 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: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- prediction_market_agent_tooling/monitor/monitor_app.py (3 hunks)
Additional comments: 2
prediction_market_agent_tooling/monitor/monitor_app.py (2)
- 7-10: The addition of the
Marketclass to the import statements aligns with the PR objectives, indicating its use in the newget_open_and_resolved_marketsfunction for handling market data. This change is straightforward and seems appropriate given the context.- 88-90: The integration of
get_open_and_resolved_marketswithin themonitor_appfunction demonstrates the practical application of the new function. This change simplifies the process of fetching market data by encapsulating the logic within a dedicated function, improving the readability and maintainability of themonitor_appfunction. The use of the new function here aligns with the PR objectives of enhancing modularity and potentially performance. This is a positive change that contributes to the overall objectives of the PR.
| def get_open_and_resolved_markets( | ||
| start_time: datetime, | ||
| market_type: MarketType, | ||
| ) -> tuple[list[Market], list[Market]]: | ||
| open_markets: list[Market] | ||
| resolved_markets: list[Market] | ||
|
|
||
| if market_type == MarketType.MANIFOLD: | ||
| open_markets = get_manifold_markets_dated( | ||
| oldest_date=start_time, filter_="open" | ||
| ) | ||
| resolved_markets = [ | ||
| m | ||
| for m in get_manifold_markets_dated( | ||
| oldest_date=start_time, filter_="resolved" | ||
| ) | ||
| if not m.has_unsuccessful_resolution | ||
| ] | ||
|
|
||
| elif market_type == MarketType.OMEN: | ||
| # TODO: Add Omen market support: https://github.com/gnosis/prediction-market-agent-tooling/issues/56 | ||
| open_markets = [] | ||
| resolved_markets = [] | ||
|
|
||
| else: | ||
| raise ValueError(f"Unknown market type: {market_type}") | ||
|
|
||
| return open_markets, resolved_markets |
There was a problem hiding this comment.
The get_open_and_resolved_markets function introduces a structured approach to retrieving open and resolved markets based on the market type. A few points to consider:
-
Correctness and Logic: The function correctly branches based on
market_typeto handle different market types. However, it currently only supportsMarketType.MANIFOLDwith a placeholder forMarketType.OMEN. This is a good start, but the TODO comment indicates that support for Omen markets is pending. It's important to track this as a follow-up task to ensure feature completeness. -
Performance: By separating the retrieval of open and resolved markets and filtering out markets with unsuccessful resolutions, the function potentially optimizes data processing. However, the performance impact largely depends on the implementation details of
get_manifold_markets_datedand the volume of market data. -
Error Handling: The function raises a
ValueErrorfor unknown market types, which is a good practice for handling unexpected inputs. However, consider logging this error for easier debugging and monitoring. -
Maintainability: The function is well-structured and uses clear variable names, making it maintainable. The separation of concerns (retrieving open vs. resolved markets) is well executed.
-
TODO Comment: The placeholder for Omen market support is acknowledged with a TODO comment. It's crucial to ensure this task is tracked in the project's issue tracker to not lose sight of this pending enhancement.
Overall, the function is a significant improvement in structuring market data retrieval and lays a good foundation for future enhancements.
Consider adding error logging for the ValueError raised for unknown market types to improve debuggability.
Would you like me to open a GitHub issue to track the implementation of Omen market support as indicated by the TODO comment?
Summary by CodeRabbit