Replicator fixes #673
Replicator fixes #673
Conversation
WalkthroughThis pull request modifies the market replication and resolution process. Key changes include renaming the function for finalizing and resolving replicated markets, removing an outdated redemption function call, and introducing a new function to redeem user positions. Additionally, the logic for market state validation has been refined to check if a market is open rather than resolved. Various scripts have been updated to reflect these changes, including modifications to import statements and enhancements to market liquidity statistics. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent
participant OmenModule
participant Market
Agent->>OmenModule: Initiate market unfunding
OmenModule->>Market: Log and process unfunding of replicated markets
Agent->>OmenModule: Trigger finalization and resolution
OmenModule->>OmenModule: Call renamed finalization function (…_replicated_markets_tx)
OmenModule->>OmenModule: Redeem all user positions
OmenModule->>OmenModule: Check resolution via find_resolution_on_other_markets_or_manually
Market-->>OmenModule: Return resolution status
OmenModule-->>Agent: Return finalization result
Possibly related PRs
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (5)
✨ Finishing Touches
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 (
|
| @@ -226,7 +227,7 @@ def omen_unfund_replicated_known_markets_tx( | |||
| # Optionally, if `saturation_above_threshold` is provided, skip markets that are not saturated to leave some free money motivation for agents. | |||
| if ( | |||
| saturation_above_threshold is not None | |||
| and not market.is_resolved | |||
| and market.is_open | |||
There was a problem hiding this comment.
Edge-case, if market is closed, but isn't resolved for some reason (ie bug), we should still unfund it to get money back.
![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 (1)
prediction_market_agent/agents/replicate_to_omen_agent/omen_replicate.py (1)
230-230:⚠️ Potential issuePotential regression in market unfunding logic.
The change from
not market.is_resolvedtomarket.is_openmight prevent unfunding markets that are closed but not resolved (e.g., due to bugs), which was identified as an important edge case in past review comments. This could lead to funds being locked in such markets.Apply this diff to restore the ability to unfund closed but unresolved markets:
- and market.is_open + and not market.is_resolved
🧹 Nitpick comments (2)
scripts/replicator_stats.py (1)
78-89: Consider refactoring duplicated liquidity calculations.The liquidity calculation logic is repeated multiple times with only the market filter changing. Consider extracting this into a helper function to reduce code duplication.
+def calculate_market_liquidity(markets): + return sum( + OmenAgentMarket.from_data_model(m).get_liquidity().amount + for m in markets + ) stats = { # ... - "liquidity in open markets closing in more than 1 days": sum( - [ - OmenAgentMarket.from_data_model(m).get_liquidity().amount - for m in markets_closing_in_more_than_1_days - ] - ), - "liquidity in open markets closing in more than 3 days": sum( - [ - OmenAgentMarket.from_data_model(m).get_liquidity().amount - for m in markets_closing_in_more_than_3_days - ] - ), + "liquidity in open markets closing in more than 1 days": calculate_market_liquidity(markets_closing_in_more_than_1_days), + "liquidity in open markets closing in more than 3 days": calculate_market_liquidity(markets_closing_in_more_than_3_days), # Update other similar calculations...prediction_market_agent/agents/replicate_to_omen_agent/omen_resolve_replicated.py (1)
139-168: Consider adding retry mechanism for manual resolution.While the function's logic and error handling are good, consider adding retries for transient failures during manual resolution. This would improve reliability when the
ofv_answer_binary_questioncall fails due to temporary issues.@observe() def find_resolution_on_other_markets_or_manually( market: OmenMarket, api_keys: APIKeys, ) -> Resolution | None: # Try to find resolution on other markets. resolution = find_resolution_on_other_markets(market) # Sometimes questions can be no longer found (for example Manifold allows to rephrase questions), # in that case, resolve it with our resolver. if resolution is None: logger.info( "[REPLICATOR-RESOLUTION-NOT-FOUND] Resolution not found on other markets. Trying to resolve manually." ) + max_retries = 3 + retry_delay = 1 # seconds + attempt = 0 try: - fact_check = ofv_answer_binary_question(market.question_title, api_keys) + while attempt < max_retries: + try: + fact_check = ofv_answer_binary_question(market.question_title, api_keys) + break + except Exception as e: + attempt += 1 + if attempt == max_retries: + raise + logger.warning( + f"Attempt {attempt}/{max_retries} failed for market {market.url}. Retrying in {retry_delay}s. Error: {e}" + ) + time.sleep(retry_delay) + retry_delay *= 2 # Exponential backoff resolution = ( None if fact_check is None or fact_check.factuality is None else Resolution.from_bool(fact_check.factuality) ) except Exception as e: logger.exception( f"Exception while getting factuality for market {market.url=}. Skipping. Exception: {e}" ) else: logger.info( f"[REPLICATOR-RESOLUTION-FOUND] Resolution {resolution} found on other markets for {market.url=}." ) return resolution
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lock,!**/*.lockpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (6)
prediction_market_agent/agents/replicate_to_omen_agent/deploy.py(2 hunks)prediction_market_agent/agents/replicate_to_omen_agent/omen_replicate.py(3 hunks)prediction_market_agent/agents/replicate_to_omen_agent/omen_resolve_replicated.py(6 hunks)scripts/replicate_for_hackathon.py(2 hunks)scripts/replicator_stats.py(5 hunks)scripts/resolve_replicated_on_omen.py(2 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/replicate_for_hackathon.py (1)
Learnt from: evangriffiths
PR: gnosis/prediction-market-agent#503
File: prediction_market_agent/agents/replicate_to_omen_agent/deploy.py:68-70
Timestamp: 2024-11-12T08:09:15.282Z
Learning: The function `omen_finalize_and_resolve_and_claim_back_all_markets_based_on_others_tx` includes the `realitio_bond` parameter.
🔇 Additional comments (9)
scripts/resolve_replicated_on_omen.py (1)
11-11: LGTM! Function renaming improves clarity.The renamed function better describes its purpose by explicitly mentioning "replicated markets". The changes are consistent and properly applied across imports and usage.
Also applies to: 13-13, 39-39
scripts/replicate_for_hackathon.py (1)
16-16: LGTM! Consistent function renaming.The changes maintain consistency with the function renaming across the codebase and correctly use the
realitio_bondparameter.Also applies to: 42-42
prediction_market_agent/agents/replicate_to_omen_agent/deploy.py (1)
62-73: LGTM! Improved workflow clarity.The updated comment clearly indicates that unfunding markets is the first operation, and the function renaming is consistent with other files.
scripts/replicator_stats.py (1)
23-23: LGTM! Improved time handling.Using
utcnow()ensures consistent time handling across different timezones.prediction_market_agent/agents/replicate_to_omen_agent/omen_resolve_replicated.py (3)
17-19: LGTM!The new imports are correctly placed and properly organized.
Also applies to: 36-38
54-54: LGTM!The new function name better reflects its purpose by emphasizing that it deals with replicated markets.
124-125: LGTM!The redemption call is correctly placed after market resolution to ensure funds can be redeemed.
prediction_market_agent/agents/replicate_to_omen_agent/omen_replicate.py (2)
27-27: LGTM!The new import is correctly placed and properly organized.
249-250: LGTM!The redemption call is correctly placed after unfunding markets to ensure funds can be redeemed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
prediction_market_agent/agents/replicate_to_omen_agent/omen_resolve_replicated.py (1)
140-169: Consider enhancing error logging in the manual resolution fallback.The function implements a good fallback strategy, but the error logging could be more detailed to help with debugging.
Consider this enhancement:
except Exception as e: logger.exception( - f"Exception while getting factuality for market {market.url=}. Skipping. Exception: {e}" + f"[REPLICATOR-RESOLUTION-ERROR] Failed to resolve market {market.url=} manually. " + f"Question: {market.question_title}. Exception: {e}" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lock,!**/*.lockpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (1)
prediction_market_agent/agents/replicate_to_omen_agent/omen_resolve_replicated.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-build-image
- GitHub Check: pytest-docker
- GitHub Check: pytest
- GitHub Check: mypy
- GitHub Check: build-and-push-image
🔇 Additional comments (2)
prediction_market_agent/agents/replicate_to_omen_agent/omen_resolve_replicated.py (2)
17-19: LGTM! Import changes align with new functionality.The new imports support the added features for market resolution and user position redemption.
Also applies to: 36-40
54-54: Function name better reflects its purpose and adds user position redemption.The changes improve clarity and completeness:
- Renamed to explicitly mention "replicated markets"
- Added redemption of user positions after market resolution
Also applies to: 127-128
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/replicator_stats.py (2)
51-56: Consider refactoring market filtering logic to reduce duplication.The market filtering logic is repeated with different day values. Consider extracting this into a helper function for better maintainability.
+def get_markets_closing_after_days(markets, now, days): + return [m for m in markets if m.opening_datetime > now + timedelta(days=days)] -markets_closing_in_more_than_3_days = [ - m for m in markets if m.opening_datetime > now + timedelta(days=3) -] -markets_closing_in_more_than_1_days = [ - m for m in markets if m.opening_datetime > now + timedelta(days=1) -] +markets_closing_in_more_than_3_days = get_markets_closing_after_days(markets, now, 3) +markets_closing_in_more_than_1_days = get_markets_closing_after_days(markets, now, 1)
61-61: Consider consistent formatting for stats dictionary entries.While the new statistics are valuable additions, consider aligning the formatting:
- Some entries use parentheses, others don't
- Some entries span multiple lines with different indentation
- "markets without an answer": len( - [m for m in markets if m.currentAnswer is None] - ), + "markets without an answer": len([m for m in markets if m.currentAnswer is None]),Also applies to: 64-66, 81-92, 103-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/replicator_stats.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-build-image
- GitHub Check: pytest-docker
- GitHub Check: pytest
- GitHub Check: mypy
- GitHub Check: build-and-push-image
🔇 Additional comments (3)
scripts/replicator_stats.py (3)
1-1: LGTM! Import changes improve timezone handling.The switch to
utcnow()fromdatetime.now()is a good practice for consistent timezone handling. The new imports support the reality balance feature.Also applies to: 6-8, 13-14
23-23: LGTM! Consistent UTC time handling.Using
utcnow()ensures all market timing calculations are consistently performed in UTC.
58-58: LGTM! Reality balance tracking added.The reality balance calculation is correctly implemented with proper unit conversion using
wei_to_xdai.
| def find_resolution_on_other_markets_or_manually( | ||
| market: OmenMarket, | ||
| api_keys: APIKeys, | ||
| ) -> Resolution | None: |
There was a problem hiding this comment.
I would rephrase, manually -> using_ofv_resolver or similar.
Manually at first glance means doing something outside of the script.
build please