501 seer add subgraph to subgraphhandler #535
Conversation
WalkthroughThe changes introduce several new classes and modify existing ones to enhance the handling of subgraph queries in the prediction market tooling. A new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…-add-subgraph-to-subgraphhandler
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
prediction_market_agent_tooling/markets/seer/data_models.py (4)
6-13: Add documentation and field validations.Consider enhancing the model with:
- A docstring explaining the model's purpose and field meanings
- Field validations for the lists to ensure they're non-empty if required
- Clear null/optional field indicators
Example improvements:
class SeerMarket(BaseModel): + """Represents a Seer prediction market. + + Attributes: + id: Unique identifier of the market + title: Human-readable market name + outcomes: List of possible market outcomes + parent_market: ID of the parent market if exists + wrapped_tokens: List of wrapped token addresses used in this market + """ model_config = ConfigDict(populate_by_name=True) id: HexBytes title: str = Field(alias="marketName") - outcomes: list[str] + outcomes: list[str] = Field(..., min_items=1) parent_market: HexBytes = Field(alias="parentMarket") - wrapped_tokens: list[HexBytes] = Field(alias="wrappedTokens") + wrapped_tokens: list[HexBytes] = Field(alias="wrappedTokens", min_items=1)
16-19: Enhance token model with documentation and validations.The token model should:
- Include documentation
- Add string validations
- Consider adding model_config for consistency with other classes
Suggested improvements:
class SeerToken(BaseModel): + """Represents a token in the Seer protocol. + + Attributes: + id: Token contract address + name: Token name + symbol: Token symbol + """ + model_config = ConfigDict(populate_by_name=True) + id: HexBytes - name: str - symbol: str + name: str = Field(..., min_length=1) + symbol: str = Field(..., min_length=1)
22-27: Add pool documentation and business rule validations.The pool model should:
- Include documentation
- Validate liquidity value
- Ensure token0 and token1 are different
Suggested improvements:
class SeerPool(BaseModel): + """Represents a liquidity pool in the Seer protocol. + + Attributes: + id: Pool contract address + liquidity: Total liquidity in the pool + token0: First token in the pair + token1: Second token in the pair + """ model_config = ConfigDict(populate_by_name=True) + id: HexBytes - liquidity: int + liquidity: int = Field(..., ge=0) token0: SeerToken token1: SeerToken + + def model_post_init(self, *args, **kwargs) -> None: + """Validate that token0 and token1 are different.""" + if self.token0.id == self.token1.id: + raise ValueError("token0 and token1 must be different")
1-27: Consider architectural improvements for robustness.To enhance the robustness and usability of these models:
- Consider making models immutable using
frozen=Truein ConfigDict- Add helper methods for common operations (e.g.,
is_active(),get_outcome_token())- Consider adding a base class for common blockchain entity attributes (id, etc.)
Example base class:
class BlockchainEntity(BaseModel): """Base class for blockchain entities with common attributes.""" model_config = ConfigDict(populate_by_name=True, frozen=True) id: HexBytestests_integration/markets/seer/test_seer_subgraph_handler.py (2)
11-14: Add error handling to the fixture.While the fixture is well-typed and appropriately scoped, consider adding error handling for initialization failures.
@pytest.fixture(scope="module") def handler() -> t.Generator[SeerSubgraphHandler, None, None]: - yield SeerSubgraphHandler() + try: + handler = SeerSubgraphHandler() + yield handler + except Exception as e: + pytest.fail(f"Failed to initialize SeerSubgraphHandler: {e}")
16-19: Add more assertions for market properties.The test only verifies the number of markets. Consider validating other market properties to ensure data integrity.
def test_get_all_seer_markets(handler: SeerSubgraphHandler) -> None: markets = handler.get_binary_markets() assert len(markets) > 1 + for market in markets: + assert market.id is not None + assert isinstance(market.id, HexBytes) + assert len(market.wrapped_tokens) > 0prediction_market_agent_tooling/markets/base_subgraph_handler.py (1)
37-37: Clarify the docstring for_parse_items_from_jsonThe docstring on line 37 could be improved for clarity and grammar. Consider rephrasing it for better understanding.
Suggested change:
-"""subgrounds return a weird key as a dict key""" +"""Parse items from the JSON result returned by Subgrounds. Subgrounds may return items indexed by a key as a list or as a dictionary if there's only a single element. """prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py (2)
13-13: Redundant use of.lower()onADDRESS_ZEROSince
ADDRESS_ZEROfromweb3.constantsis already in lowercase ("0x0000000000000000000000000000000000000000"), applying.lower()is unnecessary. Removing it can simplify the code without affecting functionality.Apply this diff to remove the redundant
.lower():-BINARY_MARKETS_FILTER = {"parentMarket": ADDRESS_ZERO.lower()} +BINARY_MARKETS_FILTER = {"parentMarket": ADDRESS_ZERO}
41-52: Refactor field selection methods to reduce code duplicationBoth
_get_fields_for_marketsand_get_fields_for_poolsconstruct lists of fields for queries in a similar manner. Consider creating a generic method or utilizing class variables to define the fields, enhancing maintainability.Apply this refactoring suggestion:
# Define class variables for fields MARKET_FIELDS = [ 'id', 'factory', 'creator', 'marketName', 'outcomes', 'parentMarket', 'finalizeTs', 'wrappedTokens', ] POOL_FIELDS = [ 'id', 'liquidity', 'token0.id', 'token0.name', 'token0.symbol', 'token1.id', 'token1.name', 'token1.symbol', ] def _get_fields(self, base_field: FieldPath, fields: list[str]) -> list[FieldPath]: return [reduce(getattr, field.split('.'), base_field) for field in fields] # Update existing methods to use the new function def _get_fields_for_markets(self, markets_field: FieldPath) -> list[FieldPath]: return self._get_fields(markets_field, self.MARKET_FIELDS) def _get_fields_for_pools(self, pools_field: FieldPath) -> list[FieldPath]: return self._get_fields(pools_field, self.POOL_FIELDS)Also applies to: 70-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
prediction_market_agent_tooling/markets/base_subgraph_handler.py(1 hunks)prediction_market_agent_tooling/markets/omen/omen_subgraph_handler.py(4 hunks)prediction_market_agent_tooling/markets/seer/data_models.py(1 hunks)prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py(1 hunks)tests_integration/markets/seer/test_seer_subgraph_handler.py(1 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent_tooling/markets/base_subgraph_handler.py
40-40: Loop control variable k not used within loop body
Rename unused k to _k
(B007)
🔇 Additional comments (9)
prediction_market_agent_tooling/markets/seer/data_models.py (1)
1-4: LGTM! Imports are clean and appropriate.
The imports are minimal and well-chosen for the data models' requirements.
tests_integration/markets/seer/test_seer_subgraph_handler.py (2)
1-9: LGTM! Clean and minimal imports.
The imports are well-organized and include all necessary dependencies with proper type hints.
1-46: Consider adding these test scenarios for better coverage:
- Test handling of malformed market IDs
- Test pagination/limits when retrieving all markets
- Test error cases for network issues
- Test concurrent access to the handler (since it's a singleton)
- Test empty or invalid pool scenarios
prediction_market_agent_tooling/markets/omen/omen_subgraph_handler.py (6)
7-7: Approved: Import of FieldPath
The import statement correctly adds FieldPath from the subgrounds library, which is necessary for handling field paths in subgraph queries.
17-19: Approved: Import of BaseSubgraphHandler
Importing BaseSubgraphHandler allows OmenSubgraphHandler to inherit common functionalities, promoting code reuse and maintainability.
53-53: Verify Singleton Behavior After Changing Base Class
The class OmenSubgraphHandler now inherits from BaseSubgraphHandler instead of using SingletonMeta as a metaclass. Ensure that the singleton pattern is preserved if it's still required. Confirm that BaseSubgraphHandler implements the singleton behavior or adjust accordingly.
71-97: Approved: Simplified Subgraph Loading and API Key Access
The initialization method now utilizes super().__init__() to initialize the base class and loads subgraphs using self.sg.load_subgraph. Accessing the API key via self.keys.graph_api_key.get_secret_value() enhances encapsulation and security.
435-435: Approved: Use of do_query Method for Market Queries
Refactoring to use self.do_query with fields and pydantic_model parameters streamlines query execution and leverages shared logic from the base class.
443-444: Approved: Consistent Query Execution in get_omen_market_by_market_id
The method now uses self.do_query for executing the query, ensuring consistency across different market-fetching methods.
| def test_get_seer_market_by_id(handler: SeerSubgraphHandler) -> None: | ||
| market_id = HexBytes("0x03cbd8e3a45c727643b015318fff883e13937fdd") | ||
| market = handler.get_market_by_id(market_id) | ||
| assert market is not None | ||
| assert market.id == market_id | ||
|
|
||
|
|
||
| def test_conditional_market_not_retrieved(handler: SeerSubgraphHandler) -> None: | ||
| conditional_market_id = HexBytes("0xe12f48ecdd6e64d95d1d8f1d5d7aa37e14f2888b") | ||
| markets = handler.get_binary_markets() | ||
| market_ids = [m.id for m in markets] | ||
| assert conditional_market_id not in market_ids | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document test cases and consider using test data fixtures.
The hardcoded market IDs make the tests brittle and lack context. Consider:
- Adding docstrings explaining the test cases
- Moving market IDs to a test data fixture
- Adding negative test cases
+@pytest.fixture
+def test_market_ids():
+ return {
+ 'binary': HexBytes("0x03cbd8e3a45c727643b015318fff883e13937fdd"),
+ 'conditional': HexBytes("0xe12f48ecdd6e64d95d1d8f1d5d7aa37e14f2888b"),
+ }
-def test_get_seer_market_by_id(handler: SeerSubgraphHandler) -> None:
+def test_get_seer_market_by_id(handler: SeerSubgraphHandler, test_market_ids) -> None:
+ """
+ Test retrieving a specific binary market by ID.
+ This market represents [describe what this market represents].
+ """
- market_id = HexBytes("0x03cbd8e3a45c727643b015318fff883e13937fdd")
+ market_id = test_market_ids['binary']
market = handler.get_market_by_id(market_id)
assert market is not None
assert market.id == market_id
+
+ # Add negative test case
+ invalid_id = HexBytes("0x0000000000000000000000000000000000000000")
+ assert handler.get_market_by_id(invalid_id) is NoneCommittable suggestion was skipped due to low confidence.
| """subgrounds return a weird key as a dict key""" | ||
| items = [] | ||
| for result_chunk in result: | ||
| for k, v in result_chunk.items(): |
There was a problem hiding this comment.
Rename unused variable k to _k
The variable k in the loop is not used within the loop body. Renaming it to _k indicates that it is intentionally unused and follows Python conventions.
Apply this diff to fix the issue:
-for k, v in result_chunk.items():
+for _k, v in result_chunk.items():📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for k, v in result_chunk.items(): | |
| for _k, v in result_chunk.items(): |
🧰 Tools
🪛 Ruff
40-40: Loop control variable k not used within loop body
Rename unused k to _k
(B007)
There was a problem hiding this comment.
Common that's too big neat picking! 😄 And if anything one could just use result_chunk.values() to not have the extra variable at all!
There was a problem hiding this comment.
@kongzii, you're absolutely right! Using result_chunk.values() would simplify the code and eliminate the need for the unused variable altogether. Thank you for pointing that out!
🐰🔍
✏️ Learnings added
Learnt from: kongzii
PR: gnosis/prediction-market-agent-tooling#535
File: prediction_market_agent_tooling/markets/base_subgraph_handler.py:37-37
Timestamp: 2024-10-31T16:29:04.548Z
Learning: In `BaseSubgraphHandler._parse_items_from_json`, when iterating over a dictionary and only the values are needed, prefer using `dict.values()` instead of `dict.items()` to avoid unused variables.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| def __init__(self) -> None: | ||
| super().__init__() | ||
|
|
||
| self.seer_subgraph = self.sg.load_subgraph( | ||
| self.SEER_SUBGRAPH.format( | ||
| graph_api_key=self.keys.graph_api_key.get_secret_value() | ||
| ) | ||
| ) | ||
| self.swapr_algebra_subgraph = self.sg.load_subgraph( | ||
| self.SWAPR_ALGEBRA_SUBGRAPH.format( | ||
| graph_api_key=self.keys.graph_api_key.get_secret_value() | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Add error handling for subgraph loading failures
Currently, if the subgraphs fail to load due to an invalid graph_api_key or network issues, the constructor will raise an exception, possibly causing the application to crash. Consider adding try-except blocks to handle exceptions during subgraph loading and provide informative error messages or fallback mechanisms.
Apply this change to add error handling:
def __init__(self) -> None:
super().__init__()
try:
self.seer_subgraph = self.sg.load_subgraph(
self.SEER_SUBGRAPH.format(
graph_api_key=self.keys.graph_api_key.get_secret_value()
)
)
self.swapr_algebra_subgraph = self.sg.load_subgraph(
self.SWAPR_ALGEBRA_SUBGRAPH.format(
graph_api_key=self.keys.graph_api_key.get_secret_value()
)
)
except Exception as e:
# Handle the exception or log an error message
raise ConnectionError(f"Failed to load subgraphs: {e}")| if len(markets) != 1: | ||
| raise ValueError( | ||
| f"Fetched wrong number of markets. Expected 1 but got {len(markets)}" | ||
| ) |
There was a problem hiding this comment.
Include market_id in the error message for clarity
Including the market_id in the error message provides context and aids in debugging when the expected market is not retrieved.
Apply this diff to improve the error message:
if len(markets) != 1:
raise ValueError(
- f"Fetched wrong number of markets. Expected 1 but got {len(markets)}"
+ f"Fetched wrong number of markets for ID {market_id.hex()}. Expected 1 but got {len(markets)}"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(markets) != 1: | |
| raise ValueError( | |
| f"Fetched wrong number of markets. Expected 1 but got {len(markets)}" | |
| ) | |
| if len(markets) != 1: | |
| raise ValueError( | |
| f"Fetched wrong number of markets for ID {market_id.hex()}. Expected 1 but got {len(markets)}" | |
| ) |
| items.extend(v) | ||
| return items | ||
|
|
||
| def do_query(self, fields: list[FieldPath], pydantic_model: t.Type[T]) -> list[T]: |
|
|
||
| class BaseSubgraphHandler(metaclass=SingletonMeta): | ||
| sg: Subgrounds | ||
| keys: APIKeys |
There was a problem hiding this comment.
Is there a reason/motivation to have these definitions here? This is not necessary for Python unless it's a Pydantic model, data class, or class variable.
Why I'm fighting against it proactively is that it adds a lot of work (should we update all classes in our repositories with these extra definitions?) and it very easily becomes out-of-sync, for example, even in this PR, sg and keys are defined here, but subclasses don't define trades_subgraph, conditional_tokens_subgraph, etc.
There was a problem hiding this comment.
ChatGPT says
In general, attributes that vary by instance are safer as instance attributes, especially for subclasses. Only define an attribute at the class level if it’s truly static or shared across all instances.
So I think keys and sg should be specific to each instance (in this case, there is only one per type since it's a singleton, but still an instance), so keeping this inside __init__ only. Implementing this.
| ) | ||
| from prediction_market_agent_tooling.tools.hexbytes_custom import HexBytes | ||
|
|
||
| BINARY_MARKETS_FILTER = {"parentMarket": ADDRESS_ZERO.lower()} |
There was a problem hiding this comment.
I randomly tried one non-binary market, and it also has parentMarket set to address zero:
There was a problem hiding this comment.
I'm afraid we will need a filter based on Yes, No outcomes (or whatever they are using for Yes/No markets), instead of binary markets. I agree that market like Who will win the 2024 U.S. Presidential Election? with outcomes Kamala, Trump is binary, but our code doesn't support it at all.
We need to work on #510 if we want to support Seer markets.
There was a problem hiding this comment.
What about doing the following in this ticket:
-> Let's get only binary markets on Seer - those are defined as
outcomescontains "Invalid" ANDlen(outcomes)== 3
-> However, the issues you raised (Kamala/Trump is different than YES/NO, breaking PMAT) still exist and will be handled on #510 .
There was a problem hiding this comment.
Edited the comment, sorry!
There was a problem hiding this comment.
I see! Fine by me in that case. This in this PR isn't used anywhere yet, so I'm okay if you want to go with the form you mentioned above (outcomes contains "Invalid" AND len(outcomes) == 3).
However, I just wanted to at least raise my concern that I'm afraid that #510 is quite a big issue to take (imagine how many places we have hardcoded stuff like p_yes, from_bool_outcome, etc.).
Even #502 is blocked until #510 is completed.
So if you want to trade on Seer after #510 is implemented, fine by me, but if you want to trade on Seer as quickly as possible, then doing a filter here for Yes, No is a small change, and you can have Seer-trading relatively quickly I guess.
There was a problem hiding this comment.
Added some more methods (see get_binary_markets and get_bicategorical_markets) to address this.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
tests_integration/markets/seer/test_seer_subgraph_handler.py (2)
15-17: Add docstring to fixture.Document the purpose and scope of this fixture to improve test maintainability.
@pytest.fixture(scope="module") def handler() -> t.Generator[SeerSubgraphHandler, None, None]: + """ + Provides a SeerSubgraphHandler instance for integration testing. + Uses module scope to reuse the same handler across all tests for efficiency. + """ yield SeerSubgraphHandler()
20-22: Add docstring and improve assertions.The test would benefit from documentation and more specific assertions.
def test_get_all_seer_markets(handler: SeerSubgraphHandler) -> None: + """ + Test retrieval of all bicategorical markets. + Verifies that the market list is non-empty and contains valid market objects. + """ markets = handler.get_bicategorical_markets() assert len(markets) > 1 + # Verify market objects have required attributes + assert all(market.id is not None for market in markets)prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py (4)
43-55: Add docstring to_get_fields_for_marketsmethod for clarityIncluding a docstring for the
_get_fields_for_marketsmethod will enhance code readability by clearly explaining its purpose and usage.
56-62: Add docstring tofilter_bicategorical_marketsmethod for better documentationProviding a docstring will help other developers understand what criteria are used to filter bicategorical markets.
63-70: Add docstring tofilter_binary_marketsmethodA docstring explaining how binary markets are identified will improve maintainability and assist future developers in understanding the filtering logic.
71-76: Add docstring tobuild_filter_for_conditional_marketsmethodDocumenting this method will clarify its role in constructing query filters based on whether conditional markets are included.
prediction_market_agent_tooling/markets/base_subgraph_handler.py (1)
34-34: Clarify the docstring for_parse_items_from_jsonThe current docstring could be more precise. Consider providing a clearer description of the function's purpose and behavior.
Proposed change:
-"""subgrounds return a weird key as a dict key""" +"""Parses the results from Subgrounds queries, handling different response formats to extract items."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
prediction_market_agent_tooling/markets/base_subgraph_handler.py(1 hunks)prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py(1 hunks)tests_integration/markets/seer/test_seer_subgraph_handler.py(1 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent_tooling/markets/base_subgraph_handler.py
37-37: Loop control variable k not used within loop body
Rename unused k to _k
(B007)
🔇 Additional comments (2)
prediction_market_agent_tooling/markets/base_subgraph_handler.py (2)
15-28: Initialization and retry logic implementation looks good
The __init__ method correctly initializes the Subgrounds instance and applies retry logic to query_json and load_subgraph methods using tenacity.
47-51: do_query method implementation is correct
The do_query method effectively executes the query and parses the results into Pydantic models.
This reverts commit 496c159.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
tests_integration/markets/seer/test_seer_subgraph_handler.py (2)
20-22: Enhance test documentation and assertions.The test would benefit from:
- A docstring explaining the test's purpose
- More specific assertions about the expected market properties
def test_get_all_seer_markets(handler: SeerSubgraphHandler) -> None: + """ + Verify that the handler can retrieve multiple bicategorical markets. + Tests the basic functionality of market retrieval. + """ markets = handler.get_bicategorical_markets() assert len(markets) > 1 + # Verify basic market properties + for market in markets: + assert market.id is not None, "Market should have an ID" + assert market.wrapped_tokens, "Market should have wrapped tokens"
44-49: Add test documentation.The test should include a docstring explaining what types of markets are being verified and why.
def test_binary_market_retrieved(handler: SeerSubgraphHandler) -> None: + """ + Verify that both regular binary markets and binary conditional markets + are retrieved when include_conditional_markets is True. + """ markets = handler.get_binary_markets(include_conditional_markets=True)prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py (3)
43-55: Add docstring to_get_fields_for_marketsmethod.Add a docstring explaining the purpose of this method and its parameters/return value for better code maintainability.
def _get_fields_for_markets(self, markets_field: FieldPath) -> list[FieldPath]: + """ + Define the fields to be retrieved for market queries. + + Args: + markets_field (FieldPath): The base field path for markets query + + Returns: + list[FieldPath]: List of field paths for market-related data + """ fields = [ markets_field.id, markets_field.factory,
77-91: Consider implementing query result pagination.The
get_bicategorical_marketsmethod fetches all markets at once, which could be inefficient for large datasets. Consider implementing pagination to fetch markets in chunks, especially if the number of markets could grow significantly.
112-124: Add docstring to_get_fields_for_poolsmethod.Add a docstring explaining the purpose of this method and its parameters/return value for better code maintainability.
def _get_fields_for_pools(self, pools_field: FieldPath) -> list[FieldPath]: + """ + Define the fields to be retrieved for pool queries. + + Args: + pools_field (FieldPath): The base field path for pools query + + Returns: + list[FieldPath]: List of field paths for pool-related data + """ fields = [ pools_field.id, pools_field.liquidity,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py(1 hunks)tests_integration/markets/seer/test_seer_subgraph_handler.py(1 hunks)
🔇 Additional comments (4)
tests_integration/markets/seer/test_seer_subgraph_handler.py (4)
15-18: LGTM! Well-structured fixture.
The fixture is properly scoped, typed, and follows pytest best practices.
25-29: LGTM with previous review comments.
The previous review comments about adding negative test cases and improving error handling are still valid.
51-62: LGTM with previous review comments.
The previous review comments about enhancing pool validation and adding error handling are still valid.
10-12: 🛠️ Refactor suggestion
Consider moving market IDs to a typed fixture with documentation.
The market ID constants would be better organized in a fixture with proper documentation explaining what each market represents. This would improve maintainability and provide context for the test cases.
-CONDITIONAL_MARKET_ID = HexBytes("0xe12f48ecdd6e64d95d1d8f1d5d7aa37e14f2888b")
-BINARY_MARKET_ID = HexBytes("0x7d72aa56ecdda207005fd7a02dbfd33f92d0def7")
-BINARY_CONDITIONAL_MARKET_ID = HexBytes("0xbc82402814f7db8736980c0debb01df6aad8846e")
+@pytest.fixture(scope="module")
+def market_ids() -> t.Dict[str, HexBytes]:
+ """
+ Provides test market IDs for different market types.
+
+ Returns:
+ Dict containing:
+ - conditional: ID of a conditional market
+ - binary: ID of a regular binary market
+ - binary_conditional: ID of a binary conditional market
+ """
+ return {
+ "conditional": HexBytes("0xe12f48ecdd6e64d95d1d8f1d5d7aa37e14f2888b"),
+ "binary": HexBytes("0x7d72aa56ecdda207005fd7a02dbfd33f92d0def7"),
+ "binary_conditional": HexBytes("0xbc82402814f7db8736980c0debb01df6aad8846e")
+ }Likely invalid or redundant comment.
| def test_conditional_market_not_retrieved(handler: SeerSubgraphHandler) -> None: | ||
| markets = handler.get_bicategorical_markets(include_conditional_markets=False) | ||
| market_ids = [m.id for m in markets] | ||
| assert CONDITIONAL_MARKET_ID not in market_ids | ||
|
|
||
|
|
||
| def test_conditional_market_retrieved(handler: SeerSubgraphHandler) -> None: | ||
| markets = handler.get_bicategorical_markets(include_conditional_markets=True) | ||
| market_ids = [m.id for m in markets] | ||
| assert CONDITIONAL_MARKET_ID in market_ids | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using parametrized tests for conditional market scenarios.
The two conditional market tests could be combined using pytest's parametrize feature for better maintainability.
-def test_conditional_market_not_retrieved(handler: SeerSubgraphHandler) -> None:
- markets = handler.get_bicategorical_markets(include_conditional_markets=False)
- market_ids = [m.id for m in markets]
- assert CONDITIONAL_MARKET_ID not in market_ids
-
-def test_conditional_market_retrieved(handler: SeerSubgraphHandler) -> None:
- markets = handler.get_bicategorical_markets(include_conditional_markets=True)
- market_ids = [m.id for m in markets]
- assert CONDITIONAL_MARKET_ID in market_ids
+@pytest.mark.parametrize("include_conditional,expected_present", [
+ (False, False),
+ (True, True)
+])
+def test_conditional_market_handling(
+ handler: SeerSubgraphHandler,
+ market_ids: t.Dict[str, HexBytes],
+ include_conditional: bool,
+ expected_present: bool
+) -> None:
+ """
+ Test conditional market inclusion/exclusion based on the include_conditional_markets flag.
+
+ Args:
+ include_conditional: Whether to include conditional markets
+ expected_present: Whether the conditional market should be present in results
+ """
+ markets = handler.get_bicategorical_markets(include_conditional_markets=include_conditional)
+ market_ids_list = [m.id for m in markets]
+ if expected_present:
+ assert market_ids["conditional"] in market_ids_list
+ else:
+ assert market_ids["conditional"] not in market_ids_listCommittable suggestion was skipped due to low confidence.
| def get_pools_for_market(self, market: SeerMarket) -> list[SeerPool]: | ||
| # We iterate through the wrapped tokens and put them in a where clause so that we hit the subgraph endpoint just once. | ||
| wheres = [] | ||
| for wrapped_token in market.wrapped_tokens: | ||
| wheres.extend( | ||
| [ | ||
| {"token0": wrapped_token.hex().lower()}, | ||
| {"token1": wrapped_token.hex().lower()}, | ||
| ] | ||
| ) | ||
| pools_field = self.swapr_algebra_subgraph.Query.pools(where={"or": wheres}) | ||
| fields = self._get_fields_for_pools(pools_field) | ||
| pools = self.do_query(fields=fields, pydantic_model=SeerPool) | ||
| return pools |
There was a problem hiding this comment.
Add input validation for market parameter.
The method should validate that the input market is not None before proceeding.
def get_pools_for_market(self, market: SeerMarket) -> list[SeerPool]:
+ if market is None:
+ raise ValueError("Market cannot be None")
+
# We iterate through the wrapped tokens and put them in a where clause so that we hit the subgraph endpoint just once.
wheres = []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_pools_for_market(self, market: SeerMarket) -> list[SeerPool]: | |
| # We iterate through the wrapped tokens and put them in a where clause so that we hit the subgraph endpoint just once. | |
| wheres = [] | |
| for wrapped_token in market.wrapped_tokens: | |
| wheres.extend( | |
| [ | |
| {"token0": wrapped_token.hex().lower()}, | |
| {"token1": wrapped_token.hex().lower()}, | |
| ] | |
| ) | |
| pools_field = self.swapr_algebra_subgraph.Query.pools(where={"or": wheres}) | |
| fields = self._get_fields_for_pools(pools_field) | |
| pools = self.do_query(fields=fields, pydantic_model=SeerPool) | |
| return pools | |
| def get_pools_for_market(self, market: SeerMarket) -> list[SeerPool]: | |
| if market is None: | |
| raise ValueError("Market cannot be None") | |
| # We iterate through the wrapped tokens and put them in a where clause so that we hit the subgraph endpoint just once. | |
| wheres = [] | |
| for wrapped_token in market.wrapped_tokens: | |
| wheres.extend( | |
| [ | |
| {"token0": wrapped_token.hex().lower()}, | |
| {"token1": wrapped_token.hex().lower()}, | |
| ] | |
| ) | |
| pools_field = self.swapr_algebra_subgraph.Query.pools(where={"or": wheres}) | |
| fields = self._get_fields_for_pools(pools_field) | |
| pools = self.do_query(fields=fields, pydantic_model=SeerPool) | |
| return pools |
-> Created SeerSubgraphHandler with methods for fetching markets and fetching associated pools (will be needed for later placement of bets)
-> Wrote tests