Add AgentMarket.get_most_recent_trade_datetime #512
Conversation
WalkthroughThe pull request enhances 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 (
|
![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
🧹 Outside diff range and nitpick comments (3)
prediction_market_agent_tooling/markets/agent_market.py (1)
299-300: LGTM: New method enhances AgentMarket interface.The addition of the
get_most_recent_trade_datetimemethod is a valuable enhancement to theAgentMarketclass. It provides a standardized way for subclasses to implement retrieval of the most recent trade datetime for a specific user.Consider adding a brief docstring to explain the method's purpose and return value. For example:
def get_most_recent_trade_datetime(self, user_id: str) -> DatetimeUTC | None: """ Get the datetime of the most recent trade for the specified user. Args: user_id (str): The ID of the user. Returns: DatetimeUTC | None: The datetime of the most recent trade, or None if no trades found. """ raise NotImplementedError("Subclasses must implement this method")tests_integration_with_local_chain/markets/omen/test_omen.py (1)
545-583: LGTM: Comprehensive test forget_most_recent_trade_datetime.The new test function effectively verifies the behavior of
get_most_recent_trade_datetimefor both buying and selling trades. It uses appropriate assertions to check that the returned datetime falls within the expected range.Suggestion for improvement:
Consider adding a small delay between the buy and sell operations to ensure distinct timestamps. This can prevent potential issues on systems with high-resolution timers.You could add a small delay between buy and sell operations like this:
import time # After the buy operation and its assertions time.sleep(0.1) # 100 ms delay dt_before_sell_trade = utcnow() # ... (sell operation code)This ensures that the buy and sell operations have distinct timestamps, even on systems with high-resolution timers.
prediction_market_agent_tooling/markets/omen/omen.py (1)
661-672: New method implementation looks good, but consider optimizing the trade retrieval.The new
get_most_recent_trade_datetimemethod is well-implemented and follows the class's coding style. It correctly retrieves trades for a specific user and market, then returns the most recent trade's datetime. However, there are a couple of points to consider:
The TODO comment on line 669 suggests that the trades might already be sorted. If this is the case, we can optimize the code by removing the sorting step.
If we only need the most recent trade, we could potentially optimize the query to fetch just one trade instead of all trades for the user and market.
Consider the following optimizations:
- Verify if the trades are already sorted and remove the sorting step if unnecessary.
- If possible, modify the
OmenSubgraphHandler().get_trades()method to accept alimitparameter, allowing us to fetch only the most recent trade:def get_most_recent_trade_datetime(self, user_id: str) -> DatetimeUTC | None: trades = OmenSubgraphHandler().get_trades( better_address=Web3.to_checksum_address(user_id), market_id=Web3.to_checksum_address(self.id), limit=1, sort_by='creation_datetime', sort_direction='desc' ) return trades[0].creation_datetime if trades else NoneThis would reduce both the amount of data transferred and the processing required in this method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- prediction_market_agent_tooling/markets/agent_market.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (1 hunks)
- tests_integration_with_local_chain/markets/omen/test_omen.py (3 hunks)
🧰 Additional context used
📓 Learnings (1)
prediction_market_agent_tooling/markets/omen/omen.py (2)
Learnt from: evangriffiths PR: gnosis/prediction-market-agent-tooling#300 File: prediction_market_agent_tooling/markets/omen/omen.py:344-352 Timestamp: 2024-10-08T17:30:32.487Z 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: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.
🔇 Additional comments (3)
prediction_market_agent_tooling/markets/agent_market.py (1)
298-298: LGTM: Indentation correction improves code consistency.The indentation of the
get_user_idmethod has been properly aligned with other methods in the class, enhancing code readability and maintaining consistent style.tests_integration_with_local_chain/markets/omen/test_omen.py (1)
317-317: LGTM: Improved test isolation and alignment with updated method signature.The changes enhance the test's clarity and specificity by:
- Creating a dedicated
api_keysobject with a specific private key.- Updating the
place_betmethod call to include theapi_keysparameter, aligning with the updated method signature.These modifications improve the test's isolation and make it more robust.
Also applies to: 330-335
prediction_market_agent_tooling/markets/omen/omen.py (1)
Line range hint
1-672: Consider refactoring for improved maintainability and addressing TODOsWhile the new
get_most_recent_trade_datetimemethod is a good addition, I have some general observations about the file:
The file is quite large and contains a lot of functionality. Consider splitting it into smaller, more focused modules to improve maintainability.
There are several TODO comments throughout the code. It would be beneficial to address these or create issues to track them for future improvements.
Here are some suggestions:
Consider splitting the
OmenAgentMarketclass into smaller, more focused classes or modules. For example, you could separate the market operations, token operations, and utility functions into different files.Review and address the TODO comments. If they can't be addressed immediately, consider creating GitHub issues to track them and remove the TODO comments from the code.
Implement a linting rule to flag TODO comments in future pull requests, encouraging developers to address them or create proper tracking issues.
To get an overview of the TODOs in the file, we can run the following script:
This will help us identify which TODOs need to be addressed or tracked.
✅ Verification successful
File size and TODO confirmation
The
prediction_market_agent_tooling/markets/omen/omen.pyfile contains 1306 lines, which is significantly large and could benefit from refactoring for better maintainability.Additionally, there's a TODO comment at line 669 that needs to be addressed:
# Sort trades by creation timestamp and return the most recent one. TODO is it already sorted?🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash echo "TODOs found in the file:" grep -n "TODO" prediction_market_agent_tooling/markets/omen/omen.pyLength of output: 233
Script:
#!/bin/bash echo "Line count for the file:" wc -l prediction_market_agent_tooling/markets/omen/omen.pyLength of output: 177
| if not trades: | ||
| return None | ||
|
|
||
| # Sort trades by creation timestamp and return the most recent one. TODO is it already sorted? |
There was a problem hiding this comment.
You could use orderBy + orderDirection on graph
similarly as we allow in getting markets.
Should be quick to add it here
And then argument limit can be introduced to get_trades, so it would fetch only 1 (newest) trade, as we are doing with markets:
However, as this is limited to better + market combination, so just a few trades to fetch, I'm fine with just pinging 🐰 to create an issue as a potential improvement for the codebase 😄
There was a problem hiding this comment.
Great idea! added
| "BET_FROM_PRIVATE_KEY" | ||
| ] = test_keys.bet_from_private_key.get_secret_value() | ||
| api_keys = APIKeys() | ||
| api_keys = APIKeys(BET_FROM_PRIVATE_KEY=test_keys.bet_from_private_key) |
| """ | ||
| market = OmenAgentMarket.from_data_model(pick_binary_market()) | ||
| outcome = True | ||
| api_keys = APIKeys(BET_FROM_PRIVATE_KEY=test_keys.bet_from_private_key) |
There was a problem hiding this comment.
if you already have test_keys: APIKeys,, why it's needed to re-initialise api_keys = APIKeys? 🤔
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent_tooling/markets/omen/omen.py (1)
662-673: Implement sorting optimization and remove TODO comment.The implementation of
get_most_recent_trade_datetimelooks good overall. However, there are two points to address:
The TODO comment on line 670 should be resolved. If the trades are already sorted by the API, you can remove the sorting step entirely.
If the trades are not pre-sorted, you can optimize the method by using the
limitparameter in theget_tradescall instead of fetching all trades and then sorting them.Here's a suggested optimization:
def get_most_recent_trade_datetime(self, user_id: str) -> DatetimeUTC | None: trades = OmenSubgraphHandler().get_trades( better_address=Web3.to_checksum_address(user_id), market_id=Web3.to_checksum_address(self.id), limit=1, sort_by="creation_datetime", sort_direction="desc" ) return trades[0].creation_datetime if trades else NoneThis assumes that
OmenSubgraphHandler().get_trades()supportslimit,sort_by, andsort_directionparameters. If it doesn't, consider adding these features to improve efficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- prediction_market_agent_tooling/markets/omen/omen.py (2 hunks)
🧰 Additional context used
📓 Learnings (1)
prediction_market_agent_tooling/markets/omen/omen.py (2)
Learnt from: evangriffiths PR: gnosis/prediction-market-agent-tooling#300 File: prediction_market_agent_tooling/markets/omen/omen.py:344-352 Timestamp: 2024-10-08T17:30:32.487Z 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: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.
🔇 Additional comments (2)
prediction_market_agent_tooling/markets/omen/omen.py (2)
189-189: LGTM: Indentation correction.The indentation of the
get_user_idmethod has been corrected, improving code consistency and readability.
Line range hint
1-673: Overall assessment: Good addition with minor improvements needed.The changes in this file successfully implement the new
get_most_recent_trade_datetimemethod as described in the PR objectives. The indentation correction forget_user_idimproves code consistency. Consider implementing the suggested optimization for the new method to enhance its efficiency.
| @@ -316,24 +315,23 @@ def test_omen_buy_and_sell_outcome( | |||
| outcome_str = get_bet_outcome(outcome) | |||
| bet_amount = market.get_bet_amount(amount=0.4) | |||
|
|
|||
| # TODO hack until https://github.com/gnosis/prediction-market-agent-tooling/issues/266 is complete | |||
There was a problem hiding this comment.
Issue has been fixed. I picked up on this as I initially added a test here, but then moved it to tests/markets/omen/test_omen.py. Will just leave this code-tidy here 😃
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
tests/markets/omen/test_omen.py (1)
296-323: Improve test robustness and efficiency.The test function
test_get_most_recent_trade_datetimeis a good addition to verify the behavior ofget_most_recent_trade_datetime. However, there are a few areas that could be improved:
- Consider parameterizing the test with different market IDs and user addresses to make it more robust against data changes.
- The use of
sys.maxsizeas the limit forget_tradesmight be excessive. Consider using a more reasonable upper limit or paginating the results.- The assertions for multiple trades and unique timestamps might fail if the market conditions change. Consider making these checks conditional or use a different approach to ensure the test's validity.
- To improve efficiency, you could retrieve a smaller number of trades sorted by creation time in descending order, instead of fetching all trades.
Here's a suggested refactor to address these points:
import pytest from web3 import Web3 @pytest.mark.parametrize("market_id, user_id", [ ("0x1e0e1d092bffebfb4fa90eeba7bfeddcebc9751c", "0x2DD9f5678484C1F59F97eD334725858b938B4102"), # Add more test cases here ]) def test_get_most_recent_trade_datetime(market_id: str, user_id: str) -> None: """ Tests that `get_most_recent_trade_datetime` returns the correct datetime from all possible trade datetimes. """ market_id = Web3.to_checksum_address(market_id) user_id = Web3.to_checksum_address(user_id) sgh = OmenSubgraphHandler() trades = sgh.get_trades( limit=10, # Adjust this value as needed better_address=user_id, market_id=market_id, sort_by="creation_time", sort_direction="desc" ) if not trades: pytest.skip(f"No trades found for market {market_id} and user {user_id}") market = OmenAgentMarket.get_binary_market(id=market_id) most_recent_trade_datetime = check_not_none( market.get_most_recent_trade_datetime(user_id=user_id) ) assert trades[0].creation_datetime == most_recent_trade_datetime, \ "The most recent trade datetime doesn't match the expected value"This refactored version addresses the identified issues and makes the test more robust and efficient.
tests_integration_with_local_chain/markets/omen/test_omen.py (1)
320-320: LGTM! Improved test robustness and consistency.The changes enhance the test by:
- Consistently using
api_keysin method calls, aligning with the updated API structure.- Checking for sufficient funds before placing a bet, which is a good practice.
These improvements make the test more robust and consistent with the rest of the codebase.
Consider adding an assertion to verify that the bet amount is less than or equal to the available balance:
assert bet_amount.amount <= balances.xdai + balances.wxdai, "Insufficient funds for the bet"This would make the fund check more explicit and fail the test if there are insufficient funds.
Also applies to: 326-334, 344-344
prediction_market_agent_tooling/markets/omen/omen_subgraph_handler.py (4)
Line range hint
549-559: LGTM! Consider adding parameter descriptions.The new parameters
limit,sort_by_field, andsort_directionenhance the flexibility of theget_tradesmethod. This is a good improvement that allows for more controlled querying of trades.Consider adding parameter descriptions in the method's docstring to explain the purpose and expected values for these new parameters, especially
sort_by_fieldandsort_direction.
585-595: LGTM! Consider extracting the query parameters logic.The implementation correctly incorporates the new parameters into the
fpmmTradesquery. The use of a dictionary for optional parameters is a clean approach.To improve readability, consider extracting the logic for building the
optional_paramsdictionary into a separate method. This would make theget_tradesmethod more concise and easier to understand. For example:def _build_query_params(self, sort_by_field: FieldPath | None, sort_direction: str | None) -> dict: params = {} if sort_by_field is not None: params["orderBy"] = sort_by_field if sort_direction is not None: params["orderDirection"] = sort_direction return params # In get_trades method optional_params = self._build_query_params(sort_by_field, sort_direction)
Line range hint
597-617: Consider updatingget_betsto use new parameters.The
get_betsmethod currently doesn't utilize the newlimit,sort_by_field, andsort_directionparameters added toget_trades. While this might be intentional, it's worth considering whetherget_betsshould also have the ability to limit and sort the results.Consider updating the
get_betsmethod signature and implementation to include and pass along the new parameters:def get_bets( self, better_address: ChecksumAddress | None = None, start_time: DatetimeUTC | None = None, end_time: t.Optional[DatetimeUTC] = None, market_id: t.Optional[ChecksumAddress] = None, filter_by_answer_finalized_not_null: bool = False, market_opening_after: DatetimeUTC | None = None, collateral_amount_more_than: Wei | None = None, limit: int | None = None, sort_by_field: FieldPath | None = None, sort_direction: str | None = None, ) -> list[OmenBet]: return self.get_trades( better_address=better_address, start_time=start_time, end_time=end_time, market_id=market_id, filter_by_answer_finalized_not_null=filter_by_answer_finalized_not_null, type_="Buy", # We consider `bet` to be only the `Buy` trade types. market_opening_after=market_opening_after, collateral_amount_more_than=collateral_amount_more_than, limit=limit, sort_by_field=sort_by_field, sort_direction=sort_direction, )This change would provide consistent functionality between
get_tradesandget_bets, allowing users ofget_betsto also benefit from the new sorting and limiting capabilities.
Line range hint
549-595: Consider applying similar changes to other query methods.The changes to
get_tradesare well-implemented and consistent with the existing code style. To maintain consistency across the entireOmenSubgraphHandlerclass, consider applying similar changes to other query methods that might benefit from limiting and sorting capabilities.Review other query methods in the class, such as
get_omen_binary_markets,get_questions,get_answers, andget_responses. If appropriate, update these methods to includelimit,sort_by_field, andsort_directionparameters, and implement the sorting logic similarly toget_trades. This would provide a consistent API across all query methods in the class.For example:
def get_questions( self, limit: int | None = None, sort_by_field: FieldPath | None = None, sort_direction: str | None = None, # ... existing parameters ... ) -> list[RealityQuestion]: # ... existing implementation ... optional_params = self._build_query_params(sort_by_field, sort_direction) questions = self.realityeth_subgraph.Query.questions( first=(limit if limit else sys.maxsize), where=where_stms, **optional_params, ) # ... rest of the implementation ...This would create a more uniform and predictable API across the entire
OmenSubgraphHandlerclass.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- prediction_market_agent_tooling/markets/omen/omen.py (2 hunks)
- prediction_market_agent_tooling/markets/omen/omen_subgraph_handler.py (3 hunks)
- tests/markets/omen/test_omen.py (2 hunks)
- tests_integration_with_local_chain/markets/omen/test_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#456 File: prediction_market_agent_tooling/markets/omen/omen_contracts.py:578-578 Timestamp: 2024-10-08T17:30:32.487Z Learning: The `askQuestion` method calls in `tests_integration/markets/omen/test_reality.py`, `tests_integration_with_local_chain/markets/omen/test_reality.py`, and `prediction_market_agent_tooling/markets/omen/omen.py` have been updated to use `DatetimeUTC` for the `opening` parameter, and are consistent.Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#456 File: prediction_market_agent_tooling/markets/omen/omen_contracts.py:578-578 Timestamp: 2024-10-04T09:15:59.998Z Learning: The `askQuestion` method calls in `tests_integration/markets/omen/test_reality.py`, `tests_integration_with_local_chain/markets/omen/test_reality.py`, and `prediction_market_agent_tooling/markets/omen/omen.py` have been updated to use `DatetimeUTC` for the `opening` parameter, and are consistent.
🔇 Additional comments (3)
tests/markets/omen/test_omen.py (1)
1-1: LGTM: Import ofsysmodule.The import of the
sysmodule is appropriate as it's used later in the file to accesssys.maxsize.tests_integration_with_local_chain/markets/omen/test_omen.py (1)
356-356: Approved: Minor code style improvementThe indentation change improves code readability and consistency.
prediction_market_agent_tooling/markets/omen/omen_subgraph_handler.py (1)
Line range hint
549-617: Overall, good improvements to the OmenSubgraphHandler class.The changes to the
get_tradesmethod enhance its flexibility by adding limit and sorting capabilities. The implementation is consistent with the existing codebase and follows good practices.Summary of suggestions:
- Add parameter descriptions to the
get_tradesmethod docstring.- Consider extracting the query parameters logic into a separate method.
- Update the
get_betsmethod to utilize the new parameters.- Apply similar changes to other query methods in the class for consistency.
These changes and suggestions improve the overall functionality and maintainability of the
OmenSubgraphHandlerclass.
| @@ -186,6 +186,7 @@ def liquidate_existing_positions( | |||
| amount=token_amount, | |||
| auto_withdraw=False, | |||
| web3=web3, | |||
| api_keys=api_keys, | |||
There was a problem hiding this comment.
another fix for a local chain test - not sure how it was passing before!
|
Failing CI test (tests_integration_with_local_chain/markets/omen/test_omen.py::test_place_bet_with_autodeposit) is unrelated to this PR, and was failing already (e.g. see #506) |
No description provided.