Trading Seer markets using cow #612
Conversation
…-market-creation-on-pmat # Conflicts: # prediction_market_agent_tooling/markets/seer/data_models.py
…-market-creation-on-pmat # Conflicts: # pyproject.toml
…---cow-py-or-swapr # Conflicts: # prediction_market_agent_tooling/markets/seer/data_models.py # prediction_market_agent_tooling/markets/seer/seer.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
prediction_market_agent_tooling/markets/seer/data_models.py (2)
110-115: 🛠️ Refactor suggestionImprove error handling in has_valid_answer method.
Re-raise the exception with its original context to make debugging more transparent.
try: self.outcome_as_enums[SeerOutcomeEnum.NEUTRAL] - except KeyError: + except KeyError as e: raise ValueError( f"Market {self.id.hex()} has no invalid outcome. {self.outcomes}" - ) + ) from e🧰 Tools
🪛 Ruff (0.8.2)
113-115: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
176-188:⚠️ Potential issueAdd guard against division by zero in current_p_yes.
The sum of price data values could be zero, leading to a ZeroDivisionError.
yes_idx = self.outcome_as_enums[SeerOutcomeEnum.POSITIVE] - price_yes = price_data[yes_idx] / sum(price_data.values()) - return Probability(price_yes) + total_price = sum(price_data.values()) + if total_price == 0: + return Probability(0) + return Probability(price_data[yes_idx] / total_price)prediction_market_agent_tooling/markets/seer/seer.py (1)
264-272: 🛠️ Refactor suggestionSimplify auto-deposit logic.
- Use the class's
collateral_token_contract_address_checksummedproperty instead of hardcoding sDai.- Remove unnecessary
convertToAssetscall asauto_deposithandles that automatically.- collateral_contract = sDaiContract() + collateral_contract = init_collateral_token_contract( + self.collateral_token_contract_address_checksummed + ) if auto_deposit: - asset_amount = collateral_contract.convertToAssets( - xdai_to_wei(xdai_type(amount.amount)) - ) auto_deposit_collateral_token( - collateral_contract, asset_amount, api_keys, web3 + collateral_contract, + xdai_to_wei(xdai_type(amount.amount)), + api_keys, + web3 )
🧹 Nitpick comments (5)
prediction_market_agent_tooling/markets/seer/data_models.py (2)
50-53: Consider renaming outcome enums to YES/NO/INVALID.The current naming (POSITIVE/NEGATIVE/NEUTRAL) might be confusing as:
- A "NO" outcome isn't necessarily negative
- An "INVALID" outcome isn't neutral
- The code already uses "yes" in pattern matching and property names (e.g.,
current_p_yes)class SeerOutcomeEnum(str, Enum): - POSITIVE = "positive" - NEGATIVE = "negative" - NEUTRAL = "neutral" + YES = "yes" + NO = "no" + INVALID = "invalid"
145-148: Make is_binary method more outcome-agnostic.Instead of checking for exactly 3 outcomes, check if the last outcome is "Invalid". This makes the method more flexible for markets with different numbers of valid outcomes.
@property def is_binary(self) -> bool: - # 3 because Seer has also third, `Invalid` outcome. - return len(self.outcomes) == 3 + # Check if the market has exactly two valid outcomes plus an Invalid outcome + try: + return self.outcomes[-1].lower().strip() == "invalid" + except IndexError: + return Falseprediction_market_agent_tooling/markets/seer/seer.py (3)
69-76: Consider adding a default value for description.Based on past review comments,
description=Noneis used frequently. Consider adding a default value in the class definition to avoid passingNoneevery timeSeerAgentMarketis initialized.class SeerAgentMarket(AgentMarket): currency = Currency.sDai + description = None # Seer markets don't have descriptions wrapped_tokens: list[ChecksumAddress] creator: HexAddress
212-233: Consider making the baseline liquidity check value configurable.The hardcoded 1 xDai baseline value for liquidity checks might not be suitable for all scenarios. Consider making this value configurable, especially since agents can bet up to 25 xDai per trade.
+ # Default baseline value for liquidity checks + LIQUIDITY_CHECK_BASELINE = xdai_type(1) + def has_liquidity_for_outcome(self, outcome: bool) -> bool: outcome_token = self.get_wrapped_token_for_outcome(outcome) try: CowManager().get_quote( collateral_token=self.collateral_token_contract_address_checksummed, buy_token=outcome_token, - sell_amount=xdai_to_wei(xdai_type(1)), + sell_amount=xdai_to_wei(self.LIQUIDITY_CHECK_BASELINE), )
257-257: Remove redundant liquidity check.The
has_liquidity_for_outcomecheck is redundant sincecan_be_traded()already checkshas_liquidity(), which verifies liquidity for both outcomes.- if not self.has_liquidity_for_outcome(outcome): - raise ValueError( - f"Market {self.id} does not have liquidity for this {outcome=}" - )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
prediction_market_agent_tooling/markets/seer/data_models.py(2 hunks)prediction_market_agent_tooling/markets/seer/seer.py(1 hunks)prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py
🧰 Additional context used
🪛 Ruff (0.8.2)
prediction_market_agent_tooling/markets/seer/data_models.py
113-115: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: pytest - Python 3.12.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.12.x - Unit Tests
- GitHub Check: pytest - Python 3.11.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.11.x - Unit Tests
- GitHub Check: pytest - Python 3.10.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.10.x - Unit Tests
🔇 Additional comments (4)
prediction_market_agent_tooling/markets/seer/seer.py (4)
1-67: LGTM! Well-organized imports and justified constant value.The imports are logically grouped, and the
SEER_TINY_BET_AMOUNTconstant's value is properly justified with a comment explaining its larger value compared to Omen.
77-164: LGTM! Well-documented core market operations.The core market operations are well-implemented with proper documentation. The empty
redeem_winningsmethod is properly tracked with a TODO and an issue link (#499).
294-367: LGTM! Well-implemented market creation logic.The market creation implementation is thorough with proper error handling, documentation, and event processing.
196-210:❓ Verification inconclusive
Verify usage of unused parameters in get_binary_markets.
The method accepts
created_afterandexcluded_questionsparameters but doesn't pass them toSeerSubgraphHandler().get_binary_markets(). Verify if these parameters should be used or removed.
🏁 Script executed:
#!/bin/bash # Check if these parameters are used in the subgraph handler ast-grep --pattern $'class SeerSubgraphHandler { $$$ get_binary_markets($_, $_, $_, created_after: $_, excluded_questions: $_) { $$$ } $$$ }'Length of output: 154
Once you've reviewed the output of the above script, please verify if the method definition for
get_binary_marketsin theSeerSubgraphHandlerclass actually accepts and utilizes the parameterscreated_afterandexcluded_questions. If these parameters are not used anywhere in its definition, then you should either modify the call withinseer.pyto pass these parameters appropriately or remove them from the signature inget_binary_marketsto avoid confusion.
Action Required: Update handling of unused parameters in get_binary_markets
- The
get_binary_marketsmethod inprediction_market_agent_tooling/markets/seer/seer.pyacceptscreated_afterandexcluded_questionsparameters which are not forwarded to the subgraph handler'sget_binary_marketsfunction.- Please verify if these parameters are intended for future functionality. If they are required, update the call to include:
Otherwise, if they are not needed, remove them from the function signature.SeerSubgraphHandler().get_binary_markets( limit=limit, sort_by=sort_by, filter_by=filter_by, created_after=created_after, excluded_questions=excluded_questions, )
🏁 Script executed:
#!/bin/bash # Search for the definition of get_binary_markets across the repository using rg. rg -n "def get_binary_markets" .Length of output: 770
Review of Unused Parameters in
get_binary_marketsThe
get_binary_marketsmethod inprediction_market_agent_tooling/markets/seer/seer.pyacceptscreated_afterandexcluded_questionsparameters, but these are not forwarded to the call inSeerSubgraphHandler().get_binary_markets(). Please verify the intended functionality:
- Check the signature in
prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.pyto confirm whether these parameters are expected.- If
created_afterandexcluded_questionsare meant to filter or adjust behavior in the subgraph handler, update the call accordingly:SeerSubgraphHandler().get_binary_markets( limit=limit, sort_by=sort_by, filter_by=filter_by, created_after=created_after, excluded_questions=excluded_questions, )- Otherwise, if they are not required, remove these parameters from the
get_binary_marketsmethod signature to prevent confusion.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
prediction_market_agent_tooling/markets/seer/seer.py (1)
243-293: 🛠️ Refactor suggestionUse market's collateral token instead of hardcoded sDai.
The function uses hardcoded sDai contract instead of the market's collateral token. This might break if the market uses a different collateral token.
Apply this diff to use market's collateral token:
- collateral_contract = sDaiContract() + collateral_contract = init_collateral_token_contract( + self.collateral_token_contract_address_checksummed + )
🧹 Nitpick comments (4)
prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py (2)
66-69: Update the comment to reflect the actual filtering logic.The comment about checking invalid outcome is outdated. Based on past review comments, invalid outcomes are always present in Seer markets.
Apply this diff to update the comment:
- # We do an extra check for the invalid outcome for safety. + # Filter markets with exactly 3 outcomes (2 categories + invalid outcome).
114-134: Track the TODO comment for implementing liquidity conditions.The function correctly handles sorting parameters, but liquidity-based sorting is not implemented yet.
Do you want me to open a new issue to track the implementation of liquidity conditions for sorting markets?
prediction_market_agent_tooling/markets/seer/seer.py (2)
164-166: Track the TODO comment for implementing redeem_winnings.The function needs to be implemented to handle redeeming winnings.
Do you want me to help implement the redeem_winnings functionality? The implementation would be based on issue #499.
214-229: Make the baseline value configurable.The function uses a hardcoded 1 xDai as a baseline value for checking liquidity. This might not be suitable for all trades, especially when agents bet up to 25 xDai per trade.
Apply this diff to make the baseline value configurable:
+ DEFAULT_LIQUIDITY_CHECK_AMOUNT = xdai_type(1) + def has_liquidity_for_outcome( - self, outcome: bool + self, + outcome: bool, + amount: xdai_type | None = None ) -> bool: outcome_token = self.get_wrapped_token_for_outcome(outcome) try: CowManager().get_quote( collateral_token=self.collateral_token_contract_address_checksummed, buy_token=outcome_token, - sell_amount=xdai_to_wei(xdai_type(1)), + sell_amount=xdai_to_wei(amount or self.DEFAULT_LIQUIDITY_CHECK_AMOUNT), ) return True except NoLiquidityAvailableOnCowException: logger.info( - f"Could not get a quote for {outcome_token=} {outcome=}, returning no liquidity" + f"Could not get a quote for {outcome_token=} {outcome=} {amount=}, returning no liquidity" ) return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
prediction_market_agent_tooling/markets/seer/seer.py(1 hunks)prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py(5 hunks)tests_integration_with_local_chain/markets/seer/test_seer_agent_market.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: pytest - Python 3.12.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.12.x - Integration Tests
- GitHub Check: pytest - Python 3.12.x - Unit Tests
- GitHub Check: pytest - Python 3.11.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.11.x - Integration Tests
- GitHub Check: pytest - Python 3.11.x - Unit Tests
- GitHub Check: pytest - Python 3.10.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.10.x - Integration Tests
- GitHub Check: pytest - Python 3.10.x - Unit Tests
- GitHub Check: mypy
🔇 Additional comments (18)
prediction_market_agent_tooling/markets/seer/seer_subgraph_handler.py (6)
45-64: LGTM!The function correctly defines the fields needed to fetch market details from the subgraph.
71-77: LGTM!The function correctly filters binary markets by checking for "yes" and "no" outcomes in a case-insensitive manner.
80-112: LGTM!The function correctly builds filter conditions for the subgraph query based on the FilterBy enum and binary market requirements.
136-167: LGTM!The function correctly fetches and filters bicategorical markets with proper handling of optional parameters.
169-187: LGTM!The function correctly fetches and filters binary markets by reusing the bicategorical markets logic.
212-225: LGTM!The function correctly fetches Swapr pools for a market by filtering on wrapped token addresses.
prediction_market_agent_tooling/markets/seer/seer.py (12)
80-87: LGTM!The function correctly documents that predictions are stored with trades on Seer.
88-99: LGTM!The function correctly delegates trade storage to the external store_trades function.
101-107: LGTM!The function correctly validates currency compatibility and converts bet amount to wei.
108-123: LGTM!The function correctly calculates the number of outcome tokens for a given bet by:
- Getting the wrapped token for the outcome
- Converting bet amount to wei
- Getting a quote from CowManager
- Converting the quote amount back to xDai
125-128: LGTM!The function correctly converts boolean outcome to string by using SeerOutcomeEnum and seer_outcomes mapping.
131-132: LGTM!The function correctly reuses OmenAgentMarket's get_trade_balance implementation.
135-136: LGTM!The function correctly returns a tiny bet amount with the appropriate currency.
138-157: LGTM!The function correctly fetches user position by:
- Getting wrapped token for each outcome
- Getting token balance for each outcome
- Converting balances to xDai
- Ignoring invalid balances
160-161: LGTM!The function correctly reuses OmenAgentMarket's get_user_id implementation.
169-174: LGTM!The function correctly verifies operational balance by checking if total balance is greater than 0.001.
177-195: LGTM!The function correctly creates SeerAgentMarket instance from data model with proper field mapping and address conversion.
198-212: LGTM!The function correctly fetches binary markets and converts them to SeerAgentMarket instances.
tests_integration_with_local_chain/markets/seer/test_seer_agent_market.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests_integration/markets/seer/test_seer_data_model.py (1)
14-35: 🛠️ Refactor suggestionImprove test reliability by using pre-calculated values.
The test currently replicates the same logic as the implementation, which could mask potential bugs. Consider using pre-calculated values or a different calculation method to validate the probability.
Apply this diff to improve the test:
def test_current_p_yes( seer_subgraph_handler_test: SeerSubgraphHandler, cow_manager: CowManager ) -> None: - # We fetch many markets because non YES/NO markets are also fetched. - market = seer_subgraph_handler_test.get_binary_markets( - limit=100, sort_by=SortBy.HIGHEST_LIQUIDITY, filter_by=FilterBy.OPEN - )[0] - yes_idx = market.outcome_as_enums[SeerOutcomeEnum.YES] - yes_token = market.wrapped_tokens[yes_idx] - yes_price = market._get_price_for_token(Web3.to_checksum_address(yes_token)) - - no_idx = market.outcome_as_enums[SeerOutcomeEnum.NO] - no_token = market.wrapped_tokens[no_idx] - no_price = market._get_price_for_token(Web3.to_checksum_address(no_token)) - - invalid_idx = market.outcome_as_enums[SeerOutcomeEnum.INVALID] - invalid_token = market.wrapped_tokens[invalid_idx] - invalid_price = market._get_price_for_token(Web3.to_checksum_address(invalid_token)) - - current_p_yes = market.current_p_yes - expected_p_yes = yes_price / (yes_price + no_price + invalid_price) - assert np.isclose(current_p_yes, expected_p_yes, atol=0.01) + # Mock the market with known token prices + market = seer_subgraph_handler_test.get_binary_markets( + limit=1, sort_by=SortBy.HIGHEST_LIQUIDITY, filter_by=FilterBy.OPEN + )[0] + + # Mock _get_price_for_token to return known values + def mock_get_price(token: str) -> float: + prices = { + Web3.to_checksum_address(market.wrapped_tokens[market.outcome_as_enums[SeerOutcomeEnum.YES]]): 0.6, + Web3.to_checksum_address(market.wrapped_tokens[market.outcome_as_enums[SeerOutcomeEnum.NO]]): 0.3, + Web3.to_checksum_address(market.wrapped_tokens[market.outcome_as_enums[SeerOutcomeEnum.INVALID]]): 0.1, + } + return prices[token] + + market._get_price_for_token = mock_get_price # type: ignore + + # With these mocked prices, p_yes should be 0.6 + assert np.isclose(market.current_p_yes, 0.6, atol=0.01)prediction_market_agent_tooling/markets/seer/data_models.py (2)
110-117:⚠️ Potential issueImprove exception handling in has_valid_answer method.
The exception handling should preserve the original error context.
Apply this diff to improve error handling:
try: self.outcome_as_enums[SeerOutcomeEnum.INVALID] - except KeyError: + except KeyError as e: raise ValueError( f"Market {self.id.hex()} has no invalid outcome. {self.outcomes}" - ) + ) from e🧰 Tools
🪛 Ruff (0.8.2)
113-115: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
177-188:⚠️ Potential issueAdd division by zero protection in current_p_yes property.
The property could raise a ZeroDivisionError if all prices are zero.
Apply this diff to add protection:
@property def current_p_yes(self) -> Probability: price_data = {} for idx in range(len(self.outcomes)): wrapped_token = self.wrapped_tokens[idx] price = self._get_price_for_token( token=Web3.to_checksum_address(wrapped_token) ) price_data[idx] = price yes_idx = self.outcome_as_enums[SeerOutcomeEnum.YES] - price_yes = price_data[yes_idx] / sum(price_data.values()) - return Probability(price_yes) + total_price = sum(price_data.values()) + if total_price == 0: + return Probability(0) + return Probability(price_data[yes_idx] / total_price)
🧹 Nitpick comments (2)
tests_integration/markets/seer/test_seer_data_model.py (1)
38-42: Enhance test coverage for market resolution.The test only covers a single case (Resolution.YES) with a hardcoded market ID. Consider adding test cases for other resolutions and validating the market's state.
Apply this diff to improve test coverage:
-def test_resolution() -> None: - resolved_market_id = HexBytes("0xd32068304199c1885b02d3ac91e0da06b9568409") - market = SeerSubgraphHandler().get_market_by_id(market_id=resolved_market_id) - resolution = market.get_resolution_enum() - assert resolution == Resolution.YES +def test_resolution_cases() -> None: + test_cases = [ + ( + HexBytes("0xd32068304199c1885b02d3ac91e0da06b9568409"), + Resolution.YES, + "Market resolved as YES" + ), + # Add more test cases for NO and INVALID resolutions + ] + + for market_id, expected_resolution, description in test_cases: + market = SeerSubgraphHandler().get_market_by_id(market_id=market_id) + + # Verify market state + assert market.is_resolved, f"{description}: Market should be resolved" + assert market.has_valid_answer, f"{description}: Market should have valid answer" + + # Check resolution + resolution = market.get_resolution_enum() + assert resolution == expected_resolution, descriptionprediction_market_agent_tooling/markets/seer/data_models.py (1)
74-74: Improve error message in from_string method.The current error message could be more helpful by listing the valid patterns.
Apply this diff to improve the error message:
- raise ValueError(f"Could not map {value=} to an outcome.") + raise ValueError( + f"Could not map {value=} to an outcome. " + "Valid patterns are: 'yes', 'no', 'invalid', 'invalid result'" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
prediction_market_agent_tooling/markets/seer/data_models.py(2 hunks)tests_integration/markets/seer/test_seer_data_model.py(1 hunks)tests_integration/markets/seer/test_seer_outcome.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests_integration/markets/seer/test_seer_outcome.py
🧰 Additional context used
🪛 Ruff (0.8.2)
prediction_market_agent_tooling/markets/seer/data_models.py
113-115: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: pytest - Python 3.12.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.12.x - Unit Tests
- GitHub Check: pytest - Python 3.11.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.11.x - Unit Tests
- GitHub Check: pytest - Python 3.10.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.10.x - Unit Tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
prediction_market_agent_tooling/markets/seer/data_models.py (2)
115-129: 🛠️ Refactor suggestionImprove error handling in has_valid_answer method.
The error handling can be improved by including the original exception context.
Apply this diff to improve error handling:
try: self.outcome_as_enums[SeerOutcomeEnum.INVALID] - except KeyError: + except KeyError as e: raise ValueError( f"Market {self.id.hex()} has no invalid outcome. {self.outcomes}" - ) + ) from e🧰 Tools
🪛 Ruff (0.8.2)
124-126: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
209-221: 🛠️ Refactor suggestionImprove error handling granularity in _get_price_for_token.
The method currently catches all exceptions and returns 0, which could mask serious issues.
Apply this diff to improve error handling:
def _get_price_for_token(self, token: ChecksumAddress) -> float: collateral_exchange_amount = xdai_to_wei(xdai_type(1)) try: quote = CowManager().get_quote( collateral_token=self.collateral_token_contract_address_checksummed, buy_token=token, sell_amount=collateral_exchange_amount, ) except Exception as e: - logger.warning(f"Could not get quote for {token=}, returning price 0. {e=}") + if isinstance(e, (ValueError, KeyError)): + # Expected errors (e.g., no liquidity), return 0 + logger.warning(f"Could not get quote for token {token}, returning price 0: {str(e)}") + return 0 + # Log unexpected errors and re-raise + logger.error(f"Unexpected error getting quote for token {token}: {str(e)}") + raise return 0 return collateral_exchange_amount / float(quote.quote.buyAmount.root)
🧹 Nitpick comments (2)
prediction_market_agent_tooling/markets/seer/data_models.py (1)
189-207: Enhance price calculation robustness.The current_p_yes property handles the zero division case but could be more explicit about the conditions under which this occurs.
Consider adding a more descriptive log message:
if sum(price_data.values()) == 0: logger.warning( - f"Could not get p_yes for market {self.id.hex()}, all price quotes are 0." + f"Could not get p_yes for market {self.id.hex()}, all price quotes are 0. " + f"This might indicate no liquidity or issues with price discovery." ) return Probability(0)prediction_market_agent_tooling/markets/seer/seer.py (1)
214-235: Optimize liquidity checks.The
has_liquiditymethod performs redundant checks by callinghas_liquidity_for_outcometwice, which makes two separate API calls. Consider caching the results.Apply this diff to optimize liquidity checks:
def has_liquidity(self) -> bool: - # We conservatively define a market as having liquidity if it has liquidity for the `True` outcome token AND the `False` outcome token. - return self.has_liquidity_for_outcome(True) and self.has_liquidity_for_outcome( - False - ) + # Cache liquidity checks to avoid redundant API calls + has_true_liquidity = self.has_liquidity_for_outcome(True) + if not has_true_liquidity: + return False + return self.has_liquidity_for_outcome(False)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
prediction_market_agent_tooling/markets/seer/data_models.py(2 hunks)prediction_market_agent_tooling/markets/seer/seer.py(1 hunks)prediction_market_agent_tooling/tools/cow/cow_manager.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
prediction_market_agent_tooling/tools/cow/cow_manager.py
90-90: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
prediction_market_agent_tooling/markets/seer/data_models.py
124-126: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: pytest - Python 3.12.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.12.x - Integration Tests
- GitHub Check: pytest - Python 3.12.x - Unit Tests
- GitHub Check: pytest - Python 3.11.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.11.x - Integration Tests
- GitHub Check: pytest - Python 3.11.x - Unit Tests
- GitHub Check: pytest - Python 3.10.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.10.x - Integration Tests
- GitHub Check: pytest - Python 3.10.x - Unit Tests
- GitHub Check: mypy
🔇 Additional comments (6)
prediction_market_agent_tooling/markets/seer/data_models.py (2)
50-86: LGTM! Well-structured outcome handling.The implementation provides robust outcome handling with clear conversions between different formats and good error handling.
223-227: LGTM! Clean URL generation.The URL generation is clean and uses the correct chain ID from configuration.
prediction_market_agent_tooling/tools/cow/cow_manager.py (2)
30-33: LGTM!The exception class is well-documented and follows Python's exception hierarchy.
36-42: LGTM!The initialization is clean and well-documented, setting up the order book API with appropriate configuration.
prediction_market_agent_tooling/markets/seer/seer.py (2)
69-78: LGTM!The class attributes are well-defined and documented, with a clear explanation for the
descriptionfield beingNone.
80-99: LGTM!The
store_predictionandstore_tradesmethods are well-implemented, with clear documentation explaining the relationship between predictions and trades in Seer markets.
| except UnexpectedResponseError as e1: | ||
| if "NoLiquidity" in e1.message: | ||
| raise NoLiquidityAvailableOnCowException(e1.message) | ||
| return self._log_and_raise_exception(e1) | ||
| except Exception as e: | ||
| return self._log_and_raise_exception(e) |
There was a problem hiding this comment.
@kongzii if you have a better idea on how to make mypy stop complaining about try-except constructs (see python/mypy#8964 (comment)), feel free to edit directly
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
prediction_market_agent_tooling/markets/seer/data_models.py (2)
115-128: 🛠️ Refactor suggestionImprove error handling in has_valid_answer property.
Use the 'raise from' pattern to preserve the exception context.
Apply this diff:
try: self.outcome_as_enums[SeerOutcomeEnum.INVALID] - except KeyError: + except KeyError as e: raise ValueError( f"Market {self.id.hex()} has no invalid outcome. {self.outcomes}" - ) + ) from e🧰 Tools
🪛 Ruff (0.8.2)
123-125: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
209-221: 🛠️ Refactor suggestionEnhance error handling in _get_price_for_token method.
The current implementation catches all exceptions and returns 0, which could mask serious issues.
Apply this diff to improve error handling:
def _get_price_for_token(self, token: ChecksumAddress) -> float: collateral_exchange_amount = xdai_to_wei(xdai_type(1)) try: quote = CowManager().get_quote( collateral_token=self.collateral_token_contract_address_checksummed, buy_token=token, sell_amount=collateral_exchange_amount, ) - except Exception as e: - logger.warning(f"Could not get quote for {token=}, returning price 0. {e=}") + except Exception as e: + if isinstance(e, (ValueError, KeyError)): + # Expected errors for missing quotes + logger.warning( + f"Could not get quote for token {token}, returning price 0. Error: {str(e)}" + ) + return 0 + # Log unexpected errors and re-raise + logger.error( + f"Unexpected error getting quote for token {token}. Error: {str(e)}" + ) + raise return 0 return collateral_exchange_amount / float(quote.quote.buyAmount.root)
🧹 Nitpick comments (2)
prediction_market_agent_tooling/tools/cow/cow_manager.py (1)
42-42: Document the precision value.The hardcoded precision value of 18 should be documented to explain why this specific value is used for ERC1155 wrapped tokens.
Apply this diff to add a more detailed comment:
- self.precision = 18 # number of token decimals from ERC1155 wrapped tokens. + # Standard precision for ERC1155 wrapped tokens on Gnosis Chain + # This value represents the number of decimal places (1e18) used for token amounts + self.precision = 18prediction_market_agent_tooling/deploy/betting_strategy.py (1)
84-94: Update docstring to reflect boolean-based outcome determination.The docstring example uses numeric outcomes (0, 1) but the implementation uses boolean values. Consider updating the example to align with the actual implementation.
""" This helper method builds trades by rebalancing token allocations to each outcome. - For example, if we have an existing position with 10 tokens in outcome 0 and 5 in outcome 1, - and our target position is 20 tokens in outcome 0 and 0 in outcome 1, we would return these trades: + For example, if we have an existing position with 10 tokens in outcome True and 5 in outcome False, + and our target position is 20 tokens in outcome True and 0 in outcome False, we would return these trades: trades = [ - Trade(outcome=0, amount=10, trade_type=TradeType.BUY), - Trade(outcome=1, amount=5, trade_type=TradeType.SELL) + Trade(outcome=True, amount=10, trade_type=TradeType.BUY), + Trade(outcome=False, amount=5, trade_type=TradeType.SELL) ] Note that we order the trades to first buy then sell, in order to minimally tilt the odds so that sell price is higher. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (3)
prediction_market_agent_tooling/deploy/betting_strategy.py(1 hunks)prediction_market_agent_tooling/markets/seer/data_models.py(2 hunks)prediction_market_agent_tooling/tools/cow/cow_manager.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
prediction_market_agent_tooling/tools/cow/cow_manager.py
98-98: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
prediction_market_agent_tooling/markets/seer/data_models.py
123-125: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: pytest - Python 3.12.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.12.x - Unit Tests
- GitHub Check: pytest - Python 3.11.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.11.x - Unit Tests
- GitHub Check: pytest - Python 3.10.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.10.x - Unit Tests
🔇 Additional comments (7)
prediction_market_agent_tooling/tools/cow/cow_manager.py (2)
96-101: Improve error handling.The error handling needs improvement to:
- Use
raise ... from errto preserve the exception context- Be more specific about caught exceptions
Apply this diff to improve error handling:
except UnexpectedResponseError as e1: if "NoLiquidity" in e1.message: - raise NoLiquidityAvailableOnCowException(e1.message) + raise NoLiquidityAvailableOnCowException(e1.message) from e1 return self._log_and_raise_exception(e1) - except Exception as e: + except (ValueError, TypeError, RuntimeError) as e: return self._log_and_raise_exception(e)🧰 Tools
🪛 Ruff (0.8.2)
98-98: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
103-121: Add error handling for swap failures.The
swapmethod should handle potential failures fromswap_tokens_waiting.Apply this diff to add error handling:
@staticmethod def swap( amount: xDai, sell_token: ChecksumAddress, buy_token: ChecksumAddress, api_keys: APIKeys, web3: Web3 | None = None, ) -> OrderMetaData: - order_metadata = swap_tokens_waiting( - amount_wei=xdai_to_wei(amount), - sell_token=sell_token, - buy_token=buy_token, - api_keys=api_keys, - web3=web3, - ) - logger.debug( - f"Purchased {buy_token} in exchange for {sell_token}. Order details {order_metadata}" - ) - return order_metadata + try: + order_metadata = swap_tokens_waiting( + amount_wei=xdai_to_wei(amount), + sell_token=sell_token, + buy_token=buy_token, + api_keys=api_keys, + web3=web3, + ) + logger.debug( + f"Purchased {buy_token} in exchange for {sell_token}. Order details {order_metadata}" + ) + return order_metadata + except Exception as e: + logger.error(f"Failed to swap tokens: {e}") + raise ValueError(f"Failed to swap tokens: {e}") from eprediction_market_agent_tooling/deploy/betting_strategy.py (2)
96-97: LGTM! The outcome determination logic has been simplified.The changes improve the code by:
- Directly iterating over boolean values instead of using an intermediate list comprehension
- Using the market's instance method for outcome determination, which aligns with the refactoring of
get_outcome_str_from_boolfrom static to instance method
96-97:❓ Verification inconclusive
Verify the outcome determination across all betting strategies.
The changes to outcome determination affect all betting strategies. Please ensure that the market's instance method
get_outcome_str_from_boolreturns consistent results across different market types (e.g., Omen, Seer).Run the following script to verify the outcome determination:
🏁 Script executed:
#!/bin/bash # Description: Check outcome determination implementation across market types # Search for get_outcome_str_from_bool implementations ast-grep --pattern 'def get_outcome_str_from_bool' # Search for market type definitions rg -A 5 'class.*Market.*:'Length of output: 20350
Attention: Outcome Determination Consistency Check Required
Please ensure that the market instance method
get_outcome_str_from_boolreturns consistent results for bothTrueandFalseoutcomes across all betting strategies. While trade ordering (line 122), error handling (lines 107–108), and position rebalancing logic continue to work as expected, you should verify that outcome determination is implemented uniformly among the following strategies:
- MaxAccuracyBettingStrategy
- MaxExpectedValueBettingStrategy
- KellyBettingStrategy
- MaxAccuracyWithKellyScaledBetsStrategy
Additionally, review all market type implementations (e.g., Omen, Seer, Polymarket, etc.) to confirm that if any overrides exist, they deliver a consistent behavior (for example, mapping
Trueto one specific outcome string andFalseto another). Since our search did not surface an explicit definition forget_outcome_str_from_bool, please double-check that this method is inherited or implemented as intended across all relevant market classes.prediction_market_agent_tooling/markets/seer/data_models.py (3)
1-24: LGTM! Well-organized imports.The imports are logically grouped and all are being utilized in the implementation.
49-85: LGTM! Clean enum implementation with robust conversion methods.The implementation provides clear outcome mapping with proper error handling and comprehensive string pattern matching.
190-207: LGTM! Robust handling of zero prices.The implementation properly handles the case where all price quotes are zero, preventing division by zero errors.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
prediction_market_agent_tooling/tools/cow/cow_manager.py (2)
94-99: 🛠️ Refactor suggestionImprove error handling to comply with best practices.
The error handling needs improvement to comply with B904:
- Add proper exception chaining using
raise ... from err.- Be more specific about the exceptions being caught.
except UnexpectedResponseError as e1: if "NoLiquidity" in e1.message: - raise NoLiquidityAvailableOnCowException(e1.message) + raise NoLiquidityAvailableOnCowException(e1.message) from e1 return self._log_and_raise_exception(e1) - except Exception as e: + except (ValueError, TypeError, RuntimeError) as e: return self._log_and_raise_exception(e)🧰 Tools
🪛 Ruff (0.8.2)
96-96: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
101-119: 🛠️ Refactor suggestionAdd error handling for swap failures.
The
swapmethod should handle potential failures fromswap_tokens_waiting.@staticmethod def swap( amount: xDai, sell_token: ChecksumAddress, buy_token: ChecksumAddress, api_keys: APIKeys, web3: Web3 | None = None, ) -> OrderMetaData: - order_metadata = swap_tokens_waiting( - amount_wei=xdai_to_wei(amount), - sell_token=sell_token, - buy_token=buy_token, - api_keys=api_keys, - web3=web3, - ) - logger.debug( - f"Purchased {buy_token} in exchange for {sell_token}. Order details {order_metadata}" - ) - return order_metadata + try: + order_metadata = swap_tokens_waiting( + amount_wei=xdai_to_wei(amount), + sell_token=sell_token, + buy_token=buy_token, + api_keys=api_keys, + web3=web3, + ) + logger.debug( + f"Purchased {buy_token} in exchange for {sell_token}. Order details {order_metadata}" + ) + return order_metadata + except Exception as e: + logger.error(f"Failed to swap tokens: {e}") + raise ValueError(f"Failed to swap tokens: {e}") from e
🧹 Nitpick comments (2)
prediction_market_agent_tooling/markets/omen/omen.py (1)
489-492: Consider keeping the method as static.The
get_outcome_str_from_boolmethod doesn't use any instance attributes. Converting it to an instance method adds unnecessary instance binding. Since it's a pure function that converts a boolean to a string, it should remain a static method.Apply this diff to revert to a static method:
- def get_outcome_str_from_bool(self, outcome: bool) -> OutcomeStr: + @staticmethod + def get_outcome_str_from_bool(outcome: bool) -> OutcomeStr:prediction_market_agent_tooling/tools/cow/cow_manager.py (1)
35-41: Consider making token precision configurable.The precision is hardcoded to 18 decimals. While this works for most ERC1155 wrapped tokens, consider making it configurable to support tokens with different decimal places.
- def __init__(self) -> None: + def __init__(self, precision: int = 18) -> None: self.order_book_api = OrderBookApi( OrderBookAPIConfigFactory.get_config(COW_ENV, SupportedChainId.GNOSIS_CHAIN) ) - self.precision = 18 # number of token decimals from ERC1155 wrapped tokens. + self.precision = precision # number of token decimals from ERC1155 wrapped tokens.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
prediction_market_agent_tooling/markets/omen/omen.py(5 hunks)prediction_market_agent_tooling/tools/cow/cow_manager.py(1 hunks)tests/test_betting_strategy.py(4 hunks)tests_integration/markets/seer/test_seer_subgraph_handler.py(1 hunks)tests_integration_with_local_chain/markets/seer/test_seer_agent_market.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_betting_strategy.py
- tests_integration_with_local_chain/markets/seer/test_seer_agent_market.py
- tests_integration/markets/seer/test_seer_subgraph_handler.py
🧰 Additional context used
🪛 Ruff (0.8.2)
prediction_market_agent_tooling/tools/cow/cow_manager.py
96-96: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: pytest - Python 3.12.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.12.x - Integration Tests
- GitHub Check: pytest - Python 3.12.x - Unit Tests
- GitHub Check: pytest - Python 3.11.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.11.x - Integration Tests
- GitHub Check: pytest - Python 3.11.x - Unit Tests
- GitHub Check: pytest - Python 3.10.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.10.x - Integration Tests
- GitHub Check: pytest - Python 3.10.x - Unit Tests
🔇 Additional comments (5)
prediction_market_agent_tooling/markets/omen/omen.py (3)
30-33: LGTM!The new imports from
blockchain_utilsare correctly placed and used throughout the code.
202-224: LGTM!The parameter rename from
omen_auto_deposittoauto_depositimproves clarity by removing the redundant "omen" prefix while maintaining the same functionality.
428-439: LGTM!The
store_tradesmethod has been effectively refactored to use a common implementation fromblockchain_utils, reducing code duplication while maintaining the required interface.prediction_market_agent_tooling/tools/cow/cow_manager.py (2)
1-29: LGTM!The imports are well-organized and logically grouped. The COW_ENV constant is appropriately set for production use.
31-33: LGTM!The custom exception class is well-defined with a clear docstring.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/monitor/test_monitor.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: pytest - Python 3.12.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.12.x - Integration Tests
- GitHub Check: pytest - Python 3.12.x - Unit Tests
- GitHub Check: pytest - Python 3.11.x - Integration with Local Chain
- GitHub Check: mypy
- GitHub Check: pytest - Python 3.11.x - Integration Tests
- GitHub Check: pytest - Python 3.11.x - Unit Tests
- GitHub Check: pytest - Python 3.10.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.10.x - Integration Tests
- GitHub Check: pytest - Python 3.10.x - Unit Tests
| if market_type != MarketType.SEER: | ||
| # Seer agent not deployed yet. | ||
| cls = MARKET_TYPE_TO_DEPLOYED_AGENT[market_type] | ||
| agents = cls.from_all_gcp_functions() | ||
| if len(agents) == 0: | ||
| pytest.skip(f"No deployed agents found for {market_type}") | ||
|
|
||
| monitor_agent(agents[0]) | ||
| monitor_agent(agents[0]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the test skip more explicit for SEER market type.
The current implementation silently skips the test for SEER market type without any indication. This could hide the fact that SEER market type is not being tested.
Consider this improved implementation:
- if market_type != MarketType.SEER:
- # Seer agent not deployed yet.
- cls = MARKET_TYPE_TO_DEPLOYED_AGENT[market_type]
- agents = cls.from_all_gcp_functions()
- if len(agents) == 0:
- pytest.skip(f"No deployed agents found for {market_type}")
-
- monitor_agent(agents[0])
+ if market_type == MarketType.SEER:
+ pytest.skip("SEER market type monitoring is not implemented yet as the agent is not deployed")
+ return
+
+ cls = MARKET_TYPE_TO_DEPLOYED_AGENT[market_type]
+ agents = cls.from_all_gcp_functions()
+ if len(agents) == 0:
+ pytest.skip(f"No deployed agents found for {market_type}")
+
+ monitor_agent(agents[0])This change:
- Makes the skip for SEER market type explicit with a descriptive message
- Improves code readability by reducing nesting
- Provides better context about why SEER market type is skipped
📝 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 market_type != MarketType.SEER: | |
| # Seer agent not deployed yet. | |
| cls = MARKET_TYPE_TO_DEPLOYED_AGENT[market_type] | |
| agents = cls.from_all_gcp_functions() | |
| if len(agents) == 0: | |
| pytest.skip(f"No deployed agents found for {market_type}") | |
| monitor_agent(agents[0]) | |
| monitor_agent(agents[0]) | |
| if market_type == MarketType.SEER: | |
| pytest.skip("SEER market type monitoring is not implemented yet as the agent is not deployed") | |
| return | |
| cls = MARKET_TYPE_TO_DEPLOYED_AGENT[market_type] | |
| agents = cls.from_all_gcp_functions() | |
| if len(agents) == 0: | |
| pytest.skip(f"No deployed agents found for {market_type}") | |
| monitor_agent(agents[0]) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/monitor/test_monitor.py (1)
21-28: 🛠️ Refactor suggestionMake the test skip more explicit for SEER market type.
The current implementation silently skips the test for SEER market type without any indication. This could hide the fact that SEER market type is not being tested.
Consider this improved implementation:
- if market_type != MarketType.SEER: - # Seer agent not deployed yet. - cls = MARKET_TYPE_TO_DEPLOYED_AGENT[market_type] - agents = cls.from_all_gcp_functions() - if len(agents) == 0: - pytest.skip(f"No deployed agents found for {market_type}") - - monitor_agent(agents[0]) + if market_type == MarketType.SEER: + pytest.skip("SEER market type monitoring is not implemented yet as the agent is not deployed") + return + + cls = MARKET_TYPE_TO_DEPLOYED_AGENT[market_type] + agents = cls.from_all_gcp_functions() + if len(agents) == 0: + pytest.skip(f"No deployed agents found for {market_type}") + + monitor_agent(agents[0])This change:
- Makes the skip for SEER market type explicit with a descriptive message
- Improves code readability by reducing nesting
- Provides better context about why SEER market type is skipped
🧹 Nitpick comments (3)
prediction_market_agent_tooling/tools/cow/cow_manager.py (3)
2-2: Remove unused import.The
NoReturntype hint is imported but not used in the code.-from typing import NoReturn
35-41: Consider making token precision configurable.The token precision is hardcoded to 18 decimals. While this works for most ERC1155 tokens, consider making it configurable to support tokens with different decimal places.
- def __init__(self) -> None: + def __init__(self, precision: int = 18) -> None: self.order_book_api = OrderBookApi( OrderBookAPIConfigFactory.get_config(COW_ENV, SupportedChainId.GNOSIS_CHAIN) ) - self.precision = 18 # number of token decimals from ERC1155 wrapped tokens. + self.precision = precision # number of token decimals from ERC1155 wrapped tokens.
87-94: Improve error handling specificity.The generic
Exceptioncatch block could be more specific to handle expected error types.except UnexpectedResponseError as e1: if "NoLiquidity" in e1.message: raise NoLiquidityAvailableOnCowException(e1.message) logger.warning(f"Found unexpected Cow response error: {e1}") raise - except Exception as e: + except (ValueError, RuntimeError, asyncio.TimeoutError) as e: logger.warning(f"Found unhandled Cow response error: {e}") raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (4)
prediction_market_agent_tooling/deploy/agent.py(1 hunks)prediction_market_agent_tooling/tools/cow/cow_manager.py(1 hunks)tests/monitor/test_monitor.py(1 hunks)tests_integration/markets/seer/test_seer_data_model.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/deploy/agent.py
🧰 Additional context used
🪛 Ruff (0.8.2)
prediction_market_agent_tooling/tools/cow/cow_manager.py
1-1: No such file or directory (os error 2)
(E902)
tests_integration/markets/seer/test_seer_data_model.py
1-1: No such file or directory (os error 2)
(E902)
🔇 Additional comments (4)
tests/monitor/test_monitor.py (1)
12-16: LGTM! The code correctly handles the temporary exclusion of the SEER market type.The TODO comment provides clear context about why the assertion is skipped for the SEER market type, and the assertion correctly verifies that all other market types are mapped.
tests_integration/markets/seer/test_seer_data_model.py (1)
1-8: LGTM!The imports are well-organized, specific, and all imported entities are used in the test function.
🧰 Tools
🪛 Ruff (0.8.2)
1-1: No such file or directory (os error 2)
(E902)
prediction_market_agent_tooling/tools/cow/cow_manager.py (2)
31-33: LGTM!Well-defined custom exception with clear documentation.
96-114: Add error handling for swap failures.The
swapmethod should handle potential failures fromswap_tokens_waiting.@staticmethod def swap( amount: xDai, sell_token: ChecksumAddress, buy_token: ChecksumAddress, api_keys: APIKeys, web3: Web3 | None = None, ) -> OrderMetaData: - order_metadata = swap_tokens_waiting( - amount_wei=xdai_to_wei(amount), - sell_token=sell_token, - buy_token=buy_token, - api_keys=api_keys, - web3=web3, - ) - logger.debug( - f"Purchased {buy_token} in exchange for {sell_token}. Order details {order_metadata}" - ) - return order_metadata + try: + order_metadata = swap_tokens_waiting( + amount_wei=xdai_to_wei(amount), + sell_token=sell_token, + buy_token=buy_token, + api_keys=api_keys, + web3=web3, + ) + logger.debug( + f"Purchased {buy_token} in exchange for {sell_token}. Order details {order_metadata}" + ) + return order_metadata + except Exception as e: + logger.error(f"Failed to swap tokens: {e}") + raise ValueError(f"Failed to swap tokens: {e}") from e
| def test_resolution() -> None: | ||
| resolved_market_id = HexBytes("0xd32068304199c1885b02d3ac91e0da06b9568409") | ||
| market = SeerSubgraphHandler().get_market_by_id(market_id=resolved_market_id) | ||
| resolution = market.get_resolution_enum() | ||
| assert resolution == Resolution.YES |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test robustness and maintainability.
The test has several areas for improvement:
- The hardcoded market ID makes the test brittle and environment-dependent.
- Missing documentation about test purpose and market ID choice.
- No error handling for edge cases.
- Limited test coverage of resolution states.
Consider applying these improvements:
+from typing import Final
+
+# Market ID that is known to be resolved as YES
+# This market asked "<insert question>" and was resolved on <date>
+RESOLVED_YES_MARKET_ID: Final = HexBytes("0xd32068304199c1885b02d3ac91e0da06b9568409")
+
def test_resolution() -> None:
- resolved_market_id = HexBytes("0xd32068304199c1885b02d3ac91e0da06b9568409")
- market = SeerSubgraphHandler().get_market_by_id(market_id=resolved_market_id)
+ """Test market resolution retrieval for a known resolved market.
+
+ This test verifies that:
+ 1. We can fetch a resolved market
+ 2. The resolution state is correctly interpreted
+ """
+ market = SeerSubgraphHandler().get_market_by_id(market_id=RESOLVED_YES_MARKET_ID)
+ assert market is not None, f"Market {RESOLVED_YES_MARKET_ID} not found"
+ assert market.is_resolved, f"Market {RESOLVED_YES_MARKET_ID} is not resolved"
resolution = market.get_resolution_enum()
assert resolution == Resolution.YESAdditionally, consider adding more test cases:
def test_resolution_states() -> None:
"""Test resolution retrieval for markets in different states."""
test_cases = [
(RESOLVED_YES_MARKET_ID, Resolution.YES),
(RESOLVED_NO_MARKET_ID, Resolution.NO),
(RESOLVED_INVALID_MARKET_ID, Resolution.INVALID),
]
for market_id, expected_resolution in test_cases:
market = SeerSubgraphHandler().get_market_by_id(market_id=market_id)
assert market is not None, f"Market {market_id} not found"
assert market.is_resolved, f"Market {market_id} is not resolved"
resolution = market.get_resolution_enum()
assert resolution == expected_resolution
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
prediction_market_agent_tooling/tools/cow/cow_manager.py (2)
86-93: 🛠️ Refactor suggestionVerify error handling improvements.
The error handling has been improved based on previous feedback:
- NoLiquidity check is done before logging
- Warning level is used for unexpected errors
- However, the
raisestatements still need to be updated to include the causeApply this diff to properly chain exceptions:
except UnexpectedResponseError as e1: if "NoLiquidity" in e1.message: - raise NoLiquidityAvailableOnCowException(e1.message) + raise NoLiquidityAvailableOnCowException(e1.message) from e1 logger.warning(f"Found unexpected Cow response error: {e1}") - raise + raise e1 except Exception as e: logger.warning(f"Found unhandled Cow response error: {e}") - raise + raise e from None
95-113: 🛠️ Refactor suggestionAdd error handling for swap_tokens_waiting.
The swap method should handle potential failures from swap_tokens_waiting.
Apply this diff to add error handling:
@staticmethod def swap( amount: xDai, sell_token: ChecksumAddress, buy_token: ChecksumAddress, api_keys: APIKeys, web3: Web3 | None = None, ) -> OrderMetaData: - order_metadata = swap_tokens_waiting( - amount_wei=xdai_to_wei(amount), - sell_token=sell_token, - buy_token=buy_token, - api_keys=api_keys, - web3=web3, - ) - logger.debug( - f"Purchased {buy_token} in exchange for {sell_token}. Order details {order_metadata}" - ) - return order_metadata + try: + order_metadata = swap_tokens_waiting( + amount_wei=xdai_to_wei(amount), + sell_token=sell_token, + buy_token=buy_token, + api_keys=api_keys, + web3=web3, + ) + logger.debug( + f"Purchased {buy_token} in exchange for {sell_token}. Order details {order_metadata}" + ) + return order_metadata + except Exception as e: + logger.error(f"Failed to swap tokens: {e}") + raise ValueError(f"Failed to swap tokens: {e}") from e
🧹 Nitpick comments (1)
prediction_market_agent_tooling/tools/cow/cow_manager.py (1)
34-40: Document the precision value.The hardcoded precision value of 18 should be documented to explain its significance for ERC1155 wrapped tokens.
def __init__(self) -> None: self.order_book_api = OrderBookApi( OrderBookAPIConfigFactory.get_config(COW_ENV, SupportedChainId.GNOSIS_CHAIN) ) - self.precision = 18 # number of token decimals from ERC1155 wrapped tokens. + # Standard precision for ERC1155 wrapped tokens (18 decimal places) + # This value is used for token amount calculations + self.precision = 18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (1)
prediction_market_agent_tooling/tools/cow/cow_manager.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
prediction_market_agent_tooling/tools/cow/cow_manager.py
1-1: No such file or directory (os error 2)
(E902)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: pytest - Python 3.12.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.12.x - Unit Tests
- GitHub Check: pytest - Python 3.11.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.11.x - Unit Tests
- GitHub Check: pytest - Python 3.10.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.10.x - Unit Tests
🔇 Additional comments (2)
prediction_market_agent_tooling/tools/cow/cow_manager.py (2)
1-28: LGTM! Well-organized imports and clear constant definition.The imports are properly organized and the constant is well-defined.
🧰 Tools
🪛 Ruff (0.8.2)
1-1: No such file or directory (os error 2)
(E902)
30-32: LGTM! Well-defined custom exception.The exception class is properly named and documented.
No description provided.