Enable monitoring of Omen market #59
Conversation
…et implementation
WalkthroughThe recent updates focus on enhancing the prediction market agent tooling by adding new functionalities and refining existing ones across various prediction markets like Manifold and Omen. Key improvements include the introduction of new tests and functions for market and bet handling, refining data models, and updating market retrieval and monitoring logic. The changes aim to improve the efficiency and capabilities of prediction market analysis and operations, streamlining data handling and expanding the toolset for market and bet evaluation. 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: 4
Configuration used: CodeRabbit UI
Files selected for processing (12)
- prediction_market_agent_tooling/benchmark/utils.py (1 hunks)
- prediction_market_agent_tooling/deploy/agent.py (2 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (3 hunks)
- prediction_market_agent_tooling/markets/data_models.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold/api.py (2 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (2 hunks)
- prediction_market_agent_tooling/markets/markets.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (4 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (7 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (4 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (3 hunks)
- tests/markets/test_omen.py (2 hunks)
Additional comments: 37
prediction_market_agent_tooling/markets/markets.py (1)
- 16-18: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-13]
The code defines
MarketTypeenum and a mapping dictionaryMARKET_TYPE_MAPcorrectly. It follows Python best practices for enum and type hinting. The removal ofget_binary_marketsfunction aligns with the PR objectives to refactor market retrieval logic. No issues or improvements are identified in this segment.prediction_market_agent_tooling/markets/data_models.py (1)
- 17-17: The addition of the
MKTvalue to theResolutionenum is correctly implemented. This change expands the possible resolutions for markets, which could be necessary for handling new types of market outcomes. Ensure that all parts of the application that use theResolutionenum are updated to handle this new value appropriately.prediction_market_agent_tooling/markets/agent_market.py (7)
- 2-2: The import of
datetimeis correctly added to support the newcreated_timefield in theAgentMarketclass. This is a necessary change for handling market creation times.- 4-4: The import of
Enumis correctly added to support the definition of new enums within this file. Enums are a good choice for defining a set of named constants, making the code more readable and maintainable.- 8-12: The imports from
data_models.pyare correctly added to support the new fields in theAgentMarketclass. This ensures that the class can use the defined data models and enums, promoting code reuse and consistency.- 15-22: The addition of
SortByandFilterByenums is correctly implemented. These enums will facilitate sorting and filtering operations on markets, making the code more readable and maintainable. Ensure that these enums are utilized wherever sorting and filtering of markets are performed.- 35-37: The addition of the
resolution,created_time, andp_yesfields to theAgentMarketclass is correctly implemented. These fields are essential for handling market resolutions, creation times, and probabilities, respectively. Ensure that all parts of the application that instantiate or useAgentMarketobjects are updated to handle these new fields appropriately.- 49-54: The modification of the
get_binary_marketsmethod signature to include additional parameters (sort_by,filter_by,created_after) is a significant improvement. It allows for more granular control over market retrieval, aligning with the PR objectives to enhance market handling capabilities. Ensure that all calls to this method are updated to pass the necessary arguments.- 57-61: The addition of
is_resolvedandhas_successful_resolutionmethods to theAgentMarketclass is correctly implemented. These methods provide a convenient way to check the resolution status of markets, improving code readability and maintainability. Ensure that these methods are utilized wherever market resolution status checks are performed.prediction_market_agent_tooling/markets/manifold/manifold.py (4)
- 2-2: The import of
datetimeis correctly added to support the handling of market creation times. This is consistent with the changes in other parts of the application.- 6-10: The imports for
FilterByandSortByenums are correctly added to support the enhanced market retrieval logic. This ensures consistency in how markets are sorted and filtered across the application.- 44-46: The addition of
resolution,created_time, andp_yesattributes in thefrom_data_modelmethod is correctly implemented. These attributes are essential for theManifoldAgentMarketclass to align with the updatedAgentMarketclass structure. Ensure that theManifoldMarketdata model provides these attributes correctly.- 50-77: The modification of the
get_binary_marketsmethod to acceptsort_by,filter_by, andcreated_afterparameters is a significant improvement. It aligns with the PR objectives to enhance market retrieval logic. The implementation of sorting and filtering logic based on the provided enums and the handling of pagination are correctly done. Ensure that all calls to this method are updated to pass the necessary arguments and that the pagination logic correctly handles all edge cases.tests/markets/test_omen.py (3)
- 9-9: The import of
FilterByandSortByfromprediction_market_agent_tooling.markets.agent_marketis correctly added to support the new market retrieval logic in the tests. This ensures that the tests can utilize the same sorting and filtering capabilities as the rest of the application.- 16-16: The addition of the
get_omen_binary_marketsfunction import is correctly done to support the new test case. This function is essential for retrieving Omen markets to be used in the test.- 77-88: The new test case
test_p_yesis correctly implemented to verify the correctness of thep_yescalculation in Omen markets. The test iterates over markets retrieved byget_omen_binary_markets, looking for a market withoutcomeTokenProbabilities, and then asserts thatp_yesmatches the expected probability. This test enhances the test coverage for the new functionality related to Omen markets. Ensure that the test environment is correctly set up to run this test, including any necessary mock data or API responses.prediction_market_agent_tooling/monitor/monitor_app.py (4)
- 7-10: The imports for
AgentMarket,FilterBy, andSortByare correctly added to support the updated market handling approach in the monitoring application. This ensures that the application can utilize the enhanced market retrieval and filtering capabilities.- 12-12: The import of
MARKET_TYPE_MAPandMarketTypefromprediction_market_agent_tooling.markets.marketsis correctly done to support the dynamic handling of market types in the monitoring application. This promotes code reuse and consistency across the application.- 25-25: The introduction of the
MAX_MONITOR_MARKETSconstant is a good practice for limiting the number of markets monitored. This helps in managing performance and resource utilization. Ensure that this constant is used appropriately wherever market retrieval is performed.- 46-63: The
get_open_and_resolved_marketsfunction has been updated to retrieve markets based onAgentMarketmethods, with adjustments for market type handling and filtering. The use ofMARKET_TYPE_MAPto dynamically select the class based onmarket_typeand the subsequent retrieval of open and resolved markets using the updatedget_binary_marketsmethod are correctly implemented. Ensure that all market types supported by the application are correctly mapped inMARKET_TYPE_MAPand that the filtering and sorting parameters passed toget_binary_marketsare appropriate for the intended monitoring functionality.prediction_market_agent_tooling/deploy/agent.py (4)
- 13-13: The import of
AgentMarketandSortByis correctly added to support the enhanced market handling capabilities in the deployment logic. This ensures that the deployment logic can utilize the same market sorting capabilities as the rest of the application.- 14-14: The import of
MARKET_TYPE_MAPandMarketTypeis correctly done to support dynamic handling of market types in the deployment logic. This promotes code reuse and consistency across the application.- 16-16: The introduction of the
MAX_AVAILABLE_MARKETSconstant is a good practice for limiting the number of markets considered during deployment. This helps in managing performance and resource utilization. Ensure that this constant is used appropriately wherever market retrieval is performed in the deployment logic.- 115-122: The refactoring of market type handling and market retrieval logic in the
runmethod is correctly implemented. The use ofMARKET_TYPE_MAPto dynamically select the class based onmarket_typeand the subsequent retrieval of markets using the updatedget_binary_marketsmethod with theMAX_AVAILABLE_MARKETSlimit andSortBy.CLOSING_SOONESTsorting are well done. This enhances the flexibility and functionality of the deployment logic. Ensure that all market types supported by the application are correctly mapped inMARKET_TYPE_MAPand that the sorting and filtering parameters passed toget_binary_marketsare appropriate for the intended deployment functionality.prediction_market_agent_tooling/markets/omen/data_models.py (4)
- 20-20: The addition of the
Resolutionentity to the import statements indicates the introduction of a new enum or class to represent market resolutions. This is a positive change as it likely enhances the clarity and type safety of the code dealing with market resolutions.- 52-55: The addition of optional timestamp properties (
resolutionTimestamp,answerFinalizedTimestamp,currentAnswer,creationTimestamp) to theOmenMarketclass is a significant enhancement. These properties will allow for more detailed tracking and analysis of market states over time. Ensure that the use ofintfor timestamps aligns with the expected timestamp format (likely UNIX epoch time in seconds) from the data source.- 57-65: The
is_openandis_resolvedproperties provide a clear and concise way to determine the state of a market. The logic appears sound, withis_openchecking for the absence of acurrentAnswerandis_resolvedrequiring bothanswerFinalizedTimestampandcurrentAnswerto be non-None. This approach is logical and should work well in practice.- 163-163: The addition of the
resolutionTimestampproperty to theOmenBetFPMMclass aligns with the updates made to theOmenMarketclass, ensuring consistency in handling resolution timestamps across different models. This is a good practice for maintaining a coherent data model.prediction_market_agent_tooling/benchmark/utils.py (1)
- 258-263: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of the
get_manifold_markets_datedfunction fromutils.pyindicates a shift in the approach to market data retrieval and filtering. While the summary does not provide details on the replacement strategy or method, it's important to ensure that the functionality provided by the removed function is either no longer needed or has been adequately replaced by the new implementation. This change could impact how the tool interacts with Manifold markets, especially in terms of filtering markets by date. Ensure that any new or updated functions provide equivalent or improved functionality.prediction_market_agent_tooling/markets/omen/omen.py (8)
- 1-11: The addition of imports for
json,os,random,Enum,requests,Web3, andTxParamsis appropriate given the new functionality being introduced. Ensure that all imported modules are used within the file to avoid unnecessary imports.- 58-60: The introduction of constants
OMEN_QUERY_BATCH_SIZE,OMEN_DEFAULT_MARKET_FEE, andDEFAULT_COLLATERAL_TOKEN_CONTRACT_ADDRESSis clear and well-documented. These constants are crucial for the operation of the Omen market functionality.- 96-124: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [61-101]
The modifications to the
OmenAgentMarketclass, including the addition of new parameters and logic in its methods, are well-structured. However, ensure that theplace_betmethod's error handling for currency mismatch is tested thoroughly, as it directly affects transaction integrity.
- 105-118: The
get_binary_marketsmethod's implementation for retrieving and filtering markets based on provided criteria is logically sound. It's important to ensure that theget_omen_binary_marketsfunction, which this method relies on, correctly handles the parameters and returns the expected results.Verification successful
The
get_omen_binary_marketsfunction correctly handles the parameterslimit,sort_by,filter_by, andcreated_afterby passing them to theget_omen_marketsfunction with specific adjustments for binary markets, such as specifying outcomes. This suggests that the implementation is logically sound, assuming theget_omen_marketsfunction is designed to handle these parameters effectively. Without direct verification of theget_omen_marketsfunction's implementation, we assume it correctly supports the intended functionality based on the context provided.* 226-248: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [229-260]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct handling of parameters and expected results in get_omen_binary_markets function. echo "Review the get_omen_binary_markets function for correct parameter handling and expected outcomes."Length of output: 204
Script:
#!/bin/bash # Search for the get_omen_binary_markets function definition within the codebase rg "def get_omen_binary_markets" -A 20Length of output: 1732
The GraphQL query strings for market retrieval are well-formed. However, it's crucial to ensure that these queries are tested against the actual GraphQL endpoint to verify their correctness and efficiency, especially considering the potential for large data volumes.
- 276-285: The
ordering_from_sort_byfunction provides a straightforward mapping fromSortByenums to GraphQL query parameters. Ensure that all possibleSortByvalues are handled and tested to prevent runtime errors.- 337-340: The
pick_binary_marketfunction provides a convenient way to retrieve a single market based on sorting and filtering criteria. Ensure that this function is covered by tests to validate its behavior in various scenarios.Verification successful
The
pick_binary_marketfunction is covered by at least one test, as shown intests/markets/test_omen.py, which checks for the presence of two specific outcomes, "Yes" and "No". This confirms that there is some level of testing for the function, aligning with the suggestion to ensure it is covered by tests. However, the comprehensiveness of the test coverage across various scenarios remains unclear based on the provided information.* 757-757: The GraphQL query for retrieving market trades is correctly structured. It's important to ensure that this query is tested for performance and correctness, given its potential impact on data retrieval efficiency.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Test the pick_binary_market function to validate its behavior under various scenarios. echo "Ensure comprehensive testing of the pick_binary_market function."Length of output: 138
Script:
#!/bin/bash # Search for test files mentioning the pick_binary_market function rg "pick_binary_market" --type md -g '*test*.py'Length of output: 445
Script:
#!/bin/bash # Display the content of the test function for pick_binary_market rg "def test_omen_pick_binary_market" -A 20 tests/markets/test_omen.pyLength of output: 906
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (6)
- prediction_market_agent_tooling/deploy/agent.py (2 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (3 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (2 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (4 hunks)
- tests/markets/test_betting_strategies.py (3 hunks)
- tests/markets/test_omen.py (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- prediction_market_agent_tooling/markets/agent_market.py
- prediction_market_agent_tooling/markets/manifold/manifold.py
- prediction_market_agent_tooling/markets/omen/data_models.py
- tests/markets/test_omen.py
Additional comments: 3
tests/markets/test_betting_strategies.py (1)
- 1-1: The addition of the
datetimeimport is appropriate for supporting the newcreated_timeparameter in test functions.prediction_market_agent_tooling/deploy/agent.py (2)
- 13-17: The import modifications and the addition of the
MAX_AVAILABLE_MARKETSconstant are well-aligned with the PR's objectives to enhance market monitoring capabilities. The constant helps in defining a clear limit for market retrieval, which can aid in performance optimization.- 122-129: The refactoring of market type handling and market retrieval logic is a significant improvement. It leverages the
MARKET_TYPE_MAPto dynamically fetch market types and uses theMAX_AVAILABLE_MARKETSconstant to limit the number of markets retrieved. This approach enhances modularity and maintainability. However, ensure that theget_binary_marketsmethod and theMARKET_TYPE_MAPare correctly implemented to handle the dynamic nature of market types without introducing type safety issues or runtime errors.Verification successful
To further verify the implementation details of
get_binary_marketsand to view the completeMARKET_TYPE_MAP, additional information is required.
The verification process has confirmed that the
get_binary_marketsmethod is correctly implemented in theOmenAgentMarketandManifoldAgentMarketclasses, designed to handle the dynamic nature of market types effectively. Additionally, theMARKET_TYPE_MAPis correctly and completely implemented, mapping market types to their respective classes, which supports the dynamic fetching of market types as intended. These findings align with the observations made in the review comment about enhancing modularity and maintainability.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of get_binary_markets in each market type class rg "def get_binary_markets" # Verify the correctness and completeness of MARKET_TYPE_MAP cat prediction_market_agent_tooling/markets/markets.py | grep "MARKET_TYPE_MAP"Length of output: 420
Script:
#!/bin/bash # Extract the implementation details of get_binary_markets from each class cat prediction_market_agent_tooling/markets/agent_market.py | grep -A 10 "def get_binary_markets" cat prediction_market_agent_tooling/markets/omen/omen.py | grep -A 10 "def get_binary_markets" cat prediction_market_agent_tooling/markets/manifold/manifold.py | grep -A 10 "def get_binary_markets" # View the complete MARKET_TYPE_MAP cat prediction_market_agent_tooling/markets/markets.py | grep -A 10 "MARKET_TYPE_MAP"Length of output: 1612
| @@ -179,6 +237,8 @@ def to_generic_resolved_bet(self) -> ResolvedBet: | |||
| created_time=datetime.fromtimestamp(self.creationTimestamp), | |||
| market_question=self.title, | |||
| market_outcome=self.fpmm.boolean_outcome, | |||
| resolved_time=datetime.fromtimestamp(self.fpmm.answerFinalizedTimestamp), # type: ignore # TODO Mypy doesn't understand that self.fpmm.is_resolved is True and therefore timestamp is known non-None | |||
| resolved_time=datetime.fromtimestamp( | |||
There was a problem hiding this comment.
Left over fix from previous PR!
|
|
||
| markets = [ManifoldMarket.model_validate(x) for x in data] | ||
| return markets | ||
| return all_markets[:limit] |
There was a problem hiding this comment.
I know get_manifold_markets, get_manifold_markets_paged and get_manifold_markets_dated from the benchmark code are a little unpractical, but that's because of API limitations; I think I liked that more. Could that code be moved over here? Because here we will have to do some runtime checks. But if I am too much biased as I wrote that code, please resist 😄
There was a problem hiding this comment.
Haha ohhhhh I thought it was nice to get merge the 3 functions into 1. Makes sense to remove get_manifold_markets and always replace its use with get_manifold_markets_paged, but what don't you like about combining with get_manifold_markets_dated as well?
There was a problem hiding this comment.
I was worried that if we do if created_after and market.createdTime < created_after: without enforcing "newest" sort, it's not clear how much time this function will take. As the markets can be sorted arbitrarily.
If we implement created_after like this, there is no reason to not have created_before as well, but then, with a big enough limit, the function could iterate the whole market database.
That's why I liked the previous split that was based on the API limitations.
However, if you don't think that's a real problem, maybe I'm just too worried so feel free to leave it like this.
There was a problem hiding this comment.
Yeah good point. I guess I'm saying we can leave it up to the user of AgentMarket.get_binary_markets to call the function responsibly. If you sort by newest, then atm the performance should be the exact same as with get_manifold_markets_dated. In terms of timing, looks like the penalty of iterating through the markets inefficiently (i.e. if markets are sorted by date) is small relative to overall time to get the response from manifold in the first place:
import time
from prediction_market_agent_tooling.markets.manifold.api import get_manifold_binary_markets
def time_get_markets(limit: int, sort: str):
start_time = time.time()
markets = get_manifold_binary_markets(limit=limit, sort=sort, filter_="resolved")
print(f"Time for {sort}-sorted:", time.time() - start_time)
assert len(markets) == limit
limit = 5000
time_get_markets(limit, "newest")
time_get_markets(limit, "liquidity")
Running a few times:
(.venv) prediction-market-agent-tooling % python ~/scratch/get_mani_time.py
Time for newest-sorted: 3.6355371475219727
(.venv) prediction-market-agent-tooling % python ~/scratch/get_mani_time.py
Time for newest-sorted: 9.79013991355896
Time for liquidity-sorted: 4.252532958984375
(.venv) prediction-market-agent-tooling % python ~/scratch/get_mani_time.py
Time for newest-sorted: 3.142300844192505
Time for liquidity-sorted: 3.7035253047943115
(.venv) prediction-market-agent-tooling % python ~/scratch/get_mani_time.py
Time for newest-sorted: 3.22481107711792
Time for liquidity-sorted: 3.2679333686828613
| true_index = self.outcomes.index(OMEN_TRUE_OUTCOME) | ||
|
|
||
| if sum(self.outcomeTokenAmounts) == 0: | ||
| return Probability(0.5) |
| if filter_by == FilterBy.OPEN: | ||
| return [m for m in omen_markets if m.is_open] | ||
| elif filter_by == FilterBy.RESOLVED: | ||
| return [m for m in omen_markets if m.is_resolved] |
There was a problem hiding this comment.
Seems like they support where, can't that be used?
There was a problem hiding this comment.
You would think so! I had a look through the options in the docs (https://api.thegraph.com/subgraphs/name/protofire/omen-xdai/graphql?query=query+MyQuery+%7B%0A++fixedProductMarketMakers%28where%3A+%7B%7D%29%0A%7D) but couldn't find a suitable one
There was a problem hiding this comment.
Seems like
query getFixedProductMarketMakers($outcomes: [String!]) {
fixedProductMarketMakers(
where: {
outcomes: $outcomes,
resolutionTimestamp_not: null
}
) {
id
creator
title
}
}
works to get resolved markets.
However, resolutionTimestamp: null can return non-resolved, but already closed markets (markets that are closed but not yet resolved on realitio, so we can't use it to get true outcome), so that's not what we want here.
But then
query getFixedProductMarketMakers($outcomes: [String!]) {
fixedProductMarketMakers(
where: {
outcomes: $outcomes,
answerFinalizedTimestamp: null
}
) {
id
creator
title
}
}
seems to work for open markets.
There was a problem hiding this comment.
Ah great, thanks!! I've applied this now - see get_fpmm_query, which specialises the query depending on the FilterBy value.
There was a problem hiding this comment.
Much less hacky now!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (7)
- prediction_market_agent_tooling/markets/agent_market.py (3 hunks)
- prediction_market_agent_tooling/markets/manifold/api.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (2 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (4 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (7 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (3 hunks)
- tests/markets/test_omen.py (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- prediction_market_agent_tooling/markets/agent_market.py
- prediction_market_agent_tooling/markets/manifold/manifold.py
- prediction_market_agent_tooling/monitor/monitor_app.py
- tests/markets/test_omen.py
Additional comments: 11
prediction_market_agent_tooling/markets/manifold/api.py (3)
- 29-29: The addition of the
MARKETS_LIMITconstant is a good practice for managing the maximum number of markets to be returned. This helps in controlling the volume of data processed and ensures that the system can handle the data efficiently.- 36-40: The introduction of new parameters (
sort,filter_, andcreated_after) in theget_manifold_binary_marketsfunction enhances the flexibility and granularity of market retrieval. This allows for more targeted queries, which can improve the efficiency and relevance of the data retrieved.- 55-74: The pagination logic implemented to retrieve markets in batches is a good approach to handle API limitations on the number of items returned in a single request. However, there's a concern regarding the comment from
kongziiabout moving the logic fromget_manifold_markets,get_manifold_markets_paged, andget_manifold_markets_datedhere for runtime checks. It's important to ensure that the refactored logic maintains the same functionality and efficiency as the previous implementation, especially in handling different sorting and filtering scenarios.prediction_market_agent_tooling/markets/omen/data_models.py (3)
- 20-20: The addition of the
Resolutionentity is a good practice for representing the resolution state of markets in a type-safe manner. This enhances code readability and maintainability.- 54-57: Adding properties like
resolutionTimestamp,answerFinalizedTimestamp,currentAnswer, andcreationTimestampto theOmenMarketclass is crucial for capturing the necessary details about market states and timelines. This information is essential for accurately determining market status and outcomes.- 240-242: The update to the
to_generic_resolved_betmethod inOmenBetFPMMto include theresolutionTimestampproperty is a necessary improvement for accurately capturing the resolution time of bets. This ensures that the generic resolved bet model has all the required information for analysis and reporting.prediction_market_agent_tooling/markets/omen/omen.py (5)
- 1-11: The addition of imports for
json,os,random,Enum,requests,Web3, andTxParamsis appropriate given the functionality being introduced. Ensure that all imported modules are used within the file to avoid unnecessary imports which can lead to confusion and slight performance degradation.- 58-60: The constants
OMEN_QUERY_BATCH_SIZE,OMEN_DEFAULT_MARKET_FEE, andDEFAULT_COLLATERAL_TOKEN_CONTRACT_ADDRESSare well-defined and crucial for the operations within this module. It's good practice to define such constants at the top of the file for easy modification and readability.- 96-126: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [61-102]
The
OmenAgentMarketclass has been updated to include additional parameters and logic. It's important to ensure that these changes align with the overall architecture and design principles of the system. Specifically, the methodfrom_data_modelhas been enhanced to include fields likeresolution,created_time, andp_yes. This is a positive change as it enriches the data model with more detailed information about the markets, which is essential for analysis and decision-making processes.
- 120-120: The
Arbitratorenum addition is a good practice for defining a set of predefined arbitrators. This enhances code readability and maintainability by using meaningful names instead of hard-coded strings throughout the codebase.- 228-250: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [231-262]
The updated GraphQL query strings, specifically
_QUERY_GET_FIXED_PRODUCT_MARKETS_MAKERS, are well-structured and align with the requirements for retrieving market data. Ensure that the query parameters are correctly used and that the response is properly handled, especially considering the dynamic nature of the data being queried.Verification successful
The examination of the usage of
_QUERY_GET_FIXED_PRODUCT_MARKETS_MAKERSwithinomen.pyconfirms that the query parameters are correctly used and that the response is properly handled. The parameters are dynamically assigned, and the response from the query execution is processed appropriately, aligning with the requirements for retrieving and processing market data. This verifies the original review comment regarding the GraphQL query strings.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct usage of query parameters and handling of the response in the GraphQL queries. echo "Review GraphQL query parameter usage and response handling."Length of output: 128
Script:
#!/bin/bash # Search for usage of _QUERY_GET_FIXED_PRODUCT_MARKETS_MAKERS in the codebase rg "_QUERY_GET_FIXED_PRODUCT_MARKETS_MAKERS"Length of output: 258
Script:
#!/bin/bash # Extract and review the code section around the usage of _QUERY_GET_FIXED_PRODUCT_MARKETS_MAKERS in omen.py sed -n -e '/_QUERY_GET_FIXED_PRODUCT_MARKETS_MAKERS/,+20p' prediction_market_agent_tooling/markets/omen/omen.pyLength of output: 1523
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- prediction_market_agent_tooling/markets/manifold/api.py (1 hunks)
Additional comments: 1
prediction_market_agent_tooling/markets/manifold/api.py (1)
- 29-29: The introduction of the
MARKETS_LIMITconstant is a good practice for managing the maximum number of markets to be fetched. This helps in avoiding overloading the API with too large requests.
Co-authored-by: Peter Jung <peter@jung.ninja>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- prediction_market_agent_tooling/markets/manifold/api.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/markets/manifold/manifold.py
Additional comments: 3
prediction_market_agent_tooling/markets/manifold/api.py (3)
- 29-29: The introduction of the
MARKETS_LIMITconstant is a good practice to enforce a maximum limit on the number of markets fetched, which can help prevent performance issues due to large data volumes.- 36-40: Adding new parameters (
sort,filter_, andcreated_after) to theget_manifold_binary_marketsfunction enhances its flexibility and allows for more granular control over the markets fetched. This aligns well with the PR objectives of making market monitoring more efficient.- 48-49: The correction from
max(limit, MARKETS_LIMIT)tomin(limit, MARKETS_LIMIT)has been implemented as suggested in the previous comments. This ensures that the number of markets fetched does not exceed the defined limit, preventing potential performance issues.
| @@ -14,6 +14,7 @@ class Resolution(str, Enum): | |||
| YES = "YES" | |||
| NO = "NO" | |||
| CANCEL = "CANCEL" | |||
| MKT = "MKT" | |||
There was a problem hiding this comment.
There is manifold_to_generic_resolved_bet that uses is_resolved_non_cancelled and then does market_outcome = market.get_resolution_enum() == Resolution.YES, but this MKT is not accounted for.
Maybe we could use pattern from benchmark, where the API's model has
class CancelableMarketResolution(str, Enum):
YES = "yes"
NO = "no"
CANCEL = "cancel"
MKT = "mkt"
and then that's converted to
class MarketResolution(str, Enum):
YES = "yes"
NO = "no"
in probable_resolution? So we don't have to worry about other potential changes.
There was a problem hiding this comment.
Hmm yeah it's a little messy. I have tidied this a bit now. I feel like we essentially do have that pattern now:
The 'cancellable resolution':
class ManifoldBet(BaseModel):
resolution: t.Optional[Resolution] = None
and the boolean yes/no resolution:
class ResolvedBet(Bet):
market_outcome: bool
Also good spot, have accounted for Resolution.MKT cases now.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- prediction_market_agent_tooling/benchmark/utils.py (1 hunks)
- prediction_market_agent_tooling/deploy/agent.py (2 hunks)
- prediction_market_agent_tooling/markets/manifold/api.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (2 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (4 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (3 hunks)
Files skipped from review as they are similar to previous changes (5)
- prediction_market_agent_tooling/benchmark/utils.py
- prediction_market_agent_tooling/deploy/agent.py
- prediction_market_agent_tooling/markets/manifold/manifold.py
- prediction_market_agent_tooling/monitor/monitor.py
- prediction_market_agent_tooling/monitor/monitor_app.py
Additional comments: 6
prediction_market_agent_tooling/markets/manifold/api.py (6)
- 29-29: The introduction of the
MARKETS_LIMITconstant is a good practice for managing the maximum number of markets that can be returned by Manifold. This helps in avoiding potential performance issues due to large data volumes.- 36-40: Adding new parameters (
sort,filter_, andcreated_after) to theget_manifold_binary_marketsfunction enhances its flexibility and allows for more precise market queries. This is a positive change that aligns with the PR objectives of improving market retrieval capabilities.- 48-49: The use of
min(limit, MARKETS_LIMIT)to determine thelimitparameter value ensures that the number of markets fetched does not exceed the predefined maximum. This is a sensible safeguard.- 55-74: The implementation of pagination logic using an offset and a while loop is a robust solution for fetching markets iteratively based on the limit. This approach effectively addresses the challenge of retrieving a large number of markets in a controlled manner. However, it's important to ensure that the API being queried supports the
offsetparameter and handles it as expected.- 67-69: The conditional check for
created_afterwithin the market appending loop is a good practice for filtering markets based on their creation time. This adds another layer of flexibility to the market retrieval process.- 76-76: Regarding the existing comment from
kongziiabout preferring the previous code due to API limitations, it's important to balance the benefits of runtime checks against the simplicity and performance of the previous implementation. If the new approach introduces significant complexity or performance issues, it might be worth reconsidering. However, the changes made here seem to enhance flexibility and functionality, which aligns with the PR objectives.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (7)
- prediction_market_agent_tooling/markets/manifold/api.py (3 hunks)
- prediction_market_agent_tooling/markets/manifold/data_models.py (2 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (2 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (6 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (5 hunks)
- tests/markets/test_manifold.py (2 hunks)
- tests/markets/test_omen.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/markets/manifold/manifold.py
- tests/markets/test_omen.py
Additional comments: 12
tests/markets/test_manifold.py (4)
- 1-4: Imports of
datetime,timedelta, andpytzare added to support the handling of time zones and date calculations in the tests. These are standard libraries for managing date and time in Python, ensuring that the tests can accurately simulate scenarios with specific time constraints.- 9-13: New functions from the Manifold API are imported, including
get_authenticated_user,get_manifold_bets,get_manifold_binary_markets,get_resolved_manifold_bets, andmanifold_to_generic_resolved_bet. These functions are essential for fetching market and bet data from Manifold, authenticating users, and converting resolved bets to a generic format. The inclusion of these functions in the test file suggests that they will be used to create more comprehensive tests for Manifold market interactions.- 27-32: The
test_manifold_marketsfunction tests the retrieval of Manifold binary markets with specific parameters (limit,sort,filter_). The assertion checks if the number of markets retrieved equals the specified limit, which is a straightforward way to verify the functionality ofget_manifold_binary_markets. However, it's important to ensure that the Manifold API has enough markets to meet thelimitin the test environment, or this test could fail unintentionally.- 47-61: The
test_resolved_manifold_betsfunction focuses on testing the retrieval of resolved bets for a specific user within a given time frame. It correctly usesdatetimewithpytz.UTCfor time zone consistency and checks both the uniqueness of the bet IDs and the ability to convert bets to a generic resolved bet format. This test effectively validates the functionality ofget_resolved_manifold_betsandmanifold_to_generic_resolved_bet. However, it assumes that there will be resolved bets within the specified time frame, which may not always be the case.prediction_market_agent_tooling/monitor/monitor_app.py (2)
- 7-10: The import of
AgentMarket,FilterBy, andSortByfromagent_marketindicates a shift towards a more modular and flexible approach to market handling. These classes likely encapsulate the logic for sorting and filtering markets, which is a positive step towards improving the code's maintainability and readability.- 25-25: The introduction of the
MAX_MONITOR_MARKETSconstant is a good practice for managing magic numbers within the code. It sets a clear limit on the number of markets to monitor, making the code more configurable and easier to maintain.prediction_market_agent_tooling/markets/manifold/data_models.py (3)
- 37-48: The update to use
t.Optional[Resolution]for theresolutionattribute and the addition ofcreatorAvatarUrlin theManifoldMarketmodel are positive changes that enhance the model's flexibility and completeness. Using optional types for attributes that may not always be present is a good practice, and including the creator's avatar URL enriches the data model with more user-related information.- 59-65: The
get_resolved_boolean_outcomemethod inManifoldMarkethas been updated to handleResolution.YESandResolution.NO, which is a more robust and readable approach compared to handling raw strings or booleans. This change improves the clarity and maintainability of the code by using theResolutionenum.- 148-156: The change in the
ManifoldBetmodel to useResolutionfor theoutcomeattribute and the update to theget_resolved_boolean_outcomemethod are consistent with the improvements made in theManifoldMarketmodel. These changes standardize the handling of outcomes across the codebase, making it easier to understand and maintain.prediction_market_agent_tooling/markets/manifold/api.py (1)
- 28-29: The addition of the
MARKETS_LIMITconstant is a good practice for managing the maximum number of markets that can be returned by the Manifold API. This makes the code more maintainable by centralizing the limit value, making it easier to adjust if needed.prediction_market_agent_tooling/markets/omen/data_models.py (2)
- 20-28: The addition of the
Resolutionentity and theINVALID_ANSWERconstant are important updates that enhance the model's ability to accurately represent the state of Omen markets. TheResolutionentity provides a clear and standardized way to represent market outcomes, while theINVALID_ANSWERconstant helps in identifying unresolved or invalid market states.- 259-261: The
to_generic_resolved_betmethod inOmenBetFPMMhas been updated to include theresolutionTimestampproperty, which is a positive change for accurately representing the time when the market was resolved. This enhances the model's completeness and accuracy in representing resolved bets.
| ) -> tuple[list[AgentMarket], list[AgentMarket]]: | ||
| cls = check_not_none(MARKET_TYPE_MAP.get(market_type)) | ||
|
|
||
| open_markets = cls.get_binary_markets( | ||
| limit=MAX_MONITOR_MARKETS, | ||
| sort_by=SortBy.NEWEST, | ||
| created_after=start_time, | ||
| filter_by=FilterBy.OPEN, | ||
| ) | ||
| resolved_markets = cls.get_binary_markets( | ||
| limit=MAX_MONITOR_MARKETS, | ||
| sort_by=SortBy.NEWEST, | ||
| created_after=start_time, | ||
| filter_by=FilterBy.RESOLVED, | ||
| ) | ||
| resolved_markets = [m for m in resolved_markets if m.has_successful_resolution()] |
There was a problem hiding this comment.
The get_open_and_resolved_markets function has been updated to retrieve markets based on AgentMarket methods, with adjustments for market type handling and filtering. This approach enhances the flexibility of market retrieval by allowing for dynamic sorting and filtering based on the MarketType. However, the function assumes that cls.get_binary_markets will always return a list of markets, which might not be the case if the API call fails or returns an unexpected response.
Consider adding error handling for the API calls within cls.get_binary_markets to ensure robustness in case of failures or unexpected responses.
| sort: t.Literal["liquidity", "score", "newest", "close-date"] = "liquidity", | ||
| filter_: t.Literal[ | ||
| "open", "closed", "resolved", "closing-this-month", "closing-next-month" | ||
| ] = "open", | ||
| created_after: t.Optional[datetime] = None, | ||
| ) -> list[ManifoldMarket]: | ||
| all_markets: list[ManifoldMarket] = [] | ||
|
|
||
| url = "https://api.manifold.markets/v0/search-markets" | ||
| params: dict[str, t.Union[str, int, float]] = { | ||
| "term": term, | ||
| "sort": sort, | ||
| "limit": limit, | ||
| "filter": "open", | ||
| "filter": filter_, | ||
| "limit": min(limit, MARKETS_LIMIT), | ||
| "contractType": "BINARY", | ||
| } | ||
| if topic_slug: | ||
| params["topicSlug"] = topic_slug | ||
| response = requests.get(url, params=params) | ||
|
|
||
| response.raise_for_status() | ||
| data = response.json() | ||
| offset = 0 | ||
| while True: | ||
| params["offset"] = offset | ||
| response = requests.get(url, params=params) | ||
| response.raise_for_status() | ||
| data = response.json() | ||
| markets = [ManifoldMarket.model_validate(x) for x in data] | ||
|
|
||
| if not markets: | ||
| break | ||
|
|
||
| for market in markets: | ||
| if created_after and market.createdTime < created_after: | ||
| continue | ||
| all_markets.append(market) | ||
|
|
||
| if len(all_markets) >= limit: | ||
| break | ||
|
|
||
| offset += len(markets) | ||
|
|
||
| markets = [ManifoldMarket.model_validate(x) for x in data] | ||
| return markets | ||
| return all_markets[:limit] |
There was a problem hiding this comment.
The enhancements to the get_manifold_binary_markets function, including the addition of new parameters (sort, filter_, created_after) and the implementation of pagination logic, significantly improve the function's flexibility and efficiency. The use of pagination ensures that the function can handle large sets of data without overwhelming the API or the application. However, the function lacks error handling for the API requests, which could lead to unhandled exceptions if the API call fails.
Consider adding error handling for the API requests to gracefully handle failures and provide meaningful feedback to the caller.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- prediction_market_agent_tooling/markets/manifold/data_models.py (3 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (6 hunks)
- tests/markets/test_manifold.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/markets/manifold/data_models.py
- tests/markets/test_manifold.py
Additional comments: 7
prediction_market_agent_tooling/markets/omen/data_models.py (7)
- 20-20: The addition of the
Resolutionentity to the imports suggests that it's being used to represent the outcome of a market. It's important to ensure that this entity is defined with clear documentation on its possible values, as it plays a crucial role in understanding market outcomes.Ensure that the
Resolutionentity is well-documented and its values are clearly defined to avoid confusion in interpreting market outcomes.
- 28-28: The
INVALID_ANSWERconstant is introduced to represent an invalid answer in the context of Omen markets. It's good practice to define such constants for magic numbers to improve code readability and maintainability.- 55-58: The addition of optional timestamp properties (
resolutionTimestamp,answerFinalizedTimestamp,currentAnswer,creationTimestamp) to theOmenMarketclass enhances the model's ability to capture crucial time-related information about market states. This is essential for accurately determining the market's current status and history.- 60-66: The
answer_indexandhas_valid_answerproperties provide mechanisms to interpret thecurrentAnswerfield and validate its correctness. This is a good approach to encapsulate the logic for answer validation within the model, improving code readability and maintainability.- 156-162: The
get_resolution_enummethod provides a way to get the market's resolution as an enum value. This method's implementation is straightforward and aligns with the class's other properties for determining the market's state. Ensuring that theResolutionenum is well-defined and matches the expected outcomes is crucial for this method's accuracy.- 97-177: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [169-193]
The
OmenBetFPMMclass has been updated with aresolutionTimestampproperty and ananswer_indexproperty. These additions are consistent with the enhancements made to theOmenMarketclass, indicating an effort to standardize the handling of market resolution and answer interpretation across different models.
- 259-261: The update to the
to_generic_resolved_betmethod in theOmenBetFPMMclass to include theresolved_timeusing theresolutionTimestampproperty is a logical enhancement. It's important to ensure that theresolutionTimestampis always set for resolved bets to avoid potentialNonevalues leading to errors.Ensure that the
resolutionTimestampis always appropriately set for resolved bets to prevent errors when accessing theresolved_time.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- prediction_market_agent_tooling/markets/omen/omen.py (7 hunks)
- tests/markets/test_omen.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/markets/test_omen.py
Additional comments: 6
prediction_market_agent_tooling/markets/omen/omen.py (6)
- 1-11: The addition of imports for
json,os,random,Enum,requests,Web3, andTxParamsis appropriate given the functionalities introduced in this file. These imports are necessary for handling JSON data, file operations, random number generation, enumeration types, making HTTP requests, and interacting with the Ethereum blockchain.- 58-60: The constants
OMEN_QUERY_BATCH_SIZE,OMEN_DEFAULT_MARKET_FEE, andDEFAULT_COLLATERAL_TOKEN_CONTRACT_ADDRESSare well-defined and follow good naming conventions. These constants are crucial for defining the batch size for queries, the default market fee, and the default collateral token contract address, which are used throughout the file in various operations related to the Omen markets.- 55-64: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [61-99]
The
OmenAgentMarketclass has been updated to include additional parameters and logic. This class now better encapsulates the properties and behaviors specific to Omen markets, such as handling bets and converting data models toOmenAgentMarketinstances. The use of type hints and class variables improves code readability and maintainability.
- 107-120: The
get_binary_marketsstatic method addition to theOmenAgentMarketclass is a significant enhancement. It allows for retrieving a list of binary markets based on specified criteria. This method effectively utilizes theget_omen_binary_marketsfunction, demonstrating good code reuse and modularity. However, ensure that the method's implementation efficiently handles potential exceptions and errors, especially when interacting with external data sources.- 228-255: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [232-267]
The GraphQL query strings have been updated to include filters for open and resolved markets. It's important to note the comment about the incompatibility of
answerFinalizedTimestamp: nullandresolutionTimestamp_not: null. This highlights the need for careful consideration when using these filters together. The query design seems well thought out for the intended use case of fetching market data based on specific criteria.
- 273-290: The functions
get_open_fpmm_queryandget_resolved_fpmm_queryprovide a clean way to obtain the appropriate GraphQL query string based on the filter criteria (open or resolved markets). This approach enhances code modularity and readability. Theget_fpmm_queryfunction's design, which selects the correct query based on theFilterByenum, is an excellent example of using conditional logic to streamline code execution paths.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (2)
- prediction_market_agent_tooling/markets/omen/data_models.py (6 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (7 hunks)
Additional comments: 8
prediction_market_agent_tooling/markets/omen/data_models.py (4)
- 55-58: Adding optional properties
resolutionTimestamp,answerFinalizedTimestamp,currentAnswer, andcreationTimestampto theOmenMarketclass enhances the model's ability to track important temporal and state information about markets. This is a positive change for accurately determining market status and probabilities. Ensure that these fields are properly validated and handled in scenarios where they might beNone, to avoid potential runtime errors.- 60-66: The
answer_indexandhas_valid_answerproperties are well-implemented, providing a clear mechanism to interpret thecurrentAnswerfield and check for its validity. It's important to ensure that the hexadecimal conversion and comparison withINVALID_ANSWERare thoroughly tested, especially considering edge cases wherecurrentAnswermight not be in the expected format.- 156-162: The
get_resolution_enummethod provides a clear mechanism to map the market's outcome to aResolutionenum based on theboolean_outcome. This is a straightforward and effective way to abstract the resolution logic. Ensure that this method is covered by unit tests, particularly testing scenarios where the market is not resolved or the outcome is not binary.- 259-261: Updating the
to_generic_resolved_betmethod inOmenBetFPMMto include theresolved_timeproperty is a positive change for accurately tracking bet resolution times. Ensure that thecheck_not_noneutility is robust and that its usage here is thoroughly tested to prevent potential runtime errors whenresolutionTimestampisNone.prediction_market_agent_tooling/markets/omen/omen.py (4)
- 1-11: The imports are well-organized and seem to cover the necessary modules for the functionality described. However, it's important to ensure that all imported modules are used within the file to avoid unnecessary imports which can lead to increased memory usage and slower startup times.
- 58-60: The constants
OMEN_QUERY_BATCH_SIZE,OMEN_DEFAULT_MARKET_FEE, andDEFAULT_COLLATERAL_TOKEN_CONTRACT_ADDRESSare clearly defined and well-documented. These constants are crucial for the operation of the Omen market monitoring functionalities, providing a good balance between configurability and hard-coded values.- 96-126: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [61-102]
The
OmenAgentMarketclass has been updated to include methods for placing bets and converting data models toOmenAgentMarketinstances. These changes align with the PR's objectives of restructuring code for better flexibility and monitoring capabilities. It's important to ensure that the logic within these methods accurately reflects the intended operations and interacts correctly with the underlying data models and external services.
- 107-120: The
get_binary_marketsstatic method addition to theOmenAgentMarketclass is a significant enhancement. It allows for retrieving a list of binary markets based on specified criteria. This method effectively utilizes theget_omen_binary_marketsfunction, demonstrating good code reuse and modularity. However, ensure that the method's implementation efficiently handles potential exceptions and errors, especially when interacting with external data sources.
| def get_omen_markets( | ||
| first: int, | ||
| outcomes: list[str], | ||
| sort_by: SortBy, | ||
| filter_by: FilterBy, | ||
| created_after: t.Optional[datetime] = None, | ||
| ) -> list[OmenMarket]: | ||
| order_by, order_direction = ordering_from_sort_by(sort_by) | ||
| markets = requests.post( | ||
| THEGRAPH_QUERY_URL, | ||
| json={ | ||
| "query": _QUERY_GET_FIXED_PRODUCT_MARKETS_MAKERS, | ||
| "query": get_fpmm_query(filter_by), | ||
| "variables": { | ||
| "first": first, | ||
| "outcomes": outcomes, | ||
| "orderBy": order_by, | ||
| "orderDirection": order_direction, | ||
| "creationTimestamp_gt": to_int_timestamp(created_after) | ||
| if created_after | ||
| else 0, | ||
| }, | ||
| }, | ||
| headers={"Content-Type": "application/json"}, | ||
| ).json()["data"]["fixedProductMarketMakers"] | ||
| ).json() | ||
| markets = markets["data"]["fixedProductMarketMakers"] |
There was a problem hiding this comment.
The get_omen_markets function is crucial for fetching market data from the Omen subgraph. It constructs a GraphQL query based on input parameters and sends a POST request to the subgraph URL. However, there is a potential issue with error handling. If the request fails or the response is not in the expected format, the function may raise an unhandled exception. It's recommended to add error handling mechanisms to ensure the function's robustness and reliability.
+ try:
+ response = requests.post(
+ THEGRAPH_QUERY_URL,
+ json={
+ "query": get_fpmm_query(filter_by),
+ "variables": {
+ "first": first,
+ "outcomes": outcomes,
+ "orderBy": order_by,
+ "orderDirection": order_direction,
+ "creationTimestamp_gt": to_int_timestamp(created_after)
+ if created_after
+ else 0,
+ },
+ },
+ headers={"Content-Type": "application/json"},
+ )
+ response.raise_for_status() # Raises an HTTPError if the response status code is 4XX or 5XX
+ markets = response.json()
+ markets = markets["data"]["fixedProductMarketMakers"]
+ return [OmenMarket.model_validate(market) for market in markets]
+ except requests.RequestException as e:
+ # Handle network-related errors here
+ raise
+ except KeyError:
+ # Handle unexpected response structure here
+ raise
+ except ValueError as e:
+ # Handle JSON decoding errors here
+ raiseCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def get_omen_markets( | |
| first: int, | |
| outcomes: list[str], | |
| sort_by: SortBy, | |
| filter_by: FilterBy, | |
| created_after: t.Optional[datetime] = None, | |
| ) -> list[OmenMarket]: | |
| order_by, order_direction = ordering_from_sort_by(sort_by) | |
| markets = requests.post( | |
| THEGRAPH_QUERY_URL, | |
| json={ | |
| "query": _QUERY_GET_FIXED_PRODUCT_MARKETS_MAKERS, | |
| "query": get_fpmm_query(filter_by), | |
| "variables": { | |
| "first": first, | |
| "outcomes": outcomes, | |
| "orderBy": order_by, | |
| "orderDirection": order_direction, | |
| "creationTimestamp_gt": to_int_timestamp(created_after) | |
| if created_after | |
| else 0, | |
| }, | |
| }, | |
| headers={"Content-Type": "application/json"}, | |
| ).json()["data"]["fixedProductMarketMakers"] | |
| ).json() | |
| markets = markets["data"]["fixedProductMarketMakers"] | |
| def get_omen_markets( | |
| first: int, | |
| outcomes: list[str], | |
| sort_by: SortBy, | |
| filter_by: FilterBy, | |
| created_after: t.Optional[datetime] = None, | |
| ) -> list[OmenMarket]: | |
| order_by, order_direction = ordering_from_sort_by(sort_by) | |
| try: | |
| response = requests.post( | |
| THEGRAPH_QUERY_URL, | |
| json={ | |
| "query": get_fpmm_query(filter_by), | |
| "variables": { | |
| "first": first, | |
| "outcomes": outcomes, | |
| "orderBy": order_by, | |
| "orderDirection": order_direction, | |
| "creationTimestamp_gt": to_int_timestamp(created_after) | |
| if created_after | |
| else 0, | |
| }, | |
| }, | |
| headers={"Content-Type": "application/json"}, | |
| ) | |
| response.raise_for_status() # Raises an HTTPError if the response status code is 4XX or 5XX | |
| markets = response.json() | |
| markets = markets["data"]["fixedProductMarketMakers"] | |
| return [OmenMarket.model_validate(market) for market in markets] | |
| except requests.RequestException as e: | |
| # Handle network-related errors here | |
| raise | |
| except KeyError: | |
| # Handle unexpected response structure here | |
| raise | |
| except ValueError as e: | |
| # Handle JSON decoding errors here | |
| raise |
| @property | ||
| def is_binary(self) -> bool: | ||
| return len(self.outcomes) == 2 | ||
|
|
||
| @property | ||
| def boolean_outcome(self) -> bool: | ||
| if not self.is_binary: | ||
| raise ValueError( | ||
| f"Market with title {self.title} is not binary, it has {len(self.outcomes)} outcomes." | ||
| ) | ||
| if not self.is_resolved: | ||
| raise ValueError(f"Bet with title {self.title} is not resolved.") | ||
|
|
||
| try: | ||
| outcome: str = self.outcomes[check_not_none(self.answer_index)] | ||
| except IndexError: | ||
| breakpoint() | ||
| return get_boolean_outcome(outcome) |
There was a problem hiding this comment.
The is_binary and boolean_outcome properties are essential for determining if the market is binary and interpreting its outcome. The implementation appears correct, but thorough testing is crucial, especially for the boolean_outcome property, which relies on the get_boolean_outcome function and the answer_index. Given the previous comments on these properties, it would be beneficial to follow up on the offer to assist with creating comprehensive tests.
Would you like assistance in creating comprehensive tests for the is_binary and boolean_outcome properties to ensure their reliability?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/markets/test_manifold.py (1 hunks)
Additional comments: 3
tests/markets/test_manifold.py (3)
- 1-6: Imports of
datetime,timedelta,pytz, andload_dotenvhave been added. These are standard Python modules and third-party libraries necessary for handling date and time operations, timezone adjustments, and loading environment variables, respectively. The usage of these imports aligns with the objectives of enhancing testing functionalities for Manifold markets. Ensure that the environment variables accessed viaload_dotenvare documented and securely managed.- 10-18: New functions
get_authenticated_user,get_manifold_bets,get_manifold_binary_markets,get_resolved_manifold_bets,manifold_to_generic_resolved_bet, and utilitycheck_not_nonehave been added. These functions are crucial for fetching market and bet data, authenticating users, and ensuring non-null values. It's important to ensure that these functions handle errors gracefully, especially when interacting with external APIs or when environment variables are missing or invalid.- 21-21: The call to
load_dotenv()is crucial for loading environment variables from a.envfile, which is a common practice for managing configuration and secrets in development environments. Ensure that the.envfile is properly secured and not included in version control to prevent leakage of sensitive information.
| def test_manifold_markets() -> None: | ||
| limit = 2000 | ||
| markets = get_manifold_binary_markets( | ||
| limit=limit, sort="newest", filter_="resolved" | ||
| ) | ||
| assert len(markets) == limit |
There was a problem hiding this comment.
The test test_manifold_markets fetches Manifold binary markets with a specified limit and filter. It's good practice to parameterize such limits and filters to enhance test flexibility and coverage. Consider using pytest.mark.parametrize to run this test with different limit and filter_ values.
- limit = 2000
+ @pytest.mark.parametrize("limit, filter_", [(2000, "resolved"), (100, "open")])
+ def test_manifold_markets(limit, filter_):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def test_manifold_markets() -> None: | |
| limit = 2000 | |
| markets = get_manifold_binary_markets( | |
| limit=limit, sort="newest", filter_="resolved" | |
| ) | |
| assert len(markets) == limit | |
| @pytest.mark.parametrize("limit, filter_", [(2000, "resolved"), (100, "open")]) | |
| def test_manifold_markets(limit, filter_) -> None: | |
| markets = get_manifold_binary_markets( | |
| limit=limit, sort="newest", filter_=filter_ | |
| ) | |
| assert len(markets) == limit |
This reverts commit e2417d4.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- tests/markets/test_manifold.py (1 hunks)
- tests/markets/test_omen.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/markets/test_omen.py
Additional comments: 3
tests/markets/test_manifold.py (3)
- 1-4: The imports for
datetime,timedelta, andpytzare correctly added and necessary for handling date and time operations in the test functions.- 38-45: The
test_manifold_betsfunction correctly tests fetching Manifold bets for a given user ID and time range, using thea_user_idfixture and datetime objects with timezone information.- 48-60: The
test_resolved_manifold_betsfunction correctly tests fetching resolved Manifold bets for a given user ID and time range. It ensures the uniqueness of bets and their conversion to a generic format, using thea_user_idfixture and datetime objects with timezone information.
| @pytest.fixture | ||
| def a_user_id() -> str: | ||
| return "oCJrphq1RIT7fv3echpwOX8DTJV2" |
There was a problem hiding this comment.
The a_user_id fixture correctly returns a hardcoded user ID. Consider making the user ID configurable through environment variables or a configuration file, especially if tests are run in different environments or require different user contexts.
|
|
||
| markets = [ManifoldMarket.model_validate(x) for x in data] | ||
| return markets | ||
| return all_markets[:limit] |
There was a problem hiding this comment.
I was worried that if we do if created_after and market.createdTime < created_after: without enforcing "newest" sort, it's not clear how much time this function will take. As the markets can be sorted arbitrarily.
If we implement created_after like this, there is no reason to not have created_before as well, but then, with a big enough limit, the function could iterate the whole market database.
That's why I liked the previous split that was based on the API limitations.
However, if you don't think that's a real problem, maybe I'm just too worried so feel free to leave it like this.
|
|
||
| def is_resolved_non_cancelled(self) -> bool: | ||
| return ( | ||
| self.isResolved | ||
| and self.resolutionTime is not None | ||
| and self.get_resolution_enum() != Resolution.CANCEL | ||
| and self.resolution not in [Resolution.CANCEL, Resolution.MKT] |
There was a problem hiding this comment.
We you able to find some information about what MKT means? A quick google search didn't tell me anything relevant, so idk how we should process them. But probably good to just leave it as it is, just checking if you have more information.
There was a problem hiding this comment.
It's not clear from the manifold api docs, but I had a look through examples and it seems to be a binary market that doesn't resolve to yes or no, but to a percentage. The condition for this seems to be something the creator of the market can decide.
Here's a list of some manifold markets that have resolved this way:
https://manifold.markets/Joshua/very-short-fuse-resolves-to-my-rati
https://manifold.markets/ScottAlexander/what-percentile-did-the-median-supe
https://manifold.markets/ScottAlexander/what-percentile-did-metaculus-get-i
https://manifold.markets/ScottAlexander/what-percentile-did-manifold-get-in
https://manifold.markets/KongoLandwalker/nomic-game
https://manifold.markets/Ziddletwix/exactly-how-much-will-dune-part-two
https://manifold.markets/cc6/how-much-will-manifest-2024-cost-fo
https://manifold.markets/mattyb/shortfuse-what-will-i-rate-the-kfc
https://manifold.markets/KeenenWatts/will-netflix-open-higher-than-58356
https://manifold.markets/KongoLandwalker/how-far-will-i-go-in-my-productivit
https://manifold.markets/ML12d1/swiss-popular-vote-on-030324-how-ma
https://manifold.markets/EricNeyman/how-many-people-will-attend-the-ber
https://manifold.markets/june/fun-market-structure-how-many-accou
https://manifold.markets/bohaska/how-many-days-later-will-ediths-nex
https://manifold.markets/probajoelistic/how-many-points-will-be-scored-in-s
https://manifold.markets/benjaminIkuta/if-i-get-demoted-will-it-be-because-57b513edc788
https://manifold.markets/benjaminIkuta/this-market-resolves-prob-but-if-so-d09d52ca4a9a
https://manifold.markets/ChristopherRandles/resolve-increases-20-for-each-space-31719721cd8a
https://manifold.markets/SemioticRivalry/vibebased-what-percentage-will-the
https://manifold.markets/mattyb/shortfuse-does-my-mom-know-whos-pla
https://manifold.markets/AWA/will-i-finish-reading-15-books-by-t
https://manifold.markets/Joshua/what-will-be-the-initial-metaculus
https://manifold.markets/benjaminIkuta/this-market-resolves-prob-but-if-so-8b0c529d9efe
https://manifold.markets/OanaDragomir/will-adhd-remain-catalogued-as-a-di
https://manifold.markets/benjaminIkuta/if-i-get-a-job-will-i-fix-my-sleep
https://manifold.markets/mariopasquato/will-i-find-a-robust-causal-discove
There was a problem hiding this comment.
It's not clear to me what happens to the bets made on a binary market that resolves to MKT
| markets = requests.post( | ||
| THEGRAPH_QUERY_URL, | ||
| json={ | ||
| "query": _QUERY_GET_FIXED_PRODUCT_MARKETS_MAKERS, | ||
| "query": get_fpmm_query(filter_by), |
There was a problem hiding this comment.
Looks like this will need to be unified with construct_query_get_fixed_product_markets_makers from the replication PR that's already in.
I like your get_resolved_fpmm_query and get_open_fpmm_query approach (single function that's does the job), just a warning that you need to do
query = query.replace("creator: $creator,", "")
based on the fact if creator will be used or not in the filter, because otherwise it will return 0 results.
So I'm not sure if the single-function approach will be usable or maybe everything will need to be if-ed inside of construct_query_get_fixed_product_markets_makers.
There was a problem hiding this comment.
Have modified construct_query_get_fixed_product_markets_makers to take a filter_by arg and used that 👍
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (6)
- prediction_market_agent_tooling/benchmark/utils.py (1 hunks)
- prediction_market_agent_tooling/deploy/agent.py (2 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (6 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (6 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (4 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- prediction_market_agent_tooling/benchmark/utils.py
- prediction_market_agent_tooling/deploy/agent.py
- prediction_market_agent_tooling/monitor/monitor.py
- prediction_market_agent_tooling/monitor/monitor_app.py
Additional comments: 11
prediction_market_agent_tooling/markets/omen/data_models.py (7)
- 28-28: The addition of the
INVALID_ANSWERconstant with a descriptive comment enhances code readability and understanding. Good job addressing the previous feedback.- 62-64: The
answer_indexproperty provides a convenient way to convert thecurrentAnswerfrom a hexadecimal string to an integer. This is a useful addition for processing the answer index in a more usable format.- 66-68: The
has_valid_answerproperty correctly checks for the presence of a valid answer by comparing theanswer_indexagainst theINVALID_ANSWER. This logic is crucial for determining the validity of a market's answer.- 70-72: The
is_openproperty's implementation, which checks ifcurrentAnswerisNoneto determine if the market is open, aligns with the expected logic for market status determination.- 74-76: The
is_resolvedproperty combines the check for a non-nullanswerFinalizedTimestampwith thehas_valid_answerproperty to accurately determine if a market is resolved. This implementation is logical and efficient.- 160-169: The
get_resolution_enummethod provides a clear mechanism to map the boolean outcome of a market to aResolutionenum. This method enhances the readability and maintainability of the code by abstracting the resolution logic.- 228-230: Updating the
to_generic_resolved_betmethod in theOmenBetclass to use theresolutionTimestampfrom theOmenMarketinstance for theresolved_timeis a logical change that aligns with the PR objectives. This update ensures that the resolved bet accurately reflects the market's resolution timestamp.prediction_market_agent_tooling/markets/omen/omen.py (4)
- 1-11: The imports are correctly organized and necessary for the functionality provided in this file. The use of
json,os,random,Enum,requests,Web3, andTxParamsaligns with the operations performed, such as file reading, network requests, and blockchain interactions.- 56-58: The constants
OMEN_QUERY_BATCH_SIZE,OMEN_DEFAULT_MARKET_FEE, andDEFAULT_COLLATERAL_TOKEN_CONTRACT_ADDRESSare well-defined and appropriately used throughout the file. These constants improve code readability and maintainability by centralizing configuration values.- 94-124: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [59-118]
The
OmenAgentMarketclass is well-structured and follows the principles of object-oriented design. It encapsulates the functionality specific to Omen markets, such as placing bets and converting data models. The use of static methods for market retrieval is appropriate and allows for clear separation of concerns.
- 120-123: The
Arbitratorenum is correctly defined and used to specify the arbitrator for Omen markets. This approach enhances code readability and ensures type safety when dealing with arbitrator values.
| def construct_query_get_fixed_product_markets_makers( | ||
| include_creator: bool, filter_by: FilterBy | ||
| ) -> str: | ||
| query = """ | ||
| query getFixedProductMarketMakers( | ||
| $first: Int!, | ||
| $outcomes: [String!], | ||
| $orderBy: String!, | ||
| $orderDirection: String!, | ||
| $creationTimestamp_gt: Int!, | ||
| $creator: Bytes = null, | ||
| ) { | ||
| id | ||
| title | ||
| category | ||
| creationTimestamp | ||
| collateralVolume | ||
| usdVolume | ||
| collateralToken | ||
| outcomes | ||
| outcomeTokenAmounts | ||
| outcomeTokenMarginalPrices | ||
| fee | ||
| fixedProductMarketMakers( | ||
| where: { | ||
| isPendingArbitration: false, | ||
| outcomes: $outcomes | ||
| creationTimestamp_gt: $creationTimestamp_gt | ||
| creator: $creator, | ||
| answerFinalizedTimestamp: null | ||
| resolutionTimestamp_not: null | ||
| }, | ||
| orderBy: creationTimestamp, | ||
| orderDirection: desc, | ||
| first: $first | ||
| ) { | ||
| id | ||
| title | ||
| collateralVolume | ||
| usdVolume | ||
| collateralToken | ||
| outcomes | ||
| outcomeTokenAmounts | ||
| outcomeTokenMarginalPrices | ||
| fee | ||
| answerFinalizedTimestamp | ||
| resolutionTimestamp | ||
| currentAnswer | ||
| creationTimestamp | ||
| category | ||
| } | ||
| } | ||
| }""" | ||
| """ | ||
|
|
||
| if filter_by == FilterBy.OPEN: | ||
| query = query.replace("resolutionTimestamp_not: null", "") | ||
| elif filter_by == FilterBy.RESOLVED: | ||
| query = query.replace("answerFinalizedTimestamp: null", "") | ||
| else: | ||
| raise ValueError(f"Unknown filter_by: {filter_by}") |
There was a problem hiding this comment.
The function construct_query_get_fixed_product_markets_makers correctly constructs a GraphQL query string based on the provided parameters. The conditional modifications of the query string based on filter_by and include_creator are logically sound. However, consider adding comments to explain the rationale behind these conditional modifications for future maintainability.
# If filtering by open markets, remove the condition that excludes resolved markets.
# If filtering by resolved markets, remove the condition that only includes unresolved markets.
# If not including creator in the query, remove the creator condition to avoid returning 0 results.| outcomes | ||
| title | ||
| answerFinalizedTimestamp | ||
| resolutionTimestamp | ||
| currentAnswer | ||
| isPendingArbitration | ||
| arbitrationOccurred | ||
| openingTimestamp | ||
| condition { | ||
| id | ||
| } | ||
| collateralVolume | ||
| usdVolume | ||
| collateralToken | ||
| outcomeTokenAmounts | ||
| outcomeTokenMarginalPrices | ||
| fee | ||
| category | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [362-774]
The remaining functions related to market operations, blockchain interactions, and external API queries are generally well-implemented. However, similar to the get_omen_markets function, explicit error handling is recommended for network requests and blockchain interactions to improve robustness and error reporting. Additionally, consider optimizing repeated code patterns, such as the construction and execution of blockchain transactions, to enhance maintainability.
Consider abstracting common patterns in blockchain interactions and network requests into utility functions to reduce code duplication and simplify error handling.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- prediction_market_agent_tooling/monitor/monitor_app.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/monitor/monitor_app.py
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (5)
- prediction_market_agent_tooling/markets/agent_market.py (3 hunks)
- prediction_market_agent_tooling/markets/manifold/api.py (3 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (6 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (6 hunks)
- prediction_market_agent_tooling/markets/omen/omen_replicate.py (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/markets/agent_market.py
- prediction_market_agent_tooling/markets/manifold/api.py
Additional comments: 10
prediction_market_agent_tooling/markets/omen/omen_replicate.py (3)
- 12-12: The addition of imports for
FilterByandSortByis appropriate and aligns with the changes made in the function calls within this file.- 39-40: The addition of
sort_by=SortBy.NEWESTandfilter_by=FilterBy.NONEparameters toget_omen_binary_marketscalls is logical and supports the PR's objective of enhancing market monitoring capabilities. Ensure that these changes integrate well with the rest of the system.Verification successful
The integration of
sort_by=SortBy.NEWESTandfilter_by=FilterBy.NONEparameters in calls toget_omen_binary_marketsis consistent across the system and aligns with the objective of enhancing market monitoring capabilities. The usage of these parameters is logical and supports systematic enhancements to the functionality. Ensure that these changes integrate well with the rest of the system.* 79-80: The repeated addition of `sort_by=SortBy.NEWEST` and `filter_by=FilterBy.NONE` in another `get_omen_binary_markets` call is consistent and supports the objective of enhancing market monitoring capabilities. Ensure that these changes integrate well with the rest of the system.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify integration of the new parameters in get_omen_binary_markets calls rg --type py 'get_omen_binary_markets' -C 5Length of output: 8167
Verification successful
The addition of
sort_by=SortBy.NEWESTandfilter_by=FilterBy.NONEparameters in theget_omen_binary_marketscalls across the codebase is consistent and purposefully varies according to the context of each call. This supports the objective of enhancing market monitoring capabilities while allowing for flexibility in market filtering. The integration of these changes appears well-implemented across the system.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify integration of the new parameters in get_omen_binary_markets calls rg --type py 'get_omen_binary_markets' -C 5Length of output: 8167
prediction_market_agent_tooling/markets/omen/data_models.py (1)
- 20-20: The addition of the
Resolutionentity aligns with the PR objectives and enhances the data model for market monitoring.prediction_market_agent_tooling/markets/omen/omen.py (6)
- 1-11: The imports are well-organized and relevant to the functionality provided in this file. It's good practice to keep imports clean and only include what's necessary for the file's operations.
- 56-58: The global constants
OMEN_QUERY_BATCH_SIZE,OMEN_DEFAULT_MARKET_FEE, andDEFAULT_COLLATERAL_TOKEN_CONTRACT_ADDRESSare clearly defined and serve important roles in the functionality of the Omen market operations. It's good to see these values defined at the top of the file for easy modification and reference.- 94-124: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [59-100]
The
OmenAgentMarketclass is well-structured and provides a clear interface for interacting with Omen markets. The methodsget_tiny_bet_amount,place_bet, andfrom_data_modelare logically implemented. However, ensure that theplace_betmethod properly handles all potential exceptions, especially when interacting with external systems or performing operations that might fail.
- 120-122: The
Arbitratorenum is correctly defined and provides a clear way to specify the arbitrator for a market. This approach enhances code readability and maintainability by using meaningful names instead of hard-coded strings.- 233-284: The function
construct_query_get_fixed_product_markets_makersdynamically constructs a GraphQL query based on the provided parameters. It's well-implemented, but ensure that the dynamic construction of the query does not introduce any security vulnerabilities, such as injection attacks. Consider validating or sanitizing inputs where applicable.- 365-368: The
pick_binary_marketfunction provides a convenient way to retrieve a single binary market based on specified criteria. It's a good example of simplifying complex operations into a more accessible interface for other parts of the code. Ensure that the underlyingget_omen_binary_marketsfunction it relies on is correctly implemented and handles errors appropriately.
| ResolvedBet, | ||
| ) | ||
| from prediction_market_agent_tooling.tools.utils import check_not_none | ||
| from prediction_market_agent_tooling.tools.web3_utils import wei_to_xdai | ||
|
|
||
| OMEN_TRUE_OUTCOME = "Yes" | ||
| OMEN_FALSE_OUTCOME = "No" | ||
| INVALID_ANSWER = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF |
There was a problem hiding this comment.
The addition of the INVALID_ANSWER constant is appropriate for representing an invalid answer. Consider adding a comment explaining its purpose and context for clarity.
+ # Represents an invalid answer in hexadecimal format, used to check answer validity
INVALID_ANSWER = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFCommittable 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.
| INVALID_ANSWER = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF | |
| # Represents an invalid answer in hexadecimal format, used to check answer validity | |
| INVALID_ANSWER = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF |
| resolutionTimestamp: t.Optional[int] = None | ||
| answerFinalizedTimestamp: t.Optional[int] = None | ||
| currentAnswer: t.Optional[str] = None | ||
| creationTimestamp: t.Optional[int] = None |
There was a problem hiding this comment.
The addition of optional properties (resolutionTimestamp, answerFinalizedTimestamp, currentAnswer, creationTimestamp) to the OmenMarket class enhances the model's flexibility to represent various states of a market. Ensure these properties are thoroughly tested, especially in scenarios where they might be None.
Would you like me to help with creating unit tests for these properties to ensure their accuracy?
| @property | ||
| def is_open(self) -> bool: | ||
| return self.currentAnswer is None | ||
|
|
||
| @property | ||
| def is_resolved(self) -> bool: | ||
| return self.answerFinalizedTimestamp is not None and self.has_valid_answer |
There was a problem hiding this comment.
The implementation of is_open and is_resolved properties is crucial for accurately reflecting the market's status. It's important to ensure these properties are thoroughly tested under various scenarios to confirm their reliability.
Would you like me to help with creating unit tests for the is_open and is_resolved properties to ensure their accuracy?
| @property | ||
| def p_yes(self) -> Probability: | ||
| """ | ||
| Calculate the probability of the outcomes from the relative token amounts. | ||
|
|
||
| Note, not all markets reliably have outcomeTokenMarginalPrices, hence we | ||
| use the relative proportion of outcomeTokenAmounts to calculate the | ||
| probabilities. | ||
|
|
||
| The higher the proportion of available outcome tokens for a given outcome, | ||
| the the lower the price of that token, and therefore the lower the | ||
| probability of that outcome. | ||
| """ | ||
| if self.outcomeTokenAmounts is None: | ||
| raise ValueError( | ||
| f"Market with title {self.title} has no outcomeTokenAmounts." | ||
| ) | ||
| if len(self.outcomeTokenAmounts) != 2: | ||
| raise ValueError( | ||
| f"Market with title {self.title} has {len(self.outcomeTokenAmounts)} outcomes." | ||
| ) | ||
| true_index = self.outcomes.index(OMEN_TRUE_OUTCOME) | ||
|
|
||
| class OmenBetFPMM(BaseModel): | ||
| id: HexAddress | ||
| outcomes: list[str] | ||
| title: str | ||
| answerFinalizedTimestamp: t.Optional[int] = None | ||
| currentAnswer: t.Optional[str] = None | ||
| isPendingArbitration: bool | ||
| arbitrationOccurred: bool | ||
| openingTimestamp: int | ||
| if sum(self.outcomeTokenAmounts) == 0: | ||
| return Probability(0.5) | ||
|
|
||
| @property | ||
| def is_resolved(self) -> bool: | ||
| return ( | ||
| self.answerFinalizedTimestamp is not None and self.currentAnswer is not None | ||
| return Probability( |
There was a problem hiding this comment.
The p_yes property's implementation for calculating the probability of a "Yes" outcome based on the relative proportion of outcome tokens is a thoughtful approach. However, there are a couple of potential issues:
- The method assumes binary markets only. This is not inherently wrong but should be explicitly documented to avoid confusion.
- The division by zero check (
sum(self.outcomeTokenAmounts) == 0) is correctly handled, but the method could benefit from additional error handling for unexpected cases, such as whenoutcomeTokenAmountsis not properly set.
Consider adding explicit documentation about the binary market assumption and additional error handling for unexpected cases.
| def get_binary_markets( | ||
| limit: int, | ||
| sort_by: SortBy, | ||
| filter_by: FilterBy = FilterBy.OPEN, | ||
| created_after: t.Optional[datetime] = None, | ||
| ) -> list[AgentMarket]: | ||
| return [ | ||
| OmenAgentMarket.from_data_model(m) | ||
| for m in get_omen_binary_markets(limit=limit) | ||
| for m in get_omen_binary_markets( | ||
| limit=limit, | ||
| sort_by=sort_by, | ||
| created_after=created_after, | ||
| filter_by=filter_by, | ||
| ) |
There was a problem hiding this comment.
Tip
Codebase Verification
The get_omen_markets function makes network requests and parses JSON responses but does not explicitly show try-except blocks for handling potential errors from these operations in the provided script output. It does include a validation step for the data (OmenMarket.model_validate), which can help ensure data integrity. However, the absence of direct error handling for the network request and JSON parsing steps could be a concern for robust error handling. It's recommended to implement explicit error handling mechanisms for these operations to improve the resilience of the function.
Analysis chain
The get_binary_markets static method is a useful addition for retrieving binary markets with specific criteria. It demonstrates good code reuse by utilizing the get_omen_binary_markets function. Ensure that this method and the underlying get_omen_binary_markets function implement efficient error handling, especially for network requests and data parsing.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Locate and review the get_omen_binary_markets function within the codebase, focusing on error handling
rg "def get_omen_binary_markets" -A 20
Length of output: 1799
Script:
#!/bin/bash
# Locate and review the get_omen_markets function within the codebase, focusing on error handling
rg "def get_omen_markets" -A 30
Length of output: 2781
|
|
||
| def get_omen_markets( | ||
| first: int, outcomes: list[str], creator: HexAddress | None = None | ||
| first: int, | ||
| outcomes: list[str], | ||
| sort_by: SortBy, | ||
| filter_by: FilterBy, | ||
| created_after: t.Optional[datetime] = None, | ||
| creator: t.Optional[HexAddress] = None, | ||
| ) -> list[OmenMarket]: | ||
| order_by, order_direction = ordering_from_sort_by(sort_by) | ||
| markets = requests.post( | ||
| THEGRAPH_QUERY_URL, | ||
| json={ | ||
| "query": construct_query_get_fixed_product_markets_makers( | ||
| include_creator=creator is not None | ||
| include_creator=creator is not None, | ||
| filter_by=filter_by, | ||
| ), | ||
| "variables": { | ||
| "first": first, | ||
| "outcomes": outcomes, | ||
| "orderBy": order_by, | ||
| "orderDirection": order_direction, | ||
| "creationTimestamp_gt": to_int_timestamp(created_after) | ||
| if created_after | ||
| else 0, | ||
| "creator": creator, | ||
| }, | ||
| }, | ||
| headers={"Content-Type": "application/json"}, | ||
| ).json()["data"]["fixedProductMarketMakers"] | ||
| ).json() | ||
| markets = markets["data"]["fixedProductMarketMakers"] |
There was a problem hiding this comment.
The get_omen_markets function correctly fetches Omen markets based on specified criteria. However, there is a lack of explicit error handling for the network request and JSON parsing. Consider adding try-except blocks to handle potential exceptions such as requests.exceptions.RequestException for network issues and ValueError for JSON parsing errors.
try:
response = requests.post(...)
response.raise_for_status() # Raises an HTTPError for bad responses
markets = response.json()
except requests.exceptions.RequestException as e:
# Handle network errors
raise RuntimeError(f"Network error occurred: {e}")
except ValueError:
# Handle JSON parsing errors
raise RuntimeError("Failed to parse JSON response")
Refactoring code in monitor_app so it no longer hard-codes manifold market monitoring, and allows market monitoring for a general
MarketType.Involved mainly:
Marketclass in benchmarking code to theAgentMarketclass and its subclasses.Summary by CodeRabbit