Refactor omen-specific logic out of DeployableTraderAgent + fix minimum required balance to operate #493
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
kongzii
force-pushed
the
peter/required-trade-balance
branch
from
6b0acdc to
0422b35
Compare
| @@ -294,7 +294,6 @@ def get_gcloud_fname(self, market_type: MarketType) -> str: | |||
|
|
|||
| class DeployableTraderAgent(DeployableAgent): | |||
| bet_on_n_markets_per_run: int = 1 | |||
| min_required_balance_to_operate: xDai | None = xdai_type(1) | |||
There was a problem hiding this comment.
This was hard-coded to 1, but now agents can place high bets thanks to different strategies.
Instead of hard-coding, this PR get the strategy and requires that minimal required balance is maximal possible bet from the strategy.
| check_for_gas: bool = True, | ||
| check_for_trades: bool = True, | ||
| ) -> None: | ||
| def check_min_required_balance_to_operate(self, market_type: MarketType) -> None: |
There was a problem hiding this comment.
I split the original function into 2, because the second one (because of the betting strategy) needs to accept AgentMarket as well, but we don't have that in before_process_markets
| # Have a little bandwich after the bet. | ||
| min_required_balance_to_trade = strategy.maximum_possible_bet_amount * 1.01 | ||
|
|
||
| if market_type == MarketType.OMEN and not is_minimum_required_balance( |
There was a problem hiding this comment.
I see that is_minimum_required_balance is an omen-specific function, hence the market_type check. Could you either:
- add a comment/TODO to say how this is partial coverage, and other market types will just fail depending on how the polymarket/manifold/... APIs handle it.
- or maybe a nicer thing to do would be add
passfunction on AgentMarket, and then override it for OmenAgentMarket
?
There was a problem hiding this comment.
Good point, DeployableTraderAgent was becoming a "bit" Omen-specific.
Wdyt now? (but it bloated the PR, sorry 😄 )
There was a problem hiding this comment.
Yes, but it's great!
|
|
||
| # Get the strategy to know how much it will bet. | ||
| strategy = self.get_betting_strategy(market) | ||
| # Have a little bandwich after the bet. |
There was a problem hiding this comment.
sandwich 🥪 , or bandwidth? 🤔
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
prediction_market_agent_tooling/deploy/agent.py (2)
375-375: Define the multiplier as a constantThe multiplier
1.01on line 375 is a magic number. Consider defining it as a constant or configuration parameter for clarity and maintainability.
446-446: Delay initialization ofapi_keysThe
api_keysvariable is initialized on line 446 but is only used inside theif market_type == MarketType.OMENblock starting on line 448. Consider moving the initialization inside the block to avoid unnecessary instantiation when the condition is not met.Apply this diff:
- api_keys = APIKeys() if market_type == MarketType.OMEN: + api_keys = APIKeys() self.check_min_required_balance_to_trade(market_type, market) # Exchange wxdai back to xdai if the balance is getting low, so we can keep paying for fees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prediction_market_agent_tooling/deploy/agent.py (4 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (4 hunks)
🧰 Additional context used
🔇 Additional comments (5)
prediction_market_agent_tooling/deploy/betting_strategy.py (5)
35-36: Proper use ofNotImplementedErrorin abstract methodRaising
NotImplementedErrorin thecalculate_tradesmethod enforces that subclasses must implement this method, adhering to best practices for abstract base classes.
37-40: Correct definition of abstract propertymaximum_possible_bet_amountDefining
maximum_possible_bet_amountas an abstract property ensures that all betting strategies explicitly specify their maximum possible bet amounts, promoting consistency across subclasses.
134-137: Implementation ofmaximum_possible_bet_amountinMaxAccuracyBettingStrategyThe property correctly returns
self.bet_amount, aligning with the intended behavior of this strategy.
180-183: Implementation ofmaximum_possible_bet_amountinKellyBettingStrategyThe property appropriately returns
self.max_bet_amount, ensuring consistency with the initialized maximum bet amount.
298-301:⚠️ Potential issueVerify consistency of
maximum_possible_bet_amountwith actual bet amountIn
MaxAccuracyWithKellyScaledBetsStrategy, theadjust_bet_amountmethod addsexisting_position_total_amounttoself.max_bet_amount, potentially resulting in a total bet amount exceedingself.max_bet_amount. However, themaximum_possible_bet_amountproperty returns onlyself.max_bet_amount. Please verify that this property accurately reflects the maximum possible bet amount for this strategy.
| @@ -602,7 +597,7 @@ def process_markets(self, market_type: MarketType) -> None: | |||
| logger.info("All markets processed.") | |||
|
|
|||
| def after_process_markets(self, market_type: MarketType) -> None: | |||
| pass | |||
| "Executes actions that occur after bets are placed." | |||
There was a problem hiding this comment.
Properly format the docstring
The string on line 600 is intended to be a docstring but is not properly formatted. In Python, method docstrings should be enclosed in triple quotes.
Apply this diff to fix the docstring:
def after_process_markets(self, market_type: MarketType) -> None:
- "Executes actions that occur after bets are placed."
+ """Executes actions that occur after bets are placed."""📝 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.
| "Executes actions that occur after bets are placed." | |
| def after_process_markets(self, market_type: MarketType) -> None: | |
| """Executes actions that occur after bets are placed.""" |
|
|
||
| # Get the strategy to know how much it will bet. | ||
| strategy = self.get_betting_strategy(market) | ||
| # Have a little bandwich after the bet. |
There was a problem hiding this comment.
Correct the typo in the comment
The comment on line 374 contains a typo. "bandwich" should be corrected to "bandwidth".
Apply this diff to fix the typo:
- # Have a little bandwich after the bet.
+ # Have a little bandwidth after the bet.Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
prediction_market_agent_tooling/markets/markets.py (1)
49-51: LGTM! Consider adding a docstring for clarity.The implementation of
is_blockchain_marketproperty is correct and aligns well with the PR objective. It provides a convenient way to identify blockchain markets, which can be useful for balance checks or other blockchain-specific operations.Consider adding a docstring to improve code clarity:
@property def is_blockchain_market(self) -> bool: """ Check if the current market type is a blockchain market. Returns: bool: True if the market is OMEN or POLYMARKET, False otherwise. """ return self in [MarketType.OMEN, MarketType.POLYMARKET]prediction_market_agent_tooling/deploy/agent.py (2)
367-384: Well-structured balance check for tradingThe new
check_min_required_balance_to_trademethod is a valuable addition. It ensures that the agent has sufficient funds to place a trade based on the betting strategy, with a 1% buffer for safety.However, there's a small typo in the comment:
- # Have a little bandwich after the bet. + # Have a little bandwidth after the bet.
602-602: Improve docstring formattingThe addition of a docstring to the
after_process_marketsmethod is good practice. However, the formatting should be adjusted to use triple quotes for multi-line docstrings.Apply this diff to fix the docstring formatting:
- "Executes actions that occur after bets are placed." + """Executes actions that occur after bets are placed."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prediction_market_agent_tooling/deploy/agent.py (4 hunks)
- prediction_market_agent_tooling/markets/markets.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
prediction_market_agent_tooling/markets/markets.py (1)
49-51: Consider utilizingis_blockchain_marketin other parts of the codebase.The new
is_blockchain_marketproperty provides a clean way to identify blockchain markets. Consider using this property in other parts of the codebase where you need to distinguish between blockchain and non-blockchain markets. This could simplify existing conditionals and improve code readability.To identify potential areas where this property could be useful, you can run the following script:
This script will help identify areas in the codebase (excluding the current file) where the new property might be beneficial.
✅ Verification successful
Verification Complete: No conditional statements using
OMENorPOLYMARKETwere found outsidemarkets.py.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for occurrences of OMEN or POLYMARKET in conditional statements # Test: Search for conditionals involving OMEN or POLYMARKET rg --type python -e "if.*(?:OMEN|POLYMARKET)" -e "(?:OMEN|POLYMARKET).*:" -g '!**/markets.py'Length of output: 126
Script:
#!/bin/bash # Description: Search for occurrences of OMEN or POLYMARKET in conditional statements within .py files # Search for conditionals involving OMEN or POLYMARKET, excluding markets.py rg -g '*.py' -e "if\s+.*(?:OMEN|POLYMARKET)" -e "(?:OMEN|POLYMARKET).*:" --ignore-file 'markets.py'Length of output: 3543
Script:
#!/bin/bash # Description: Search for occurrences of OMEN or POLYMARKET in conditional statements within .py files, excluding prediction_market_agent_tooling/markets/markets.py # Search for conditionals involving OMEN or POLYMARKET, excluding the specific markets.py file rg -g '*.py' -e "if\s+.*(?:OMEN|POLYMARKET)" -e "(?:OMEN|POLYMARKET).*:" --ignore 'prediction_market_agent_tooling/markets/markets.py'Length of output: 222
prediction_market_agent_tooling/deploy/agent.py (4)
354-366: Improved balance check for blockchain marketsThe
check_min_required_balance_to_operatemethod has been simplified and now focuses solely on checking the minimum required balance for blockchain markets. This change improves readability and ensures that the agent has enough funds to cover gas fees before attempting any operations.
446-457: Enhanced pre-processing for blockchain marketsThe
before_process_marketmethod has been improved with additional checks and actions for blockchain markets:
- It now checks the minimum required balance to trade, preventing potential failures due to insufficient funds.
- It exchanges wxdai back to xdai when the balance is low, ensuring the agent can continue to pay for fees.
These changes significantly enhance the robustness of the trading process on blockchain markets.
Line range hint
458-516: Improved market processing workflowThe
process_marketmethod has been enhanced with several improvements:
- It now calls
self.before_process_market, ensuring all necessary pre-processing steps are performed.- The use of
self.place_betflag allows for dry runs or simulations without actually placing bets.- The creation of a
ProcessedMarketobject standardizes the output format, improving consistency and ease of use.These changes make the market processing workflow more robust, flexible, and maintainable.
571-574: Added balance check before processing marketsThe
before_process_marketsmethod now includes a balance check for blockchain markets by callingself.check_min_required_balance_to_operate. This is a valuable addition as it ensures the agent has sufficient funds to operate before attempting to process any markets, potentially preventing errors later in the process.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
prediction_market_agent_tooling/markets/manifold/manifold.py (1)
128-131: Approve the addition with suggestions for improvementThe new
redeem_winningsmethod is a good addition to theManifoldAgentMarketclass, providing clarity on how winnings are handled. However, I have a few suggestions to enhance its implementation:
Consider renaming the method to better reflect its purpose, given that it doesn't actually perform any action. A name like
check_winnings_redemptionorverify_automatic_redemptionmight be more appropriate.Add a docstring to provide more context about the automatic redemption process on Manifold. This will help other developers understand why the method exists and how it functions.
Here's a suggested implementation incorporating these improvements:
@staticmethod def verify_automatic_redemption(api_keys: APIKeys) -> None: """ Verify that winnings are automatically redeemed on Manifold. This method exists to document that Manifold handles the redemption process automatically, and no manual action is required from the user or this API. Args: api_keys (APIKeys): The API keys for authentication (unused in this method). """ # Winnings are redeemed automatically on Manifold, no action required. passtests_integration_with_local_chain/markets/omen/test_local_chain.py (3)
89-89: Approve change, but consider renaming the test functionThe assertion has been correctly updated to use
get_total_balanceinstead ofis_minimum_required_balance, which is consistent with the import changes. However, the test function nametest_anvil_account_has_more_than_minimum_required_balanceno longer accurately describes the test's behavior.Consider renaming the test function to better reflect its new behavior, for example:
def test_anvil_account_has_more_than_half_xdai_balance(local_web3: Web3, accounts: list[TestAccount]) -> None:
92-95: Approve changes, suggest improvements in naming and documentationThe function has been updated correctly to use
get_total_balanceand no longer requires theaccountsparameter. These changes are consistent with the previous modifications. However, there are a few suggestions for improvement:
- The test function name no longer accurately describes the test's behavior. Consider renaming it to better reflect its new purpose.
- Adding a brief comment explaining why a fresh account is created would improve code readability.
Consider the following improvements:
- Rename the function:
def test_fresh_account_has_less_than_half_xdai_balance(local_web3: Web3) -> None:
- Add a comment explaining the use of a fresh account:
# Create a fresh account to ensure it starts with zero balance fresh_account_adr = Account.create().address
Remaining uses of
minimum_required_balancefound in function names. Please consider renaming these functions for consistency:
test_anvil_account_has_more_than_minimum_required_balancetest_fresh_account_has_less_than_minimum_required_balance🔗 Analysis chain
Line range hint
1-134: Summary: Approve changes, verify impact on other filesThe changes in this file are part of a larger refactoring to use
get_total_balanceinstead ofis_minimum_required_balance. The modifications are consistent and improve the balance checking logic. Minor suggestions for improvement in naming and documentation have been provided in previous comments.To ensure the changes are consistent across the codebase, please run the following verification script:
This script will help identify any other files that might need similar updates or any inconsistencies in the refactoring process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining uses of 'is_minimum_required_balance' and 'minimum_required_balance' echo "Checking for remaining uses of 'is_minimum_required_balance':" rg "is_minimum_required_balance" --type python echo "\nChecking for remaining uses of 'minimum_required_balance':" rg "minimum_required_balance" --type python echo "\nChecking for new uses of 'get_total_balance':" rg "get_total_balance" --type pythonLength of output: 607
Script:
#!/bin/bash # Description: Check for any remaining uses of 'is_minimum_required_balance' and 'minimum_required_balance' echo "Checking for remaining uses of 'is_minimum_required_balance':" rg "is_minimum_required_balance" --type py echo "\nChecking for remaining uses of 'minimum_required_balance':" rg "minimum_required_balance" --type py echo "\nChecking for new uses of 'get_total_balance':" rg "get_total_balance" --type pyLength of output: 1466
prediction_market_agent_tooling/markets/agent_market.py (3)
30-32: Consider adding a docstring to theProcessedMarketclassAdding a docstring to the
ProcessedMarketclass will enhance code readability and maintainability by providing a clear description of its purpose and usage.
208-214: Align implementation with docstring inredeem_winningsmethodThe docstring suggests that if this method is not needed, it should be implemented with
pass. Currently, it raises aNotImplementedError. Consider updating the method topassor adjusting the docstring to reflect that subclasses must implement this method by raisingNotImplementedError.
222-228: Ensure consistency between docstring and implementation inverify_operational_balanceThe docstring indicates that if operational balance verification is not needed, the method should be implemented with
pass. However, the current implementation raises aNotImplementedError. Please align the method body with the docstring or update the docstring to indicate that subclasses should override this method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- prediction_market_agent_tooling/deploy/agent.py (6 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (3 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (9 hunks)
- tests_integration_with_local_chain/markets/omen/test_local_chain.py (2 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent_tooling/deploy/agent.py
421-423: Use a single
ifstatement instead of nestedifstatements(SIM102)
🔇 Additional comments (3)
tests_integration_with_local_chain/markets/omen/test_local_chain.py (1)
11-11: LGTM: Import statement updated correctlyThe import statement has been updated to reflect the change from
is_minimum_required_balancetoget_total_balance. This change aligns with the modifications in the test functions and improves consistency in balance checking.prediction_market_agent_tooling/deploy/agent.py (1)
494-494:⚠️ Potential issueDefine
keysbefore usageOn line 494, the variable
keysis used but not defined within the scope of theafter_process_marketmethod. This will result in aNameErrorat runtime.Initialize
keysbefore using it:def after_process_market( self, market_type: MarketType, market: AgentMarket, processed_market: ProcessedMarket, ) -> None: + keys = APIKeys() market.store_prediction(processed_market=processed_market, keys=keys)Likely invalid or redundant comment.
prediction_market_agent_tooling/markets/omen/omen.py (1)
Line range hint
1269-1285:get_total_balancefunction correctly sums balancesThe implementation of
get_total_balancecorrectly calculates the total balance by summing xDai and wxDai balances based on the provided flags.
| self.check_min_required_balance_to_operate(market_type) | ||
| market_type.market_class.redeem_winnings(api_keys) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent usage of api_keys
In the before_process_markets method, api_keys is defined but not passed to check_min_required_balance_to_operate, which also uses APIKeys(). For consistency and potential future flexibility, consider passing api_keys as a parameter.
Update the method signature and calls to include api_keys:
- def check_min_required_balance_to_operate(self, market_type: MarketType) -> None:
+ def check_min_required_balance_to_operate(self, market_type: MarketType, api_keys: APIKeys) -> None:
def before_process_markets(self, market_type: MarketType) -> None:
"""
Executes actions that occur before bets are placed.
"""
api_keys = APIKeys()
- self.check_min_required_balance_to_operate(market_type)
+ self.check_min_required_balance_to_operate(market_type, api_keys)
market_type.market_class.redeem_winnings(api_keys)Committable suggestion was skipped due to low confidence.
| if market_type.is_blockchain_market: | ||
| # Exchange wxdai back to xdai if the balance is getting low, so we can keep paying for fees. | ||
| if self.min_balance_to_keep_in_native_currency is not None: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested if statements for clarity
The nested if statements on lines 421-423 can be combined into a single condition to improve readability and follow best practices.
Apply this diff to simplify the condition:
- if market_type.is_blockchain_market:
- # Exchange wxdai back to xdai if the balance is getting low, so we can keep paying for fees.
- if self.min_balance_to_keep_in_native_currency is not None:
+ if market_type.is_blockchain_market and self.min_balance_to_keep_in_native_currency is not None:
withdraw_wxdai_to_xdai_to_keep_balance(
api_keys,
min_required_balance=self.min_balance_to_keep_in_native_currency,
withdraw_multiplier=2,
)📝 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.is_blockchain_market: | |
| # Exchange wxdai back to xdai if the balance is getting low, so we can keep paying for fees. | |
| if self.min_balance_to_keep_in_native_currency is not None: | |
| if market_type.is_blockchain_market and self.min_balance_to_keep_in_native_currency is not None: | |
| withdraw_wxdai_to_xdai_to_keep_balance( | |
| api_keys, | |
| min_required_balance=self.min_balance_to_keep_in_native_currency, | |
| withdraw_multiplier=2, | |
| ) |
🧰 Tools
🪛 Ruff
421-423: Use a single
ifstatement instead of nestedifstatements(SIM102)
| def check_min_required_balance_to_trade(self, market: AgentMarket) -> None: | ||
| api_keys = APIKeys() | ||
|
|
||
| # Get the strategy to know how much it will bet. | ||
| strategy = self.get_betting_strategy(market) | ||
| # Have a little bandwich after the bet. | ||
| min_required_balance_to_trade = strategy.maximum_possible_bet_amount * 1.01 | ||
|
|
||
| if market.get_trade_balance(api_keys) < min_required_balance_to_trade: | ||
| raise OutOfFundsError( | ||
| f"Minimum required balance {self.min_required_balance_to_operate} " | ||
| f"for agent with address {api_keys.bet_from_address=} is not met." | ||
| f"Minimum required balance {min_required_balance_to_trade} for agent is not met." |
There was a problem hiding this comment.
Ensure sufficient balance before placing trades
In the check_min_required_balance_to_trade method, you calculate min_required_balance_to_trade based on the strategy's maximum_possible_bet_amount. However, there is no check to ensure that this amount is greater than zero, which could lead to unexpected behavior or errors.
Consider adding a validation to ensure maximum_possible_bet_amount is positive:
strategy = self.get_betting_strategy(market)
+ if strategy.maximum_possible_bet_amount <= 0:
+ raise ValueError("Maximum possible bet amount must be greater than zero.")
# Have a little bandwich after the bet.
min_required_balance_to_trade = strategy.maximum_possible_bet_amount * 1.01📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def check_min_required_balance_to_trade(self, market: AgentMarket) -> None: | |
| api_keys = APIKeys() | |
| # Get the strategy to know how much it will bet. | |
| strategy = self.get_betting_strategy(market) | |
| # Have a little bandwich after the bet. | |
| min_required_balance_to_trade = strategy.maximum_possible_bet_amount * 1.01 | |
| if market.get_trade_balance(api_keys) < min_required_balance_to_trade: | |
| raise OutOfFundsError( | |
| f"Minimum required balance {self.min_required_balance_to_operate} " | |
| f"for agent with address {api_keys.bet_from_address=} is not met." | |
| f"Minimum required balance {min_required_balance_to_trade} for agent is not met." | |
| def check_min_required_balance_to_trade(self, market: AgentMarket) -> None: | |
| api_keys = APIKeys() | |
| # Get the strategy to know how much it will bet. | |
| strategy = self.get_betting_strategy(market) | |
| if strategy.maximum_possible_bet_amount <= 0: | |
| raise ValueError("Maximum possible bet amount must be greater than zero.") | |
| # Have a little bandwich after the bet. | |
| min_required_balance_to_trade = strategy.maximum_possible_bet_amount * 1.01 | |
| if market.get_trade_balance(api_keys) < min_required_balance_to_trade: | |
| raise OutOfFundsError( | |
| f"Minimum required balance {min_required_balance_to_trade} for agent is not met." |
| @staticmethod | ||
| def redeem_winnings(api_keys: APIKeys) -> None: | ||
| redeem_from_all_user_positions(api_keys) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add docstring to redeem_winnings method for clarity
The redeem_winnings static method lacks a docstring explaining its purpose and usage. Adding a docstring will improve code readability and maintainability.
Suggested change:
@staticmethod
+ def redeem_winnings(api_keys: APIKeys) -> None:
+ """
+ Redeems winnings from all user positions.
+ """
redeem_from_all_user_positions(api_keys)📝 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.
| @staticmethod | |
| def redeem_winnings(api_keys: APIKeys) -> None: | |
| redeem_from_all_user_positions(api_keys) | |
| @staticmethod | |
| def redeem_winnings(api_keys: APIKeys) -> None: | |
| """ | |
| Redeems winnings from all user positions. | |
| """ | |
| redeem_from_all_user_positions(api_keys) |
| @staticmethod | ||
| def get_trade_balance(api_keys: APIKeys, web3: Web3 | None = None) -> xDai: | ||
| return get_total_balance( | ||
| address=api_keys.bet_from_address, web3=web3, sum_xdai=True, sum_wxdai=True | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add docstring to get_trade_balance method for clarity
Including a docstring for the get_trade_balance method will help explain its functionality to other developers.
Suggested change:
@staticmethod
+ def get_trade_balance(api_keys: APIKeys, web3: Web3 | None = None) -> xDai:
+ """
+ Retrieves the total trade balance by summing xDai and wxDai balances.
+ """
return get_total_balance(
address=api_keys.bet_from_address, web3=web3, sum_xdai=True, sum_wxdai=True
)📝 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.
| @staticmethod | |
| def get_trade_balance(api_keys: APIKeys, web3: Web3 | None = None) -> xDai: | |
| return get_total_balance( | |
| address=api_keys.bet_from_address, web3=web3, sum_xdai=True, sum_wxdai=True | |
| ) | |
| @staticmethod | |
| def get_trade_balance(api_keys: APIKeys, web3: Web3 | None = None) -> xDai: | |
| """ | |
| Retrieves the total trade balance by summing xDai and wxDai balances. | |
| """ | |
| return get_total_balance( | |
| address=api_keys.bet_from_address, web3=web3, sum_xdai=True, sum_wxdai=True | |
| ) |
| @staticmethod | ||
| def verify_operational_balance(api_keys: APIKeys) -> bool: | ||
| return get_total_balance( | ||
| api_keys.public_key, # Use `public_key`, not `bet_from_address` because transaction costs are paid from the EOA wallet. | ||
| sum_wxdai=False, | ||
| ) > xdai_type(0.001) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add docstring to verify_operational_balance method for clarity
Adding a docstring to the verify_operational_balance method will enhance code documentation and maintainability.
Suggested change:
@staticmethod
+ def verify_operational_balance(api_keys: APIKeys) -> bool:
+ """
+ Verifies if the operational balance (xDai) is above the minimum required threshold.
+ """
return get_total_balance(
api_keys.public_key, # Use `public_key`, not `bet_from_address` because transaction costs are paid from the EOA wallet.
sum_wxdai=False,
) > xdai_type(0.001)📝 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.
| @staticmethod | |
| def verify_operational_balance(api_keys: APIKeys) -> bool: | |
| return get_total_balance( | |
| api_keys.public_key, # Use `public_key`, not `bet_from_address` because transaction costs are paid from the EOA wallet. | |
| sum_wxdai=False, | |
| ) > xdai_type(0.001) | |
| @staticmethod | |
| def verify_operational_balance(api_keys: APIKeys) -> bool: | |
| """ | |
| Verifies if the operational balance (xDai) is above the minimum required threshold. | |
| """ | |
| return get_total_balance( | |
| api_keys.public_key, # Use `public_key`, not `bet_from_address` because transaction costs are paid from the EOA wallet. | |
| sum_wxdai=False, | |
| ) > xdai_type(0.001) |
| def store_prediction( | ||
| self, processed_market: ProcessedMarket, keys: APIKeys | ||
| ) -> None: | ||
| reasoning = ( | ||
| processed_market.answer.reasoning | ||
| if processed_market.answer.reasoning | ||
| else "" | ||
| ) | ||
|
|
||
| ipfs_hash_decoded = HexBytes(HASH_ZERO) | ||
| if keys.enable_ipfs_upload: | ||
| logger.info("Storing prediction on IPFS.") | ||
| ipfs_hash = IPFSHandler(keys).store_agent_result( | ||
| IPFSAgentResult(reasoning=reasoning) | ||
| ) | ||
| ipfs_hash_decoded = ipfscidv0_to_byte32(ipfs_hash) | ||
|
|
||
| tx_hashes = [ | ||
| HexBytes(HexStr(i.id)) for i in processed_market.trades if i.id is not None | ||
| ] | ||
| prediction = ContractPrediction( | ||
| publisher=keys.public_key, | ||
| ipfs_hash=ipfs_hash_decoded, | ||
| tx_hashes=tx_hashes, | ||
| estimated_probability_bps=int(processed_market.answer.p_yes * 10000), | ||
| ) | ||
| tx_receipt = OmenAgentResultMappingContract().add_prediction( | ||
| api_keys=keys, | ||
| market_address=Web3.to_checksum_address(self.id), | ||
| prediction=prediction, | ||
| ) | ||
| logger.info( | ||
| f"Added prediction to market {self.id}. - receipt {tx_receipt['transactionHash'].hex()}." | ||
| ) | ||
|
|
There was a problem hiding this comment.
Ensure proper error handling when interacting with external services
The store_prediction method interacts with external services like IPFS and smart contracts. Adding error handling will improve robustness by gracefully handling any exceptions during these operations.
Consider wrapping external calls with try-except blocks and logging exceptions.
ipfs_hash_decoded = HexBytes(HASH_ZERO)
if keys.enable_ipfs_upload:
logger.info("Storing prediction on IPFS.")
+ try:
ipfs_hash = IPFSHandler(keys).store_agent_result(
IPFSAgentResult(reasoning=reasoning)
)
ipfs_hash_decoded = ipfscidv0_to_byte32(ipfs_hash)
+ except Exception as e:
+ logger.error(f"Failed to store prediction on IPFS: {e}")
+ # Handle the exception appropriately
tx_hashes = [
HexBytes(HexStr(i.id)) for i in processed_market.trades if i.id is not None
]
prediction = ContractPrediction(
publisher=keys.public_key,
ipfs_hash=ipfs_hash_decoded,
tx_hashes=tx_hashes,
estimated_probability_bps=int(processed_market.answer.p_yes * 10000),
)
+ try:
tx_receipt = OmenAgentResultMappingContract().add_prediction(
api_keys=keys,
market_address=Web3.to_checksum_address(self.id),
prediction=prediction,
)
logger.info(
f"Added prediction to market {self.id}. - receipt {tx_receipt['transactionHash'].hex()}."
)
+ except Exception as e:
+ logger.error(f"Failed to add prediction to market: {e}")
+ # Handle the exception appropriately📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def store_prediction( | |
| self, processed_market: ProcessedMarket, keys: APIKeys | |
| ) -> None: | |
| reasoning = ( | |
| processed_market.answer.reasoning | |
| if processed_market.answer.reasoning | |
| else "" | |
| ) | |
| ipfs_hash_decoded = HexBytes(HASH_ZERO) | |
| if keys.enable_ipfs_upload: | |
| logger.info("Storing prediction on IPFS.") | |
| ipfs_hash = IPFSHandler(keys).store_agent_result( | |
| IPFSAgentResult(reasoning=reasoning) | |
| ) | |
| ipfs_hash_decoded = ipfscidv0_to_byte32(ipfs_hash) | |
| tx_hashes = [ | |
| HexBytes(HexStr(i.id)) for i in processed_market.trades if i.id is not None | |
| ] | |
| prediction = ContractPrediction( | |
| publisher=keys.public_key, | |
| ipfs_hash=ipfs_hash_decoded, | |
| tx_hashes=tx_hashes, | |
| estimated_probability_bps=int(processed_market.answer.p_yes * 10000), | |
| ) | |
| tx_receipt = OmenAgentResultMappingContract().add_prediction( | |
| api_keys=keys, | |
| market_address=Web3.to_checksum_address(self.id), | |
| prediction=prediction, | |
| ) | |
| logger.info( | |
| f"Added prediction to market {self.id}. - receipt {tx_receipt['transactionHash'].hex()}." | |
| ) | |
| def store_prediction( | |
| self, processed_market: ProcessedMarket, keys: APIKeys | |
| ) -> None: | |
| reasoning = ( | |
| processed_market.answer.reasoning | |
| if processed_market.answer.reasoning | |
| else "" | |
| ) | |
| ipfs_hash_decoded = HexBytes(HASH_ZERO) | |
| if keys.enable_ipfs_upload: | |
| logger.info("Storing prediction on IPFS.") | |
| try: | |
| ipfs_hash = IPFSHandler(keys).store_agent_result( | |
| IPFSAgentResult(reasoning=reasoning) | |
| ) | |
| ipfs_hash_decoded = ipfscidv0_to_byte32(ipfs_hash) | |
| except Exception as e: | |
| logger.error(f"Failed to store prediction on IPFS: {e}") | |
| # Handle the exception appropriately | |
| tx_hashes = [ | |
| HexBytes(HexStr(i.id)) for i in processed_market.trades if i.id is not None | |
| ] | |
| prediction = ContractPrediction( | |
| publisher=keys.public_key, | |
| ipfs_hash=ipfs_hash_decoded, | |
| tx_hashes=tx_hashes, | |
| estimated_probability_bps=int(processed_market.answer.p_yes * 10000), | |
| ) | |
| try: | |
| tx_receipt = OmenAgentResultMappingContract().add_prediction( | |
| api_keys=keys, | |
| market_address=Web3.to_checksum_address(self.id), | |
| prediction=prediction, | |
| ) | |
| logger.info( | |
| f"Added prediction to market {self.id}. - receipt {tx_receipt['transactionHash'].hex()}." | |
| ) | |
| except Exception as e: | |
| logger.error(f"Failed to add prediction to market: {e}") | |
| # Handle the exception appropriately |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
prediction_market_agent_tooling/deploy/agent.py (3)
336-343: LGTM! Consider improving error message formatting.The simplification of
check_min_required_balance_to_operateimproves readability and maintainability. The use ofmarket_type.market_class.verify_operational_balanceis a good abstraction.Consider using an f-string for the error message to improve readability:
- f"{api_keys=} doesn't have enough operational balance." + f"API keys {api_keys} don't have enough operational balance."
344-355: LGTM! Consider adding a comment for clarity.The new
check_min_required_balance_to_trademethod is well-structured and provides a necessary check before trading. The 1% buffer is a good practice to account for potential fluctuations or fees.Consider adding a brief comment explaining the purpose of the 1% buffer:
- min_required_balance_to_trade = strategy.maximum_possible_bet_amount * 1.01 + # Add a 1% buffer to account for potential fees or price fluctuations + min_required_balance_to_trade = strategy.maximum_possible_bet_amount * 1.01
417-429: LGTM! Consider simplifying the conditional and adding a comment.The addition of the balance check and wxdai to xdai exchange handling is important for ensuring sufficient funds and maintaining the native currency balance.
- Simplify the nested if statement:
- if market_type.is_blockchain_market: - # Exchange wxdai back to xdai if the balance is getting low, so we can keep paying for fees. - if self.min_balance_to_keep_in_native_currency is not None: + if market_type.is_blockchain_market and self.min_balance_to_keep_in_native_currency is not None: + # Exchange wxdai back to xdai if the balance is getting low, so we can keep paying for fees.
- Add a comment explaining the purpose of the wxdai to xdai exchange:
# Ensure sufficient native currency (xdai) balance for gas fees in blockchain markets🧰 Tools
🪛 Ruff
421-423: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- prediction_market_agent_tooling/deploy/agent.py (6 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent_tooling/deploy/agent.py
421-423: Use a single
ifstatement instead of nestedifstatements(SIM102)
🔇 Additional comments (4)
prediction_market_agent_tooling/deploy/agent.py (4)
501-502: LGTM! Good practice for maintaining liquidity.The addition of
check_min_required_balance_to_operateand redeeming winnings before processing markets ensures that the agent has sufficient balance to operate and maintains liquidity.
494-494: LGTM! Good encapsulation of prediction storage.The addition of
market.store_predictionencapsulates the logic for storing predictions within the market object, maintaining a good separation of concerns.
11-12: LGTM! Necessary imports added.The new imports from pydantic and typing_extensions are necessary for the changes made in the file, particularly for the use of
computed_fieldandAnnotated.
527-527:⚠️ Potential issueFix docstring formatting.
The docstring for
after_process_marketsis not properly formatted. It should be enclosed in triple quotes for consistency with Python conventions.Apply this diff to fix the docstring:
- "Executes actions that occur after bets are placed." + """Executes actions that occur after bets are placed."""Likely invalid or redundant comment.
Co-authored-by: Evan Griffiths <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
prediction_market_agent_tooling/markets/agent_market.py (2)
30-32: LGTM: NewProcessedMarketclass added.The
ProcessedMarketclass is well-structured and uses appropriate type hints. Consider adding a brief docstring to explain the purpose of this class and its attributes.class ProcessedMarket(BaseModel): + """ + Represents a processed market with an answer and associated trades. + """ answer: ProbabilisticAnswer trades: list[PlacedTrade]
230-236: LGTM: Newstore_predictionmethod added.The
store_predictionmethod is well-defined with a clear signature and informative docstring. Consider making this method a static method for consistency with the other newly added methods in this class.- def store_prediction( + @staticmethod + def store_prediction( - self, processed_market: ProcessedMarket, keys: APIKeys + processed_market: ProcessedMarket, keys: APIKeys ) -> None: """ If market allows to upload predictions somewhere, implement it in this method. """ raise NotImplementedError("Subclasses must implement this method")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- prediction_market_agent_tooling/markets/agent_market.py (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
prediction_market_agent_tooling/markets/agent_market.py (3)
14-16: LGTM: New imports added.The new imports
PlacedTradeandProbabilisticAnswerare correctly added and likely related to the newProcessedMarketclass.
208-213: LGTM: Newredeem_winningsmethod added.The
redeem_winningsmethod is well-defined with a clear signature and informative docstring. The use of@staticmethodandNotImplementedErroris appropriate for this abstract base class method.
215-228: LGTM: Newget_trade_balanceandverify_operational_balancemethods added.Both methods are well-defined with clear signatures and informative docstrings. The use of
@staticmethodandNotImplementedErroris appropriate for these abstract base class methods. Theverify_operational_balancemethod's docstring correctly implements the suggestion from the past review comment.
kongzii
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
prediction_market_agent_tooling/markets/agent_market.py (3)
208-213: LGTM: New redeem_winnings method.The
redeem_winningsmethod is a good addition for handling manual redemption of winner bets on certain markets. The implementation as an abstract method is appropriate, allowing subclasses to provide specific implementations.Consider adding a note in the docstring to clarify that this method should be implemented by subclasses:
""" On some markets (like Omen), it's needed to manually claim the winner bets. If it's not needed, just implement with `pass`. Note: This method should be implemented by subclasses to provide market-specific redemption logic. """
222-228: LGTM: New verify_operational_balance method with minor suggestion.The
verify_operational_balancemethod is a good addition for checking if the user has sufficient operational balance. The implementation as an abstract method is appropriate, allowing subclasses to provide market-specific implementations. The docstring provides clear guidance and includes a helpful example.Consider adding the return type annotation to the method signature:
@staticmethod def verify_operational_balance(api_keys: APIKeys) -> bool:
230-236: LGTM: New store_prediction method with minor suggestion.The
store_predictionmethod is a valuable addition for uploading predictions to markets that support this feature. The implementation as an abstract method is appropriate, allowing subclasses to provide market-specific implementations. The docstring clearly explains the purpose of the method.Consider adding the return type annotation to the method signature:
def store_prediction( self, processed_market: ProcessedMarket, keys: APIKeys ) -> None:prediction_market_agent_tooling/deploy/agent.py (3)
345-356: Great addition of trade-specific balance check.The new
check_min_required_balance_to_trademethod is a valuable addition that ensures sufficient funds for trading based on the betting strategy. The 1% buffer is a good practice to account for potential fluctuations or fees.Consider adding a check to ensure that
maximum_possible_bet_amountis positive to prevent potential issues with zero or negative bet amounts:strategy = self.get_betting_strategy(market) +if strategy.maximum_possible_bet_amount <= 0: + raise ValueError("Maximum possible bet amount must be greater than zero.") # Have a little bandwidth after the bet. min_required_balance_to_trade = strategy.maximum_possible_bet_amount * 1.01This addition will help prevent unexpected behavior and provide clearer error messages.
418-430: Improved pre-processing checks and balance management.The additions to
before_process_marketenhance the robustness of the trading process by checking the balance and managing the wxdai to xdai exchange for blockchain markets. These changes will help prevent failed transactions due to insufficient funds.Consider simplifying the nested if statement for better readability:
- if market_type.is_blockchain_market: - # Exchange wxdai back to xdai if the balance is getting low, so we can keep paying for fees. - if self.min_balance_to_keep_in_native_currency is not None: + if market_type.is_blockchain_market and self.min_balance_to_keep_in_native_currency is not None: + # Exchange wxdai back to xdai if the balance is getting low, so we can keep paying for fees. withdraw_wxdai_to_xdai_to_keep_balance( api_keys, min_required_balance=self.min_balance_to_keep_in_native_currency, withdraw_multiplier=2, )This change reduces nesting and improves code clarity.
🧰 Tools
🪛 Ruff
422-424: Use a single
ifstatement instead of nestedifstatements(SIM102)
528-528: Format the docstring properly.The docstring for
after_process_marketsis not correctly formatted. In Python, method docstrings should be enclosed in triple quotes.Apply this diff to fix the docstring:
- "Executes actions that occur after bets are placed." + """Executes actions that occur after bets are placed."""This change ensures consistency with Python docstring conventions and improves compatibility with documentation tools.
prediction_market_agent_tooling/markets/omen/omen.py (1)
Line range hint
1284-1300: Update the docstring to accurately reflect the function's behaviorThe
get_total_balancefunction returns the total balance of xDai and/or wxDai for a given address, but its docstring suggests that it checks whether the balance exceeds a minimum required amount. Please update the docstring to accurately describe the function's purpose.Apply this diff to correct the docstring:
def get_total_balance( address: ChecksumAddress, web3: Web3 | None = None, sum_xdai: bool = True, sum_wxdai: bool = True, ) -> xDai: - """ - Checks if the total balance of xDai and wxDai in the wallet is above the minimum required balance. - """ + """Returns the total balance of xDai and/or wxDai for the specified address.""" current_balances = get_balances(address, web3) # xDai and wxDai have equal value and can be exchanged for almost no cost, so we can sum them up. total_balance = 0.0 if sum_xdai: total_balance += current_balances.xdai if sum_wxdai: total_balance += current_balances.wxdai return xdai_type(total_balance)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (3)
- prediction_market_agent_tooling/deploy/agent.py (6 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (3 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (9 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent_tooling/deploy/agent.py
422-424: Use a single
ifstatement instead of nestedifstatements(SIM102)
🔇 Additional comments (6)
prediction_market_agent_tooling/markets/agent_market.py (2)
14-16: LGTM: New imports and ProcessedMarket class.The addition of
PlacedTradeandProbabilisticAnswerimports, along with the newProcessedMarketclass, aligns well with the new functionality being introduced. TheProcessedMarketclass effectively encapsulates the result of market processing.Also applies to: 30-32
215-220: LGTM: New get_trade_balance method.The
get_trade_balancemethod is a valuable addition for retrieving the available trading balance. The implementation as an abstract method is appropriate, allowing subclasses to provide market-specific implementations. The docstring clearly explains the purpose of the method.prediction_market_agent_tooling/deploy/agent.py (4)
11-11: LGTM: Import changes enhance type checking and introduce new functionality.The new imports from Pydantic and the addition of
ProcessedMarketsuggest improvements in type validation and the introduction of new market processing capabilities. These changes align well with best practices for type safety and code organization.Also applies to: 33-33, 38-38
337-344: Excellent refactoring of balance check logic.The simplification of the
check_min_required_balance_to_operatemethod is a great improvement. By delegating the balance verification to the market class throughverify_operational_balance, you've enhanced the separation of concerns and made the code more maintainable. This change allows for market-specific balance checks, which is likely more accurate and flexible.
495-495: Good encapsulation of prediction storage logic.The addition of
market.store_prediction(processed_market=processed_market, keys=keys)improves the separation of concerns by delegating the storage responsibility to the market class. This change enhances maintainability and allows for market-specific storage implementations if needed in the future.
502-503: Improved balance check and winnings redemption logic.The changes in
before_process_marketsalign well with the earlier modifications:
- The simplified call to
check_min_required_balance_to_operateis consistent with its updated method signature.- Replacing
redeem_from_all_user_positionswithmarket_type.market_class.redeem_winningsallows for more flexible, market-specific redemption logic.These improvements enhance the consistency and extensibility of the code.
No description provided.