383 move rebalancing logic involving previous positions to strategy #387
Conversation
…oDos for deployableTraderAgent
|
Warning Rate limit exceeded@gabrielfior has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 53 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve significant modifications to the Changes
Possibly related issues
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (1)
prediction_market_agent_tooling/deploy/agent.py (1)
Line range hint
423-455: Approved: Updatedprocess_marketmethod.The updates to the
process_marketmethod, which now uses theget_existing_position_for_marketfunction to calculate trades based on existing positions, align well with the PR's objectives. Consider enhancing the logging within this method to include more details about the trades being executed for better traceability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- prediction_market_agent_tooling/deploy/agent.py (7 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (1 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (2 hunks)
- prediction_market_agent_tooling/markets/data_models.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (1 hunks)
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Additional comments not posted (9)
prediction_market_agent_tooling/markets/data_models.py (1)
120-122: Well-implementedTradeTypeenum.The use of an enumeration for trade types (
SELLandBUY) enhances clarity and type safety in trade operations.prediction_market_agent_tooling/deploy/agent.py (1)
18-18: Approved: Changes toProcessedMarketclass.The removal of the
amountattribute from theProcessedMarketclass reflects the new focus on trade execution rather than bet placement, aligning with the PR's objectives to enhance efficiency in trade execution and rebalancing.prediction_market_agent_tooling/deploy/betting_strategy.py (5)
19-24: New Method:calculate_tradesThe introduction of the
calculate_tradesmethod is a significant change. It shifts the focus from simple bet calculation to a more complex trade construction based on existing positions. This aligns well with the PR's objectives of improving trade efficiency and rebalancing.
28-35: New Static Method:_assert_trades_currency_match_marketsThis method ensures that all trades are in the same currency as the market, which is crucial for maintaining consistency and avoiding errors during trade execution. It's a good practice to enforce such validations.
38-85: New Static Method:_build_rebalance_trades_from_positionsThis method constructs trades by comparing existing and target positions, which is central to the new rebalancing logic. The detailed documentation and example provided in the comments enhance understanding and maintainability. Sorting trades to prioritize buys before sells (line 83) is a strategic choice that could impact market dynamics favorably.
92-118: Method Implementation:calculate_tradesinMaxAccuracyBettingStrategyThe implementation of
calculate_tradesin this class uses a simple decision-making process based on the market's current probability and the probabilistic answer to determine trade directions. The use of the helper method_build_rebalance_trades_from_positionsensures that the trades are constructed correctly based on the calculated target positions.
132-154: Method Implementation:calculate_tradesinKellyBettingStrategyThe implementation of
calculate_tradesin this class correctly applies the Kelly criterion to determine the optimal size of trades based on the market's current probability, the probabilistic answer, and the user's confidence. This approach is appropriate for a strategy aimed at maximizing expected logarithmic utility, which is the goal of Kelly betting.prediction_market_agent_tooling/markets/agent_market.py (2)
9-9: Updated Import StatementThe addition of
OutcomeStrto the import statement supports the new methodget_outcome_str_from_bool, which is expected to handle outcome representations based on boolean values. This update is necessary for the new functionality introduced in this file.
196-197: New Method:get_outcome_str_from_boolThe introduction of
get_outcome_str_from_boolis a critical enhancement for handling outcomes based on boolean values. Marking it withNotImplementedErrorensures that subclasses provide specific implementations, which is a good practice for abstract methods in base classes.
| def get_existing_position_for_market( | ||
| market: AgentMarket, user_id: str | ||
| ) -> Position | None: | ||
| minimum_liquidatable_amount = market.get_tiny_bet_amount().amount / 10 |
There was a problem hiding this comment.
I think this tiny_bet / 10 logic was used somewhere already. What about refactoring it to a common property on AgentMarket ?
There was a problem hiding this comment.
Added a method liquidatable_amount to market
There was a problem hiding this comment.
Currently if we do
market0 = OmenAgentMarket(id=...)
agent.process_market(market=market0) # once
agent.process_market(market=market0) # twice
current behaviour (assuming the prediction made in each case is the same):
MaxAccuracyBettingStrategy- placebet_amountboth timesKellyBettingStrategy- place x <=max_betboth times
new behaviour:
MaxAccuracyBettingStrategy- place
bet_amountfirst time - place no bet the second time
- place
KellyBettingStrategy- place x <=
max_betfirst time - place no bet the second time
- place x <=
Is this what you intended? I would have thought for Kelly at least it would be:
- place x <=
max_betfirst time - place x <= y <=
max_betsecond time
for MaxAccuracyBettingStrategy I'm not sure.
…-rebalancing-logic-involving-previous-positions-to-strategy
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- prediction_market_agent_tooling/deploy/agent.py (6 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (1 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (4 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (3 hunks)
Additional context used
Learnings (2)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.prediction_market_agent_tooling/markets/omen/omen.py (3)
Learnt from: evangriffiths PR: gnosis/prediction-market-agent-tooling#300 File: prediction_market_agent_tooling/markets/omen/omen.py:344-352 Timestamp: 2024-07-08T07:05:58.507Z Learning: The `get_resolved_bets_made_since` method in the `OmenAgentMarket` class should include an `end_time` argument to make it more flexible.Learnt from: evangriffiths PR: gnosis/prediction-market-agent-tooling#300 File: prediction_market_agent_tooling/markets/omen/omen.py:344-352 Timestamp: 2024-07-08T07:08:57.156Z Learning: The `get_resolved_bets` method in the `OmenAgentMarket` class should include an `end_time` argument to make it more flexible.Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#139 File: tests/markets/omen/test_omen.py:144-154 Timestamp: 2024-03-28T15:05:28.580Z Learning: A GitHub issue was created to track the implementation of the `create_market_fund_market_remove_funding` test function in `test_omen.py` for the Gnosis prediction market agent tooling project. The issue number is 143.
Additional comments not posted (10)
prediction_market_agent_tooling/deploy/betting_strategy.py (5)
19-24: Approved: New methodcalculate_tradesinBettingStrategy.The new method
calculate_tradeseffectively replaces the old bet calculation logic with a more robust trading mechanism. Consider adding detailed documentation explaining the parameters and expected structure of the returnedTradeobjects.
33-41: Approved: Methodassert_trades_currency_match_markets.This method ensures currency consistency across trades, which is crucial for maintaining financial integrity. Consider handling cases where the
tradeslist might be empty to avoid unnecessary computations.
44-90: Approved: Method_build_rebalance_trades_from_positions.This method is well-implemented with clear documentation and examples. The sorting of trades to optimize pricing is a thoughtful addition. Consider evaluating the impact of this sorting on trade execution times and whether it aligns with real-world trading scenarios.
100-141: Approved: ClassMaxAccuracyBettingStrategy.The implementation of
calculate_tradesin this class is tailored to its strategy of maximizing accuracy, effectively utilizing existing positions. Consider adding unit tests to verify the behavior under various market conditions and positions.
160-194: Approved: ClassKellyBettingStrategy.The
calculate_tradesmethod integrates the Kelly criterion effectively, considering both existing positions and a maximum bet amount. Evaluate the implications of this max bet amount on trade sizes, especially in scenarios with highly volatile markets.prediction_market_agent_tooling/markets/omen/omen.py (2)
166-178: Approved: Modification toliquidate_existing_positions.The addition of the
larger_thanparameter and its default handling usingget_liquidatable_amountenhances the flexibility of the liquidation process. This change aligns well with the PR objectives of improving efficiency.
444-456: Approved: Addition ofget_existing_position_for_market.The method effectively uses the
get_liquidatable_amountto filter positions, enhancing the management of market positions. This addition aligns with the PR objectives of improving trading efficiency.prediction_market_agent_tooling/markets/agent_market.py (2)
9-9: Approved: Updated import statement.The addition of
OutcomeStrto the import statement aligns with the new methods introduced in the class, which utilize this type.
202-204: Approved: Interface methodget_outcome_str_from_bool.The method correctly sets up an interface for subclasses to implement, ensuring that specific behaviors are customized according to the needs of different market types.
prediction_market_agent_tooling/deploy/agent.py (1)
112-112: Approved: UpdatedProcessedMarketclass.The addition of the
tradesattribute aligns with the new trading logic, enhancing the clarity and maintainability of the code by separating the concerns of trade calculation and execution.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock,!**/*.lock
Files selected for processing (2)
- prediction_market_agent_tooling/markets/agent_market.py (4 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/markets/omen/omen.py
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Additional comments not posted (2)
prediction_market_agent_tooling/markets/agent_market.py (2)
10-10: Approved: Updated import statement.The import statement has been modified to include
OutcomeStralongsideProbability. This change supports the new methods introduced in this PR that deal with outcome representations, aligning with the PR's objectives to enhance outcome handling.
222-223: Approved: Methodget_outcome_str_from_boolcorrectly raisesNotImplementedError.This method serves as a template for subclasses to implement their specific logic for converting a boolean to an
OutcomeStr. It would be beneficial to include documentation or comments in the code to guide developers on how to implement this method in subclasses.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- prediction_market_agent_tooling/markets/agent_market.py (4 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/markets/agent_market.py
- prediction_market_agent_tooling/markets/omen/omen.py
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
| @@ -230,6 +239,9 @@ def get_outcome_index(self, outcome: str) -> int: | |||
| def get_token_balance(self, user_id: str, outcome: str) -> TokenAmount: | |||
| raise NotImplementedError("Subclasses must implement this method") | |||
|
|
|||
| def get_existing_position_for_market(self, api_keys: APIKeys) -> Position | None: | |||
There was a problem hiding this comment.
Looks like this only needs user_id: str, like get_positions. And could rename to just get_position.
| @@ -432,6 +445,19 @@ def get_token_balance( | |||
| currency=self.currency, | |||
| ) | |||
|
|
|||
| def get_existing_position_for_market(self, api_keys: APIKeys) -> Position | None: | |||
There was a problem hiding this comment.
Nothing in this looks omen-specific, so it could be moved to the base class
There was a problem hiding this comment.
this is not present in the updated version
| amount=adjusted_bet_amount, | ||
| currency=market.currency, | ||
| ), | ||
| market.get_outcome_str_from_bool(not direction): TokenAmount( |
There was a problem hiding this comment.
Do you need to specify these 0 TokenAmounts when instantiating the target position (here, and in KellyBettingStrategy.calculate_trades)
There was a problem hiding this comment.
In the current setup yes, otherwise Pydantic complains - we could set a default value in TokenAmount of 0, but I would advise against that to make sure people assign a value to amount.
There was a problem hiding this comment.
sorry, i mean why not just:
amounts = {
market.get_outcome_str_from_bool(direction): TokenAmount(
amount=adjusted_bet_amount,
currency=market.currency,
),
}
There was a problem hiding this comment.
Good point, we can simply not create an amount for the other direction. This will get caught on _build_rebalance_trades_from_positions.
Co-authored-by: Evan Griffiths <[email protected]>
Co-authored-by: Evan Griffiths <[email protected]>
Co-authored-by: Evan Griffiths <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- prediction_market_agent_tooling/deploy/betting_strategy.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/markets/omen/omen.py
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Additional comments not posted (6)
prediction_market_agent_tooling/deploy/betting_strategy.py (6)
5-5: LGTM!The new imports for
Position,Trade, andTradeTypedata models are consistent with the changes made in the rest of the code.Also applies to: 7-9
19-24: LGTM!The changes to the
calculate_tradesmethod signature in theBettingStrategyclass are consistent with the overall changes made in the file. Making the method abstract is appropriate as it is meant to be implemented by the derived classes.
27-31: LGTM!The changes to the
adjust_bet_amountmethod signature in theBettingStrategyclass are consistent with the overall changes made in the file. Making the method abstract is appropriate as it is meant to be implemented by the derived classes.
33-41: LGTM!The new
assert_trades_currency_match_marketsmethod in theBettingStrategyclass is a useful addition to ensure that the trades are in the same currency as the market. It enhances the robustness of the trading logic.
44-90: LGTM!The new
_build_rebalance_trades_from_positionsmethod in theBettingStrategyclass is a well-designed helper method that encapsulates the logic for building rebalance trades based on existing and target positions. It handles various edge cases and ensures the correctness of the trades.
112-135: LGTM!The changes to the
calculate_tradesmethod in theMaxAccuracyBettingStrategyandKellyBettingStrategyclasses are consistent with the changes made in the base class and the overall changes made in the file. The method now takes into account the existing position and builds rebalance trades based on it, which is a significant improvement over the previous implementation.Also applies to: 163-189
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
prediction_market_agent_tooling/markets/agent_market.py (2)
156-160: LGTM: Approved newget_liquidatable_amountmethod.The new class method is a useful addition for financial calculations. It correctly calculates a reduced bet amount based on a tiny bet amount.
Consider refactoring this logic into a common property or method to avoid duplication across different parts of the codebase.
242-243: LGTM: Approved newget_positionmethod.The new method is a good addition to the class, as it provides a way to get a user's position in a market. Marking the function with a
NotImplementedErroris a good design choice, as it ensures that subclasses implement this method.Consider renaming the function to
get_positionand removing theoutcomeparameter, as it is not used.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- prediction_market_agent_tooling/deploy/agent.py (6 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (1 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (4 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (3 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/omen/omen.py
Additional context used
Learnings (1)
Common learnings
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#224 File: prediction_market_agent_tooling/markets/markets.py:67-67 Timestamp: 2024-04-29T14:14:38.751Z Learning: The function `have_bet_on_market_since` is suggested to be refactored into a method on the `AgentMarket` class, potentially reusing code from `DeployedAgent.get_resolved_bets`. This task is planned for future work, not within the scope of the current PR.
Additional comments not posted (7)
prediction_market_agent_tooling/deploy/betting_strategy.py (6)
19-24: LGTM: Approved changes tocalculate_tradesmethod.The changes to the method signature and return type reflect a shift in focus from simply determining the amount and direction of a bet to constructing a series of trades based on existing and target positions. Making the method abstract is a good design choice, as it ensures that subclasses implement this method.
27-31: LGTM: Approved changes toadjust_bet_amountmethod.The changes to the method signature to include
existing_positionparameter indicate that the function now considers the current holdings of the user when adjusting the bet amount. Making the method abstract is a good design choice, as it ensures that subclasses implement this method.
33-41: LGTM: Approved newassert_trades_currency_match_marketsmethod.The new static method enhances the robustness of the trading logic by validating that all trades are in the same currency as the market, ensuring consistency in currency across trades.
44-90: LGTM: Approved new_build_rebalance_trades_from_positionsmethod.The new static method is a well-designed helper function that constructs trades by comparing existing and target positions, ensuring that trades are created to rebalance token allocations. The error handling for currency mismatches enhances the robustness of the trading logic.
110-130: LGTM: Approved changes tocalculate_tradesmethod.The changes to the method signature and return type reflect a shift in focus from simply determining the amount and direction of a bet to constructing a series of trades based on existing and target positions. Using the
_build_rebalance_trades_from_positionsmethod to construct trades is a good design choice, as it promotes code reuse and maintainability.
158-181: LGTM: Approved changes tocalculate_tradesmethod.The changes to the method signature and return type reflect a shift in focus from simply determining the amount and direction of a bet to constructing a series of trades based on existing and target positions. Using the
_build_rebalance_trades_from_positionsmethod to construct trades is a good design choice, as it promotes code reuse and maintainability.prediction_market_agent_tooling/markets/agent_market.py (1)
222-223: LGTM: Approved newget_outcome_str_from_boolmethod.The new method is a good addition to the class, as it provides a way to convert a boolean outcome to an
OutcomeStr. Marking the function with aNotImplementedErroris a good design choice, as it ensures that subclasses implement this method.
gabrielfior
deleted the
383-move-rebalancing-logic-involving-previous-positions-to-strategy
branch
-> Exposing method
calculate_tradesfromBettingStrategy, so that agent gets a list of trades and executes these.-> Implemented rebalance with lower gas fees (don't necessarily sell, buy only diff amount, etc)