Refactor prediction_market_agent_tooling/markets #23
Conversation
WalkthroughThe recent updates focus on refactoring and reorganizing the prediction market agent tooling, particularly around the handling and interaction with Manifold and Omen prediction markets. Major changes include the introduction of the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files selected for processing (16)
- examples/monitor/monitor.py (1 hunks)
- prediction_market_agent_tooling/deploy/agent.py (2 hunks)
- prediction_market_agent_tooling/deploy/agent_example.py (1 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (1 hunks)
- prediction_market_agent_tooling/markets/data_models.py (2 hunks)
- prediction_market_agent_tooling/markets/manifold/api.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold/data_models.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (1 hunks)
- prediction_market_agent_tooling/markets/markets.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (17 hunks)
- prediction_market_agent_tooling/monitor/markets/manifold.py (1 hunks)
- scripts/bet_omen.py (3 hunks)
- tests/deploy/test_deploy.py (1 hunks)
- tests/markets/test_manifold.py (1 hunks)
- tests/markets/test_omen.py (3 hunks)
Additional comments: 7
tests/markets/test_manifold.py (1)
- 4-7: The updated import statements correctly reflect the structural changes in the codebase, ensuring that the specific functions
pick_binary_marketandplace_betare directly imported fromprediction_market_agent_tooling.markets.manifold.api. This change aligns with the PR objectives to streamline the codebase and improve maintainability.prediction_market_agent_tooling/markets/manifold/data_models.py (5)
- 21-53: The
ManifoldMarketclass is well-structured and uses appropriate types for its fields. However, ensure that thepfield (line 44), representing probability, is always within the range [0, 1]. This might require additional validation logic.Consider adding a validator method within the
ManifoldMarketclass to ensurepis within the expected range.
- 54-66: The method
is_resolved_non_cancelledprovides a clear way to check if a market is resolved and not cancelled. This is a good example of encapsulating logic within the model for maintainability and readability.- 72-76: The
ProfitCachedmodel usesManafor its fields, which is consistent with the domain model. Ensure that theManatype correctly handles the precision required for financial calculations.- 107-111: The
ManifoldBetFillsmodel usesDecimalfor thesharesfield, which is appropriate for financial calculations. Ensure consistency in usingDecimalfor all financial-related fields across models.- 166-186: The
ManifoldContractMetricmodel is comprehensive and uses appropriate types for its fields. Ensure that thetotalSharesdictionary's keys and values are correctly handled in all use cases, especially considering the dynamic nature of dictionary keys.prediction_market_agent_tooling/markets/omen/omen.py (1)
- 56-61: The static method
get_binary_marketsin theOmenAgentMarketclass is a good example of providing a clear interface for retrieving market data. Ensure that theget_omen_binary_marketsfunction it calls is correctly implemented and handles potential errors or edge cases.
| market = pick_binary_market() | ||
| print("Placing bet on market:", market.question) | ||
| manifold.place_bet(mana_type(0.01), market.id, True) | ||
| place_bet(mana_type(1), market.id, True) |
There was a problem hiding this comment.
The usage of pick_binary_market to select a market and then placing a bet on it using place_bet demonstrates a clear and straightforward test of the functionality. However, consider adding more comprehensive tests to cover edge cases or failure scenarios to ensure robustness.
Consider adding tests that cover various scenarios, including invalid market IDs, handling of API failures, and verification of the bet placement outcome.
| class ManifoldPool(BaseModel): | ||
| NO: float | ||
| YES: float |
There was a problem hiding this comment.
The ManifoldPool class defines NO and YES as float. Consider using Decimal for financial calculations to avoid floating-point arithmetic issues.
- NO: float
- YES: float
+ NO: Decimal
+ YES: DecimalCommittable 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.
| class ManifoldPool(BaseModel): | |
| NO: float | |
| YES: float | |
| class ManifoldPool(BaseModel): | |
| NO: Decimal | |
| YES: Decimal |
| class ManifoldUser(BaseModel): | ||
| """ | ||
| https://docs.manifold.markets/api#get-v0userusername | ||
| """ | ||
|
|
||
| id: str | ||
| createdTime: datetime | ||
| name: str | ||
| username: str | ||
| url: str | ||
| avatarUrl: t.Optional[str] = None | ||
| bio: t.Optional[str] = None | ||
| bannerUrl: t.Optional[str] = None | ||
| website: t.Optional[str] = None | ||
| twitterHandle: t.Optional[str] = None | ||
| discordHandle: t.Optional[str] = None | ||
| isBot: t.Optional[bool] = None | ||
| isAdmin: t.Optional[bool] = None | ||
| isTrustworthy: t.Optional[bool] = None | ||
| isBannedFromPosting: t.Optional[bool] = None | ||
| userDeleted: t.Optional[bool] = None | ||
| balance: Mana | ||
| totalDeposits: Mana | ||
| lastBetTime: t.Optional[datetime] = None | ||
| currentBettingStreak: t.Optional[int] = None | ||
| profitCached: ProfitCached |
There was a problem hiding this comment.
The ManifoldUser model includes optional fields for various user attributes. Consider adding documentation or comments to clarify under what conditions these fields might be None. This can improve the maintainability of the code by making it easier for future developers to understand the data model.
| class ManifoldBetFees(BaseModel): | ||
| platformFee: Decimal | ||
| liquidityFee: Decimal | ||
| creatorFee: Decimal | ||
|
|
||
| def get_total(self) -> Decimal: | ||
| return Decimal(sum([self.platformFee, self.liquidityFee, self.creatorFee])) |
There was a problem hiding this comment.
The method get_total in the ManifoldBetFees model is a good example of encapsulating logic within the model. Consider adding error handling for potential arithmetic issues, such as overflow or underflow, when summing the fees.
| class ManifoldBet(BaseModel): | ||
| """ | ||
| https://docs.manifold.markets/api#get-v0bets | ||
| """ | ||
|
|
||
| shares: Decimal | ||
| probBefore: Probability | ||
| isFilled: t.Optional[bool] = None | ||
| probAfter: Probability | ||
| userId: str | ||
| amount: Mana | ||
| contractId: str | ||
| id: str | ||
| fees: ManifoldBetFees | ||
| isCancelled: t.Optional[bool] = None | ||
| loanAmount: Mana | ||
| orderAmount: t.Optional[Mana] = None | ||
| fills: t.Optional[list[ManifoldBetFills]] = None | ||
| createdTime: datetime | ||
| outcome: str | ||
|
|
||
| def get_resolved_boolean_outcome(self) -> bool: | ||
| outcome = Resolution(self.outcome) | ||
| if outcome == Resolution.YES: | ||
| return True | ||
| elif outcome == Resolution.NO: | ||
| return False | ||
| else: | ||
| should_not_happen(f"Unexpected bet outcome string, '{outcome.value}'.") | ||
|
|
||
| def get_profit(self, market_outcome: bool) -> ProfitAmount: | ||
| profit = ( | ||
| self.shares - self.amount | ||
| if self.get_resolved_boolean_outcome() == market_outcome | ||
| else -self.amount | ||
| ) | ||
| profit -= self.fees.get_total() | ||
| return ProfitAmount( | ||
| amount=profit, | ||
| currency=Currency.Mana, | ||
| ) |
There was a problem hiding this comment.
The ManifoldBet model's method get_profit calculates the profit correctly. However, consider adding more detailed documentation to explain the calculation, especially the logic behind subtracting self.amount and adjusting by fees. This can improve code readability and maintainability.
| class OmenAgentMarket(AgentMarket): | ||
| """ | ||
| Omen's market class that can be used by agents to make predictions. | ||
| """ | ||
|
|
||
| currency: Currency = Currency.xDai | ||
| collateral_token_contract_address_checksummed: ChecksumAddress | ||
| market_maker_contract_address_checksummed: ChecksumAddress | ||
|
|
||
| def get_tiny_bet_amount(self) -> BetAmount: | ||
| return BetAmount(amount=Decimal(0.00001), currency=self.currency) | ||
|
|
||
| def place_bet( | ||
| self, outcome: bool, amount: BetAmount, omen_auto_deposit: bool = True | ||
| ) -> None: | ||
| if amount.currency != self.currency: | ||
| raise ValueError(f"Omen bets are made in xDai. Got {amount.currency}.") | ||
| amount_xdai = xDai(amount.amount) | ||
| keys = APIKeys() | ||
| binary_omen_buy_outcome_tx( | ||
| amount=check_not_none(amount_xdai), | ||
| from_address=check_not_none(keys.bet_from_address), | ||
| from_private_key=check_not_none(keys.bet_from_private_key), | ||
| market=self, | ||
| binary_outcome=outcome, | ||
| auto_deposit=omen_auto_deposit, | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def from_data_model(model: OmenMarket) -> "OmenAgentMarket": | ||
| return OmenAgentMarket( | ||
| id=model.id, | ||
| question=model.title, | ||
| outcomes=model.outcomes, | ||
| collateral_token_contract_address_checksummed=model.collateral_token_contract_address_checksummed, | ||
| market_maker_contract_address_checksummed=model.market_maker_contract_address_checksummed, | ||
| ) |
There was a problem hiding this comment.
The OmenAgentMarket class introduces methods for handling bets in Omen markets. Ensure that the place_bet method (lines 30-44) correctly handles the currency check and uses secure practices for interacting with external contracts. Consider adding error handling for the external calls to binary_omen_buy_outcome_tx.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- prediction_market_agent_tooling/deploy/agent.py (2 hunks)
- prediction_market_agent_tooling/markets/manifold/api.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/deploy/agent.py
- prediction_market_agent_tooling/markets/manifold/api.py
| ) | ||
|
|
||
| @staticmethod | ||
| def from_data_model(model: ManifoldMarket) -> "ManifoldAgentMarket": |
There was a problem hiding this comment.
Should this method be defined as NonImplemented in AgentMarket?
There was a problem hiding this comment.
No it doesn't have to be - these methods are only intended to be called inside the respective markets// modules - not from the base class
| Manifold's market class that can be used by agents to make predictions. | ||
| """ | ||
|
|
||
| currency: Currency = Currency.Mana |
There was a problem hiding this comment.
This could be classvar to avoid mis-initialisation.
There was a problem hiding this comment.
Is there an easy way to do this for pydantic classes?
There was a problem hiding this comment.
t.ClassVar[Currency] = Currency. Mana
should work, but will need the same change in the parent class too
There was a problem hiding this comment.
Ah nice. So it doesn't fail on initialisation, but it does fail mypy
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- prediction_market_agent_tooling/markets/agent_market.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (17 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- prediction_market_agent_tooling/markets/agent_market.py
- prediction_market_agent_tooling/markets/manifold/manifold.py
- prediction_market_agent_tooling/markets/omen/omen.py
Additional comments: 2
prediction_market_agent_tooling/monitor/monitor_app.py (2)
- 9-9: The import statement for
get_authenticated_userhas been updated to reflect the new module location. This change is consistent with the PR's objective of reorganizing the codebase for better maintainability and readability. Ensure that all references toget_authenticated_userthroughout the codebase have been updated to match this new import path.Verification successful
The verification process confirms that the import statement for
get_authenticated_userhas been correctly updated inmonitor_app.py, and there are no instances of the old import path remaining in the codebase. This change aligns with the PR's objective of reorganizing the codebase for better maintainability and readability.* 6-12: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-48]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old import paths that might still be in use. Expecting no results. rg --type py 'from prediction_market_agent_tooling.markets.api import get_authenticated_user' # Confirm the new import path is used consistently across the codebase. rg --type py 'from prediction_market_agent_tooling.markets.manifold.api import get_authenticated_user'Length of output: 343
Overall, the file
monitor_app.pyappears to be correctly updated in line with the PR's objectives, particularly concerning the reorganization of code and updating of import statements. It's important to ensure that the refactor does not introduce regressions, especially in the functionality related to market and agent monitoring. Comprehensive testing should be conducted to verify that the tooling behaves as expected post-refactor.
Just a refactor, no functional changes. Directory structure before:
after:
Each market type now has a class derived from
AgentMarketthat is used to take actions. Now if you want to add support for a new market, you only have to create a new directory undermarketsand implement this new derived class (and also add the market to theMARKET_TYPE_MAP).Unfortunately there isn't a clean separation in Omen between the API functions and the
OmenAgentMarketclass, so they both have to live insideomen.pyto avoid circular import issues. This can fix fixed later on if desired.Summary by CodeRabbit
Summary by CodeRabbit
AgentMarketclass for a unified market template across prediction markets.AgentMarketclass.AgentMarketsubclasses for Manifold and Omen markets.