-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Market
class to the import statements aligns with the PR objectives, indicating its use in the newget_open_and_resolved_markets
function for handling market data. This change is straightforward and seems appropriate given the context.- 88-90: The integration of
get_open_and_resolved_markets
within themonitor_app
function 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_app
function. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_type
to handle different market types. However, it currently only supportsMarketType.MANIFOLD
with 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_dated
and the volume of market data. -
Error Handling: The function raises a
ValueError
for 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