137 arbitrage agent #511
137 arbitrage agent #511
Conversation
…trage-agent # Conflicts: # poetry.lock # pyproject.toml
WalkthroughThis pull request introduces updates to several files, primarily focusing on 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: 6
🧹 Outside diff range and nitpick comments (2)
tests/agents/arbitrage_agent/test_arbitrage_agent.py (2)
13-45: LGTM: Well-designed fixtures with a minor suggestion.The fixtures are well-structured and effectively isolate components for testing. The use of mocking in the
arbitrage_agentfixture and the creation of distinct market scenarios are particularly good practices.Consider adding type hints to the mock objects in the market fixtures for improved code clarity:
m1 = Mock(spec=OmenAgentMarket)This change would make the intended type of the mock object more explicit.
48-66: LGTM: Well-structured test with parameterization. Consider additional test cases.The test function is well-designed, using parameterization to efficiently test both related and unrelated market scenarios. It effectively utilizes the fixtures and checks the correlation calculation against a threshold.
Consider adding the following test cases to increase coverage:
- Test with edge case correlations (e.g., correlation very close to the threshold).
- Test the behavior when markets have identical questions.
- Test with markets in different languages to ensure language-agnostic correlation.
Example:
@pytest.mark.parametrize( "market1_question, market2_question, expected_correlation", [ ("Will X happen?", "Will X occur?", 0.99), # Nearly identical ("Will X happen?", "Will X happen?", 1.0), # Identical ("Will X happen?", "Wird X passieren?", 0.9), # Different languages ] ) def test_correlation_edge_cases( arbitrage_agent: DeployableOmenArbitrageAgent, market1_question: str, market2_question: str, expected_correlation: float, ) -> None: market1 = Mock(spec=OmenAgentMarket, question=market1_question) market2 = Mock(spec=OmenAgentMarket, question=market2_question) correlation = arbitrage_agent.calculate_correlation_between_markets( market=market1, related_market=market2 ) assert pytest.approx(correlation.correlation, abs=0.01) == expected_correlationThis additional test would cover more scenarios and increase the robustness of your test suite.
📜 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 (3)
- prediction_market_agent/agents/arbitrage_agent/data_models.py (1 hunks)
- prediction_market_agent/agents/arbitrage_agent/deploy.py (1 hunks)
- tests/agents/arbitrage_agent/test_arbitrage_agent.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
tests/agents/arbitrage_agent/test_arbitrage_agent.py (2)
1-10: LGTM: Imports are appropriate and well-organized.The imports section includes all necessary modules and classes for the test file. The organization is logical, separating standard library imports from project-specific ones.
1-66: Overall, excellent test structure with room for minor enhancements.This test file is well-organized and effectively tests the core functionality of the arbitrage agent. The use of fixtures, parameterization, and mocking demonstrates good testing practices.
To further improve the test suite:
- Consider adding type hints to mock objects in fixtures.
- Expand test cases to cover edge scenarios and language-agnostic correlation.
These enhancements will increase the robustness and clarity of your test suite, ensuring better coverage of the arbitrage agent's functionality.
prediction_market_agent/agents/arbitrage_agent/data_models.py (1)
16-18: Verify compatibility with Pydantic version and decorator usageThe use of
@computed_fieldalongside@propertymay not be necessary and could lead to unexpected behavior. Additionally, the@computed_fielddecorator is available in Pydantic v2.Ensure that the project uses Pydantic v2. If not, consider one of the following:
- Option 1: Remove the
@computed_fielddecorator if not required.- Option 2: Adjust the code to match the appropriate Pydantic version's conventions.
Example adjustment:
- @computed_field # type: ignore[prop-decorator] @property def potential_profit_per_bet_unit(self) -> float:Please verify that the computed properties behave as intended after making changes.
Also applies to: 36-38, 45-47
prediction_market_agent/agents/arbitrage_agent/deploy.py (2)
74-74: Verify the model name 'gpt-4o-mini'In the
_build_chainmethod, the model parameter is set to'gpt-4o-mini'. Please ensure that this is the correct model name and that it is accessible via the OpenAI API.
138-138: Ensure unique market identificationThe condition
related_market.id != market.idchecks if the markets are different. Verify thatidattributes are correctly populated and comparable betweenAgentMarketand the market objects retrieved from the subgraph handler.
| "main_market_question": market, | ||
| "related_market_question": related_market, |
There was a problem hiding this comment.
Incorrect arguments passed to self.chain.invoke
In the get_correlated_markets method, you're passing market and related_market objects to self.chain.invoke, whereas the chain expects strings containing the market questions.
Apply this diff to fix the arguments:
- "main_market_question": market,
- "related_market_question": related_market,
+ "main_market_question": market.question,
+ "related_market_question": related_market.question,📝 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.
| "main_market_question": market, | |
| "related_market_question": related_market, | |
| "main_market_question": market.question, | |
| "related_market_question": related_market.question, |
| @pytest.fixture(scope="module") | ||
| def unrelated_market() -> t.Generator[AgentMarket, None, None]: | ||
| m1 = Mock(OmenAgentMarket, wraps=OmenAgentMarket) | ||
| m1.question = "Will Donald Duck ever retire from his adventures in Duckburg?" |
| self, pair: CorrelatedMarketPair | ||
| ) -> list[Trade]: | ||
| market_to_bet_yes, market_to_bet_no = pair.main_market, pair.related_market | ||
| total_amount: BetAmount = pair.main_market.get_tiny_bet_amount() |
There was a problem hiding this comment.
This seems like something that should be configurable at the top level
| Trade( | ||
| trade_type=TradeType.BUY, | ||
| outcome=True, | ||
| amount=TokenAmount( | ||
| amount=amount_yes, currency=market_to_bet_yes.currency | ||
| ), | ||
| ), | ||
| Trade( | ||
| trade_type=TradeType.BUY, | ||
| outcome=False, | ||
| amount=TokenAmount( | ||
| amount=amount_no, currency=market_to_bet_no.currency | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Buying [market1: yes, market2: no] assumes positive correlation between markets. I think you need to have some logic that chooses the directions based on correlation
There was a problem hiding this comment.
Positive correlation is assumed here -> https://github.com/gnosis/prediction-market-agent/pull/511/files/433656d955694050935c662e1574f80c552d3cc7#diff-1b9d8df9eb7cb55918e642ebc0f26902d5211d4946a8c4d0454e9889dd05c371R138-R140
I didn't implement logic for the negatively correlated case (it was on an earlier version), I think positive correlation is a good start. Also, negative correlation is harder to determine, e.g. Trump president vs Kamala president is 100% negatively correlated but not trivial for LLM to tell, hence preferred to stick to 100% correlation case.
Happy with suggestions though.
There was a problem hiding this comment.
Oh okay. Fwiw I think it's okay if the LLMs find it hard to determine that, as long as there aren't false positives.
There was a problem hiding this comment.
Going with your bool suggestion from the other thread.
| class DeployableOmenArbitrageAgent(DeployableTraderAgent): | ||
| """Agent that places mirror bets on Omen for (quasi) risk-neutral profit.""" | ||
|
|
||
| correlation_threshold: float = 0.8 |
There was a problem hiding this comment.
I thought the plan was to go with the 'perfectly +vely/-vely correlated only" cases to begin with?
Looks like the implementation of split_bet_amount_between_yes_and_no also assumes perfect correlation, so I think you make them consistent by either:
- changing the split function to take correlation into account
- change/simplify the prompting in
_build_chainto only get perfectly correlated markets
I'd vote (2)
There was a problem hiding this comment.
I also like (2) better, but markets that are obviously strongly correlated, e.g. like in the test
m1.question = "Will Kamala Harris win the US presidential election?"
m1.question = "Will Kamala Harris become the US president in 2025?"
only delivers 0.8 correlation.
There was a problem hiding this comment.
I ran it with gpt-4o instead of 4o-mini and it gives 1.0!!
from prediction_market_agent.agents.arbitrage_agent.deploy import (
DeployableOmenArbitrageAgent,
)
agent = DeployableOmenArbitrageAgent()
correlation = agent.chain.invoke(
{
"main_market_question": "Will Kamala Harris win the US presidential election?",
"related_market_question": "Will Kamala Harris become the US president in 2025?",
}
)
print(correlation)
Maybe use that instead? Should be very cheap to run these prompts. Have you checked?
There was a problem hiding this comment.
An alternative approach and (IMO simpler) is to prompt the agent to return a boolean, which is True if 'perfect or near-perfect correlation'. Then you don't need any numerical filtering based on some threshold. I say this, because we know that LLMs aren't the best at giving accurate numerical values!
There was a problem hiding this comment.
Sounds like a good approach, implemented
| correlation_threshold: float = 0.8 | ||
|
|
||
| def load(self) -> None: | ||
| self.subgraph_handler = OmenSubgraphHandler() |
There was a problem hiding this comment.
Any reason you've made this agent omen-specific? Looks like the only thing you've used the sgh for is to get markets. So it looks like this could easily be changed to become market-platform agnostic
There was a problem hiding this comment.
I made it Omen-specific because the related_markets should be queryable via Pinecone (because of the TTA agent), and only Omen agents are stored there currently.
There was a problem hiding this comment.
Oh I didn't realise that. Where does the indexing of omen markets in the pincone db happen? If it's not part of this agent, it goes against @kongzii's philosophy of "anyone should be able to run the agents (if they have appropriate api keys), and get the same kind of results".
Regardless, there doesn't seem to be a need to have any Omen dependencies here - you could just have a if market_type != MarketType.Omen: raise ... check
There was a problem hiding this comment.
Indexing occurs below
I don't think it prevents someone from running the agent - you just need Pinecone api keys and, on the first run, it will fill out Pinecone and have the same dataset as we do.
Regardless, there doesn't seem to be a need to have any Omen dependencies here - you could just have a if market_type != MarketType.Omen: raise ... check
Makes sense to me, implementing this.
There was a problem hiding this comment.
Well it does if the user wants to run just this agent, and not the DeployableThinkThoroughlyAgent as well! I think we need to do self.pinecone_handler.insert_all_omen_markets_if_not_exists(...) somewhere here
There was a problem hiding this comment.
Sure - added 55aee24
There was a problem hiding this comment.
Just had a look at what pinecone_handler.update_markets is doing. Is this what we want? Two things I'm not sure about:
- It doesn't look like it's removing closed markets from the index
- It only fetches the last 7 days' worth of created markets
created_after = utcnow() - datetime.timedelta(days=7). Don't we want all open markets?
| @pytest.fixture(scope="module") | ||
| def related_market() -> t.Generator[AgentMarket, None, None]: | ||
| m1 = Mock(OmenAgentMarket, wraps=OmenAgentMarket) | ||
| m1.question = "Will Kamala Harris become the US president in 2025?" |
There was a problem hiding this comment.
I'm a bit confused by this, if you expect high correlation with the other question shouldn't this be either:
"Will Kamala Harris be the US president in 2025?"or"Will Kamala Harris become the US president in 2024?"
because the election is in Nov 2024
There was a problem hiding this comment.
Oh scrap that sorry, elected president takes office in Jan 2025. Forgot!
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prediction_market_agent/agents/arbitrage_agent/deploy.py (1 hunks)
- prediction_market_agent/agents/arbitrage_agent/prompt.py (1 hunks)
🧰 Additional context used
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/agents/arbitrage_agent/data_models.py (1)
12-35: LGTM: CorrelatedMarketPair class and potential_profit_per_bet_unit method are well-implemented.The class structure and method implementation are appropriate. The method handles the case where total probability is zero, avoiding potential division by zero errors.
However, there's a minor improvement we can make to the comment:
Update the comment to remove the reference to LLM prompt:
- # Negative correlations are not yet supported by the current LLM prompt, hence not handling those for now. + # Negative correlations are not yet supported, hence not handling those for now.
📜 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 (4)
- prediction_market_agent/agents/arbitrage_agent/data_models.py (1 hunks)
- prediction_market_agent/agents/arbitrage_agent/deploy.py (1 hunks)
- prediction_market_agent/agents/arbitrage_agent/prompt.py (1 hunks)
- tests/agents/arbitrage_agent/test_arbitrage_agent.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (12)
tests/agents/arbitrage_agent/test_arbitrage_agent.py (5)
1-10: LGTM: Imports are well-organized and follow best practices.The imports are correctly structured, following the PEP 8 style guide. They include all necessary modules and classes for the test file.
13-24: LGTM: Well-implemented arbitrage_agent fixture.The fixture correctly mocks the
loadmethod and builds the agent's chain, ensuring isolated testing of theDeployableOmenArbitrageAgent.
27-31: LGTM: Market fixtures are well-defined for testing correlation scenarios.The main_market, related_market, and unrelated_market fixtures create appropriate mock OmenAgentMarket instances with relevant questions for testing market correlations.
Also applies to: 33-38, 40-45
48-63: LGTM: Well-structured and comprehensive test function.The
test_correlation_for_similar_marketsfunction is well-implemented:
- It uses parameterization to test both correlated and uncorrelated scenarios efficiently.
- The function correctly utilizes the fixtures to set up test cases.
- It tests the
calculate_correlation_between_marketsmethod of the arbitrage agent.- The assertion checks for the expected correlation result.
The test covers both positive (correlated) and negative (uncorrelated) cases, ensuring thorough testing of the correlation calculation functionality.
1-63: Great job on implementing comprehensive tests for the arbitrage agent!This test file is well-structured and follows best practices for Python testing:
- Proper use of fixtures for setting up test scenarios.
- Effective use of parameterization for testing multiple cases.
- Clear and descriptive test function that covers both correlated and uncorrelated markets.
- Good isolation of the arbitrage agent through mocking.
The tests provide solid coverage for the correlation calculation functionality of the arbitrage agent. This will help ensure the reliability and correctness of the agent's behavior in different market scenarios.
prediction_market_agent/agents/arbitrage_agent/data_models.py (3)
1-5: LGTM: Imports are appropriate and concise.The imports are well-chosen for the functionality implemented in this file. They include necessary components from typing, Pydantic, and the custom AgentMarket class.
7-10: LGTM: Correlation class is well-defined and addresses previous feedback.The Correlation class is simple and clear. The addition of the 'reasoning' field addresses the previous review comment, which will aid in debugging from langfuse traces.
55-71:⚠️ Potential issueHandle potential division by zero in
split_bet_amount_between_yes_and_noThe method correctly implements the splitting logic as described. However, there's still a risk of division by zero if the sum of probabilities is zero.
Add a check to ensure the denominator is not zero before performing the division:
def split_bet_amount_between_yes_and_no( self, total_bet_amount: float ) -> t.Tuple[float, float]: """Splits total bet amount following equations below: A1/p1 = A2/p2 (same profit regardless of outcome resolution) A1 + A2 = total bet amount """ + denominator = ( + self.market_to_bet_yes.current_p_yes + + self.market_to_bet_no.current_p_no + ) + if denominator == 0: + raise ValueError("Sum of probabilities is zero, cannot split bet amount") amount_to_bet_yes = ( total_bet_amount * self.market_to_bet_yes.current_p_yes - / ( - self.market_to_bet_yes.current_p_yes - + self.market_to_bet_no.current_p_no - ) + / denominator ) amount_to_bet_no = total_bet_amount - amount_to_bet_yes return amount_to_bet_yes, amount_to_bet_noThis change ensures that we don't attempt to divide by zero, which would cause a runtime error.
Likely invalid or redundant comment.
prediction_market_agent/agents/arbitrage_agent/deploy.py (4)
1-42: LGTM: Import statements are appropriate and well-organized.The import statements are comprehensive and relevant to the functionality of the
DeployableOmenArbitrageAgentclass. They include necessary modules from both external libraries and internal packages.
73-89: LGTM:_build_chainmethod is well-implementedThe
_build_chainmethod effectively constructs a language model chain using OpenAI's ChatGPT, a custom prompt template, and a Pydantic output parser. The use ofAPIKeys().openai_api_key_secretstr_v1for the API key is a good practice for keeping sensitive information secure.
1-179: Overall assessment: Well-structured implementation with room for improvementsThe
DeployableOmenArbitrageAgentclass provides a solid foundation for arbitrage trading on the Omen platform. The code is well-organized and implements complex logic for market correlation analysis and trade building. However, there are several areas where the implementation could be improved:
- Error handling: Add try-except blocks in methods making external API calls to improve robustness.
- Configurability: Make certain parameters (e.g., model, bet amounts) configurable to enhance flexibility.
- Parameter utilization: Incorporate
answerandexisting_positionparameters in the trade building logic for more informed decision-making.- Input validation: Ensure correct data types are passed to methods, especially when invoking the language model chain.
Addressing these points will significantly enhance the agent's reliability, flexibility, and effectiveness in arbitrage trading scenarios.
55-68:⚠️ Potential issueUse the input
limitparameter inget_marketsThe
get_marketsmethod currently ignores the inputlimitparameter and hardcodes it to 100. To improve flexibility, consider using the inputlimitparameter:return super().get_markets( market_type=market_type, - limit=100, + limit=limit, sort_by=SortBy.HIGHEST_LIQUIDITY, filter_by=FilterBy.OPEN, )This change allows the caller to control the number of markets fetched while still focusing on high liquidity markets.
Likely invalid or redundant comment.
| prompt_template = """Given two markets, MARKET 1 and MARKET 2, provide a boolean value that represents the correlation between these two markets' outcomes. Return True if the outcomes are perfectly or nearly perfectly correlated, meaning there is a high probability that both markets resolve to the same outcome. Return False if the correlation is weak or non-existent. | ||
| Correlation can also be understood as the conditional probability that market 2 resolves to YES, given that market 1 resolved to YES. | ||
| In addition to the boolean value, explain the reasoning behind your decision. | ||
| [MARKET 1] | ||
| {main_market_question} | ||
| [MARKET 2] | ||
| {related_market_question} | ||
| Follow the formatting instructions below for producing an output in the correct format. | ||
| {format_instructions}""" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine the prompt template for more accurate correlation representation.
While the current template provides a framework for assessing market correlations, there are several areas where it could be improved:
-
Boolean output vs. float: The current template asks for a boolean value, which oversimplifies the concept of correlation. Consider reverting to a float between -1 and 1 for more nuanced representation.
-
Correlation definition: The definition focusing on conditional probability may be misleading. Consider including a more comprehensive explanation of correlation.
-
Handling of negative correlations and unrelated markets: The template doesn't provide guidance on these scenarios. Include explanations for these cases.
Consider updating the prompt template to address these points. Here's a suggested improvement:
prompt_template = """Given two prediction markets, MARKET 1 and MARKET 2, provide a float value between -1 and 1 that represents the correlation between these two markets' outcomes.
Correlation measures the strength and direction of the relationship between the outcomes of the two markets:
- A correlation of 1 means the markets are perfectly positively correlated (if market 1 is YES, market 2 is very likely to be YES).
- A correlation of -1 means the markets are perfectly negatively correlated (if market 1 is YES, market 2 is very likely to be NO).
- A correlation of 0 means the markets are not correlated (the outcome of market 1 does not influence market 2).
If the markets appear to be completely unrelated, output 0.
[MARKET 1]
{main_market_question}
[MARKET 2]
{related_market_question}
Analyze the relationship between these markets and provide your correlation estimate. In addition to the float value, explain the reasoning behind your decision.
Follow the formatting instructions below for producing an output in the correct format.
{format_instructions}"""This updated template provides a more comprehensive explanation of correlation and how to interpret different values, which should lead to more accurate and consistent results.
| @computed_field # type: ignore[prop-decorator] | ||
| @property | ||
| def market_to_bet_yes(self) -> AgentMarket: | ||
| return ( | ||
| self.main_market | ||
| if self.main_market.current_p_yes <= self.related_market.current_p_yes | ||
| else self.related_market | ||
| ) | ||
|
|
||
| @computed_field # type: ignore[prop-decorator] | ||
| @property | ||
| def market_to_bet_no(self) -> AgentMarket: | ||
| return ( | ||
| self.main_market | ||
| if self.main_market.current_p_yes > self.related_market.current_p_yes | ||
| else self.related_market | ||
| ) |
There was a problem hiding this comment.
Correct the logic in market_to_bet_no method
The market_to_bet_yes method looks good. However, the market_to_bet_no method's logic doesn't align with its purpose.
Update the condition in the market_to_bet_no method to compare current_p_no:
def market_to_bet_no(self) -> AgentMarket:
return (
self.main_market
- if self.main_market.current_p_yes > self.related_market.current_p_yes
+ if self.main_market.current_p_no <= self.related_market.current_p_no
else self.related_market
)This change ensures we're selecting the market with the higher "NO" probability, which is consistent with the method's name and purpose.
📝 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.
| @computed_field # type: ignore[prop-decorator] | |
| @property | |
| def market_to_bet_yes(self) -> AgentMarket: | |
| return ( | |
| self.main_market | |
| if self.main_market.current_p_yes <= self.related_market.current_p_yes | |
| else self.related_market | |
| ) | |
| @computed_field # type: ignore[prop-decorator] | |
| @property | |
| def market_to_bet_no(self) -> AgentMarket: | |
| return ( | |
| self.main_market | |
| if self.main_market.current_p_yes > self.related_market.current_p_yes | |
| else self.related_market | |
| ) | |
| @computed_field # type: ignore[prop-decorator] | |
| @property | |
| def market_to_bet_yes(self) -> AgentMarket: | |
| return ( | |
| self.main_market | |
| if self.main_market.current_p_yes <= self.related_market.current_p_yes | |
| else self.related_market | |
| ) | |
| @computed_field # type: ignore[prop-decorator] | |
| @property | |
| def market_to_bet_no(self) -> AgentMarket: | |
| return ( | |
| self.main_market | |
| if self.main_market.current_p_no <= self.related_market.current_p_no | |
| else self.related_market | |
| ) |
| class DeployableOmenArbitrageAgent(DeployableTraderAgent): | ||
| """Agent that places mirror bets on Omen for (quasi) risk-neutral profit.""" | ||
|
|
||
| model = "gpt-4o-mini" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the model attribute configurable
The model attribute is currently hardcoded to "gpt-4o-mini". To improve flexibility and allow for easier updates or testing with different models, consider making it a configurable parameter, either through the constructor or a configuration file.
| def load(self) -> None: | ||
| self.subgraph_handler = OmenSubgraphHandler() | ||
| self.pinecone_handler = PineconeHandler() | ||
| self.chain = self._build_chain() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling to the load method
The load method initializes important components of the agent. To improve robustness, consider adding error handling to catch and log any exceptions that might occur during initialization of subgraph_handler, pinecone_handler, or chain.
| @observe() | ||
| def get_correlated_markets(self, market: AgentMarket) -> list[CorrelatedMarketPair]: | ||
| # We try to find similar, open markets which point to the same outcome. | ||
| correlated_markets = [] | ||
| related = self.pinecone_handler.find_nearest_questions_with_threshold( | ||
| limit=10, text=market.question | ||
| ) | ||
|
|
||
| omen_markets = self.subgraph_handler.get_omen_binary_markets( | ||
| limit=len(related), | ||
| id_in=[i.market_address.lower() for i in related], | ||
| resolved=False, | ||
| ) | ||
|
|
||
| # Note that negative correlation is hard - e.g. for the US presidential election, markets on each candidate are not seen as -100% correlated. | ||
| for related_market in omen_markets: | ||
| result: Correlation = self.chain.invoke( | ||
| { | ||
| "main_market_question": market, | ||
| "related_market_question": related_market, | ||
| }, | ||
| config=get_langfuse_langchain_config(), | ||
| ) | ||
| if related_market.id != market.id and result.near_perfect_correlation: | ||
| related_agent_market = OmenAgentMarket.from_data_model(related_market) | ||
| correlated_markets.append( | ||
| CorrelatedMarketPair( | ||
| main_market=market, | ||
| related_market=related_agent_market, | ||
| correlation=result.correlation, | ||
| ) | ||
| ) | ||
| return correlated_markets |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix arguments passed to self.chain.invoke
In the get_correlated_markets method, you're passing market and related_market objects to self.chain.invoke, whereas the chain expects strings containing the market questions. Update the code as follows:
result: Correlation = self.chain.invoke(
{
- "main_market_question": market,
- "related_market_question": related_market,
+ "main_market_question": market.question,
+ "related_market_question": related_market.question,
},
config=get_langfuse_langchain_config(),
)This change ensures that the correct data is passed to the language model for correlation analysis.
Add error handling for external API calls
The get_correlated_markets method makes several external API calls (to Pinecone, Omen subgraph, and the language model) without any error handling. Consider adding try-except blocks to catch and handle potential exceptions from these calls. This will improve the robustness of the method and prevent potential crashes due to network issues or API failures.
📝 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.
| @observe() | |
| def get_correlated_markets(self, market: AgentMarket) -> list[CorrelatedMarketPair]: | |
| # We try to find similar, open markets which point to the same outcome. | |
| correlated_markets = [] | |
| related = self.pinecone_handler.find_nearest_questions_with_threshold( | |
| limit=10, text=market.question | |
| ) | |
| omen_markets = self.subgraph_handler.get_omen_binary_markets( | |
| limit=len(related), | |
| id_in=[i.market_address.lower() for i in related], | |
| resolved=False, | |
| ) | |
| # Note that negative correlation is hard - e.g. for the US presidential election, markets on each candidate are not seen as -100% correlated. | |
| for related_market in omen_markets: | |
| result: Correlation = self.chain.invoke( | |
| { | |
| "main_market_question": market, | |
| "related_market_question": related_market, | |
| }, | |
| config=get_langfuse_langchain_config(), | |
| ) | |
| if related_market.id != market.id and result.near_perfect_correlation: | |
| related_agent_market = OmenAgentMarket.from_data_model(related_market) | |
| correlated_markets.append( | |
| CorrelatedMarketPair( | |
| main_market=market, | |
| related_market=related_agent_market, | |
| correlation=result.correlation, | |
| ) | |
| ) | |
| return correlated_markets | |
| @observe() | |
| def get_correlated_markets(self, market: AgentMarket) -> list[CorrelatedMarketPair]: | |
| # We try to find similar, open markets which point to the same outcome. | |
| correlated_markets = [] | |
| related = self.pinecone_handler.find_nearest_questions_with_threshold( | |
| limit=10, text=market.question | |
| ) | |
| omen_markets = self.subgraph_handler.get_omen_binary_markets( | |
| limit=len(related), | |
| id_in=[i.market_address.lower() for i in related], | |
| resolved=False, | |
| ) | |
| # Note that negative correlation is hard - e.g. for the US presidential election, markets on each candidate are not seen as -100% correlated. | |
| for related_market in omen_markets: | |
| result: Correlation = self.chain.invoke( | |
| { | |
| "main_market_question": market.question, | |
| "related_market_question": related_market.question, | |
| }, | |
| config=get_langfuse_langchain_config(), | |
| ) | |
| if related_market.id != market.id and result.near_perfect_correlation: | |
| related_agent_market = OmenAgentMarket.from_data_model(related_market) | |
| correlated_markets.append( | |
| CorrelatedMarketPair( | |
| main_market=market, | |
| related_market=related_agent_market, | |
| correlation=result.correlation, | |
| ) | |
| ) | |
| return correlated_markets |
| from prediction_market_agent.utils import APIKeys | ||
|
|
||
|
|
||
| class DeployableArbitrageAgent(DeployableTraderAgent): |
There was a problem hiding this comment.
Any reason you're subclassing DeployableTraderAgent instead of DeployableAgent? Not sure which is better, just asking
There was a problem hiding this comment.
I'm doing so because other trading agents (KnownOutcome, TTA and even coinflip) are subclassing the DeployableTraderAgent - as I understand,it does some langfuse magic and checks for some balances, which doesn't hurt.
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- prediction_market_agent/agents/arbitrage_agent/data_models.py (1 hunks)
- prediction_market_agent/agents/arbitrage_agent/deploy.py (1 hunks)
- prediction_market_agent/run_agent.py (3 hunks)
- tests/agents/arbitrage_agent/test_arbitrage_agent.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/agents/arbitrage_agent/test_arbitrage_agent.py
🧰 Additional context used
🔇 Additional comments (9)
prediction_market_agent/agents/arbitrage_agent/data_models.py (4)
1-14: LGTM: Well-structured data modelsThe imports and class definitions are appropriate and well-structured. The
CorrelationandCorrelatedMarketPairclasses extend Pydantic'sBaseModel, which is a good practice for data validation and serialization.
16-17: LGTM: Clear and concise__repr__methodThe
__repr__method provides a clear and informative string representation of theCorrelatedMarketPairinstance, showing the questions for both the main and related markets.
19-37: Remove LLM prompt reference and consider adding reasoningThe
potential_profit_per_bet_unitmethod is well-implemented and handles edge cases correctly. However, there are two points to address:
As mentioned in a previous review, remove the reference to the LLM prompt in the comment on line 28.
Consider adding a
reasoningfield to this method, as suggested in a past review comment. This would help with debugging from langfuse traces.Update the comment on line 28:
- # Negative correlations are not yet supported by the current LLM prompt, hence not handling those for now. + # Negative correlations are not yet supported, hence not handling those for now.Consider adding a
reasoningfield to provide context for the calculated profit:reasoning = f"Potential profit calculated based on p_yes={p_yes:.2f} and p_no={p_no:.2f}" return (1.0 / total_probability) - 1.0, reasoningThis change would require updating the return type and adjusting any code that calls this method.
39-46: LGTM: Correct implementation ofmarket_to_bet_yesThe
market_to_bet_yesmethod correctly selects the market with the lower "YES" probability, which is the appropriate logic for determining where to place a "YES" bet in an arbitrage scenario.prediction_market_agent/run_agent.py (3)
15-17: LGTM: Import statement for DeployableArbitrageAgent.The import statement for
DeployableArbitrageAgentis correctly added and follows the existing import style in the file.
80-80: LGTM: Addition of 'arbitrage' to RunnableAgent enum.The
arbitragemember is correctly added to theRunnableAgentenum, following the existing pattern and maintaining the current structure.
Line range hint
1-124: Summary: Successful integration of DeployableArbitrageAgentThe changes in this file successfully integrate the new
DeployableArbitrageAgentinto the existing agent framework. The modifications are consistent with the PR objectives and follow the established patterns in the file. The new agent is properly imported, added to theRunnableAgentenum, and included in theRUNNABLE_AGENTSdictionary.These changes enable the arbitrage agent to be recognized and executed within the system, allowing for YES/NO or NO/YES trades between correlated markets as described in the PR objectives.
No existing functionality appears to be modified or removed, minimizing the risk of unintended side effects. However, as with any new feature, thorough testing is recommended to ensure the arbitrage agent functions correctly within the broader system.
prediction_market_agent/agents/arbitrage_agent/deploy.py (2)
1-47: LGTM: Imports and class definition look good.The imports are comprehensive and well-organized. The class definition extends
DeployableTraderAgentand provides a clear docstring explaining its purpose.
107-138:⚠️ Potential issueFix arguments passed to
self.chain.invokeIn the
get_correlated_marketsmethod, you're passingmarketandrelated_marketobjects toself.chain.invoke, whereas the chain expects strings containing the market questions. Update the code as follows:- "main_market_question": market, - "related_market_question": related_market, + "main_market_question": market.question, + "related_market_question": related_market.question,This change ensures that the correct data is passed to the language model for correlation analysis.
Likely invalid or redundant comment.
| @computed_field # type: ignore[prop-decorator] | ||
| @property | ||
| def market_to_bet_no(self) -> AgentMarket: | ||
| return ( | ||
| self.main_market | ||
| if self.main_market.current_p_yes > self.related_market.current_p_yes | ||
| else self.related_market | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring for clarity, but current implementation is correct
The current implementation of market_to_bet_no is functionally correct, as it selects the market with the higher "YES" probability for a "NO" bet. However, for consistency and clarity, consider refactoring to directly compare "NO" probabilities.
While the current implementation is correct, you might want to consider the following refactor for improved readability:
def market_to_bet_no(self) -> AgentMarket:
return (
self.main_market
- if self.main_market.current_p_yes > self.related_market.current_p_yes
+ if self.main_market.current_p_no >= self.related_market.current_p_no
else self.related_market
)This change makes the logic more intuitive by directly comparing "NO" probabilities, which aligns with the method's name and purpose.
📝 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.
| @computed_field # type: ignore[prop-decorator] | |
| @property | |
| def market_to_bet_no(self) -> AgentMarket: | |
| return ( | |
| self.main_market | |
| if self.main_market.current_p_yes > self.related_market.current_p_yes | |
| else self.related_market | |
| ) | |
| @computed_field # type: ignore[prop-decorator] | |
| @property | |
| def market_to_bet_no(self) -> AgentMarket: | |
| return ( | |
| self.main_market | |
| if self.main_market.current_p_no >= self.related_market.current_p_no | |
| else self.related_market | |
| ) |
| def split_bet_amount_between_yes_and_no( | ||
| self, total_bet_amount: float | ||
| ) -> t.Tuple[float, float]: | ||
| """Splits total bet amount following equations below: | ||
| A1/p1 = A2/p2 (same profit regardless of outcome resolution) | ||
| A1 + A2 = total bet amount | ||
| """ | ||
| amount_to_bet_yes = ( | ||
| total_bet_amount | ||
| * self.market_to_bet_yes.current_p_yes | ||
| / ( | ||
| self.market_to_bet_yes.current_p_yes | ||
| + self.market_to_bet_no.current_p_no | ||
| ) | ||
| ) | ||
| amount_to_bet_no = total_bet_amount - amount_to_bet_yes | ||
| return amount_to_bet_yes, amount_to_bet_no |
There was a problem hiding this comment.
Add check for division by zero
The split_bet_amount_between_yes_and_no method correctly implements the formula for splitting the bet amount. However, it's important to handle the case where the denominator could be zero to prevent potential runtime errors.
Add a check to ensure the denominator is not zero before performing the division:
+ denominator = (
+ self.market_to_bet_yes.current_p_yes
+ + self.market_to_bet_no.current_p_no
+ )
+ if denominator == 0:
+ raise ValueError("Sum of probabilities is zero, cannot split bet amount")
amount_to_bet_yes = (
total_bet_amount
* self.market_to_bet_yes.current_p_yes
- / (
- self.market_to_bet_yes.current_p_yes
- + self.market_to_bet_no.current_p_no
- )
+ / denominator
)This change will prevent division by zero errors and provide a clear error message when the bet amount cannot be split due to zero probabilities.
The rest of the implementation looks good and correctly calculates the split bet amounts.
📝 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 split_bet_amount_between_yes_and_no( | |
| self, total_bet_amount: float | |
| ) -> t.Tuple[float, float]: | |
| """Splits total bet amount following equations below: | |
| A1/p1 = A2/p2 (same profit regardless of outcome resolution) | |
| A1 + A2 = total bet amount | |
| """ | |
| amount_to_bet_yes = ( | |
| total_bet_amount | |
| * self.market_to_bet_yes.current_p_yes | |
| / ( | |
| self.market_to_bet_yes.current_p_yes | |
| + self.market_to_bet_no.current_p_no | |
| ) | |
| ) | |
| amount_to_bet_no = total_bet_amount - amount_to_bet_yes | |
| return amount_to_bet_yes, amount_to_bet_no | |
| def split_bet_amount_between_yes_and_no( | |
| self, total_bet_amount: float | |
| ) -> t.Tuple[float, float]: | |
| """Splits total bet amount following equations below: | |
| A1/p1 = A2/p2 (same profit regardless of outcome resolution) | |
| A1 + A2 = total bet amount | |
| """ | |
| denominator = ( | |
| self.market_to_bet_yes.current_p_yes | |
| + self.market_to_bet_no.current_p_no | |
| ) | |
| if denominator == 0: | |
| raise ValueError("Sum of probabilities is zero, cannot split bet amount") | |
| amount_to_bet_yes = ( | |
| total_bet_amount | |
| * self.market_to_bet_yes.current_p_yes | |
| / denominator | |
| ) | |
| amount_to_bet_no = total_bet_amount - amount_to_bet_yes | |
| return amount_to_bet_yes, amount_to_bet_no |
| @@ -98,6 +102,7 @@ class RunnableAgent(str, Enum): | |||
| RunnableAgent.ofv_challenger: OFVChallengerAgent, | |||
| RunnableAgent.prophet_o1preview: DeployablePredictionProphetGPTo1PreviewAgent, | |||
| RunnableAgent.prophet_o1mini: DeployablePredictionProphetGPTo1MiniAgent, | |||
| RunnableAgent.arbitrage: DeployableArbitrageAgent, | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Detected: Missing Import Statement for DeployableArbitrageAgent
The DeployableArbitrageAgent has been added to the RUNNABLE_AGENTS dictionary in prediction_market_agent/run_agent.py, but the corresponding import statement is missing. This will prevent the agent from being recognized and cause runtime errors.
- Location:
prediction_market_agent/run_agent.py(Line 105)
Please add the following import statement to ensure proper integration:
from prediction_market_agent.agents.arbitrage_agent.deploy import DeployableArbitrageAgent🔗 Analysis chain
LGTM: Addition of DeployableArbitrageAgent to RUNNABLE_AGENTS dictionary.
The DeployableArbitrageAgent is correctly mapped to RunnableAgent.arbitrage in the RUNNABLE_AGENTS dictionary, following the existing pattern.
To ensure the DeployableArbitrageAgent is properly integrated, please run the following verification script:
This script will help verify that the DeployableArbitrageAgent is properly integrated into the system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of DeployableArbitrageAgent
# Test 1: Check if DeployableArbitrageAgent is imported correctly
echo "Checking import statement:"
rg --type python "from prediction_market_agent.agents.arbitrage_agent.deploy import DeployableArbitrageAgent" prediction_market_agent/run_agent.py
# Test 2: Verify RunnableAgent enum update
echo "Verifying RunnableAgent enum update:"
rg --type python "arbitrage = \"arbitrage\"" prediction_market_agent/run_agent.py
# Test 3: Confirm RUNNABLE_AGENTS dictionary update
echo "Confirming RUNNABLE_AGENTS dictionary update:"
rg --type python "RunnableAgent.arbitrage: DeployableArbitrageAgent" prediction_market_agent/run_agent.py
# Test 4: Check if DeployableArbitrageAgent is implemented correctly
echo "Checking DeployableArbitrageAgent implementation:"
rg --type python "class DeployableArbitrageAgent" prediction_market_agent/agents/arbitrage_agent/deploy.py
Length of output: 927
Script:
#!/bin/bash
# Description: Verify the integration of DeployableArbitrageAgent
# Test 1: Check if DeployableArbitrageAgent is imported correctly
echo "Checking import statement:"
rg 'from prediction_market_agent\.agents\.arbitrage_agent\.deploy import DeployableArbitrageAgent' $(fd --type f --extension py)
# Test 2: Verify RunnableAgent enum update
echo "Verifying RunnableAgent enum update:"
rg 'arbitrage\s*=\s*"arbitrage"' prediction_market_agent/run_agent.py
# Test 3: Confirm RUNNABLE_AGENTS dictionary update
echo "Confirming RUNNABLE_AGENTS dictionary update:"
rg 'RunnableAgent\.arbitrage\s*:\s*DeployableArbitrageAgent' prediction_market_agent/run_agent.py
# Test 4: Check if DeployableArbitrageAgent is implemented correctly
echo "Checking DeployableArbitrageAgent implementation:"
rg 'class DeployableArbitrageAgent' prediction_market_agent/agents/arbitrage_agent/deploy.py
Length of output: 6864
| @observe() | ||
| def build_trades_for_correlated_markets( | ||
| self, pair: CorrelatedMarketPair | ||
| ) -> list[Trade]: | ||
| market_to_bet_yes, market_to_bet_no = pair.main_market, pair.related_market | ||
| total_amount: BetAmount = pair.main_market.get_tiny_bet_amount() | ||
| # Split between main_market and related_market | ||
| amount_yes, amount_no = pair.split_bet_amount_between_yes_and_no( | ||
| total_amount.amount | ||
| ) | ||
| trades = [ | ||
| Trade( | ||
| trade_type=TradeType.BUY, | ||
| outcome=True, | ||
| amount=TokenAmount( | ||
| amount=amount_yes, currency=market_to_bet_yes.currency | ||
| ), | ||
| ), | ||
| Trade( | ||
| trade_type=TradeType.BUY, | ||
| outcome=False, | ||
| amount=TokenAmount( | ||
| amount=amount_no, currency=market_to_bet_no.currency | ||
| ), | ||
| ), | ||
| ] | ||
| logger.info(f"Placing arbitrage trades {trades}") | ||
| return trades |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the bet amount configurable.
The build_trades_for_correlated_markets method currently uses a fixed "tiny bet amount" for trades. To improve flexibility and allow for different trading strategies, consider making this bet amount configurable. You could add a parameter to the method or use a class attribute that can be set during initialization or runtime.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- prediction_market_agent/agents/arbitrage_agent/deploy.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
prediction_market_agent/agents/arbitrage_agent/deploy.py (1)
125-131:⚠️ Potential issueFix arguments passed to
self.chain.invoke.The
self.chain.invokecall is passingmarketandrelated_marketobjects directly, but it should be passing theirquestionattributes instead.Apply this diff to fix the issue:
result: Correlation = self.chain.invoke( { - "main_market_question": market, - "related_market_question": related_market, + "main_market_question": market.question, + "related_market_question": related_market.question, }, config=get_langfuse_langchain_config(), )This change ensures that the correct data (the market questions) is passed to the language model for correlation analysis.
Likely invalid or redundant comment.
| def run(self, market_type: MarketType) -> None: | ||
| if market_type != MarketType.OMEN: | ||
| raise RuntimeError( | ||
| "Can arbitrage only on Omen since related markets embeddings available only for Omen markets." | ||
| ) | ||
| self.subgraph_handler = OmenSubgraphHandler() | ||
| self.pinecone_handler = PineconeHandler() | ||
| self.chain = self._build_chain() | ||
| super().run(market_type=market_type) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for initialization of handlers and chain.
The run method initializes important components (subgraph_handler, pinecone_handler, and chain) without any error handling. Consider wrapping these initializations in a try-except block to gracefully handle any potential errors during setup.
Example implementation:
def run(self, market_type: MarketType) -> None:
if market_type != MarketType.OMEN:
raise RuntimeError(
"Can arbitrage only on Omen since related markets embeddings available only for Omen markets."
)
try:
self.subgraph_handler = OmenSubgraphHandler()
self.pinecone_handler = PineconeHandler()
self.chain = self._build_chain()
except Exception as e:
logger.error(f"Error initializing arbitrage agent: {str(e)}")
raise
super().run(market_type=market_type)This change would improve the robustness of the initialization process and provide more informative error messages if something goes wrong.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/db/pinecone_handler.py (1)
113-117: Consider making the cutoff period configurableCurrently, the
update_marketsmethod uses a hardcoded value of seven days for thecreated_afterparameter. Making this value configurable would enhance the flexibility of the method, allowing it to accommodate different time frames as needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- prediction_market_agent/agents/arbitrage_agent/deploy.py (1 hunks)
- prediction_market_agent/agents/think_thoroughly_agent/deploy.py (1 hunks)
- prediction_market_agent/agents/think_thoroughly_agent/think_thoroughly_agent.py (0 hunks)
- prediction_market_agent/db/pinecone_handler.py (3 hunks)
- tests/agents/arbitrage_agent/test_arbitrage_agent.py (1 hunks)
💤 Files with no reviewable changes (1)
- prediction_market_agent/agents/think_thoroughly_agent/think_thoroughly_agent.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/agents/arbitrage_agent/test_arbitrage_agent.py
🧰 Additional context used
🔇 Additional comments (5)
prediction_market_agent/agents/think_thoroughly_agent/deploy.py (1)
33-33: Approve change, but verify impact and update documentationThe modification to use
self.agent.pinecone_handler.update_markets()instead ofself.agent.update_markets()appears to be a significant architectural change in how market updates are handled. This change likely improves performance or functionality by leveraging Pinecone's capabilities.Please verify the following:
- The impact of this change on the overall system performance and behavior.
- Consistency of this change across the codebase.
Run the following script to check for any remaining direct calls to
update_markets()on the agent:Consider updating the class or method documentation to reflect this architectural change, explaining why the pinecone_handler is now responsible for updating markets.
✅ Verification successful
Verification Successful: All
update_markets()Calls UpdatedThe search confirmed that there are no remaining direct calls to
self.agent.update_markets()in the codebase, ensuring that all updates now utilizeself.agent.pinecone_handler.update_markets().
- Documentation Update: Ensure that the class or method documentation is updated to reflect this architectural change, explaining the role of
pinecone_handlerin managing market updates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining direct calls to update_markets() on the agent # Expect: No results, indicating all calls have been updated to use pinecone_handler rg --type python "self\.agent\.update_markets\(\)"Length of output: 83
Script:
#!/bin/bash # Description: Retry searching for any remaining direct calls to update_markets() on the agent using a glob pattern for Python files # Expect: No results, indicating all calls have been updated to use pinecone_handler rg "self\.agent\.update_markets\(\)" --glob "*.py"Length of output: 52
prediction_market_agent/agents/arbitrage_agent/deploy.py (3)
81-96: Consider making the model name configurable.The
_build_chainmethod uses a hardcoded model name "gpt-4o" from the class attribute. To improve flexibility and allow for easier updates or testing with different models, consider making it a configurable parameter, either through the constructor or a configuration file.This change has already been suggested in a previous comment for the class attributes. If implemented, you can update this method to use the configurable
self.modelattribute.
143-170: LGTM: Well-implemented trade building for correlated markets.The
build_trades_for_correlated_marketsmethod is implemented correctly. It properly splits the bet amount between YES and NO outcomes for the correlated market pair and creates the appropriate Trade objects. The logging of trades is a good practice for debugging and monitoring.
1-186: Overall approval with suggestions for improvementsThe
DeployableArbitrageAgentclass is well-structured and implements a sound strategy for arbitrage in prediction markets. Here's a summary of the main points from the review:
- Consider making
modelandtotal_trade_amountconfigurable parameters.- Add error handling for initialization steps in the
runmethod.- Update the
get_marketsmethod signature or add a docstring to clarify its behavior.- Implement actual market analysis in
answer_binary_marketor add a TODO comment.- Add error handling for chain invocation in
calculate_correlation_between_markets.- Improve error handling and fix arguments in
get_correlated_markets.- Consider utilizing or removing unused parameters in
build_trades.The code follows good practices like logging and using decorators for observability. With the suggested improvements, particularly in error handling and configurability, this implementation of the arbitrage agent will be more robust and flexible.
prediction_market_agent/db/pinecone_handler.py (1)
2-2: LGTM!The added imports (
datetime,DatetimeUTC, andutcnow) are appropriate and necessary for the new functionality.Also applies to: 17-17
| model = "gpt-4o" | ||
| # trade amount will be divided between correlated markets. | ||
| total_trade_amount = BetAmount(amount=0.1, currency=OmenAgentMarket.currency) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making model and total_trade_amount configurable.
The model and total_trade_amount are currently hardcoded. To improve flexibility and allow for easier updates or testing with different configurations, consider making these configurable parameters. You could add them as constructor parameters with default values:
def __init__(self, model: str = "gpt-4o", total_trade_amount: float = 0.1):
self.model = model
self.total_trade_amount = BetAmount(amount=total_trade_amount, currency=OmenAgentMarket.currency)This change would allow users to easily adjust these parameters when instantiating the agent.
| def get_markets( | ||
| self, | ||
| market_type: MarketType, | ||
| limit: int = MAX_AVAILABLE_MARKETS, | ||
| sort_by: SortBy = SortBy.CLOSING_SOONEST, | ||
| filter_by: FilterBy = FilterBy.OPEN, | ||
| ) -> t.Sequence[AgentMarket]: | ||
| return super().get_markets( | ||
| market_type=market_type, | ||
| limit=limit, | ||
| sort_by=SortBy.HIGHEST_LIQUIDITY, | ||
| # Fetching most liquid markets since more likely they will have related markets | ||
| filter_by=FilterBy.OPEN, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update method signature or add docstring to reflect actual behavior.
The get_markets method ignores the sort_by and filter_by parameters, always using SortBy.HIGHEST_LIQUIDITY and FilterBy.OPEN. While this is intentional, it might be confusing for users of the method. Consider either:
- Updating the method signature to remove unused parameters, or
- Adding a docstring explaining that these parameters are ignored and why.
Option 2 (recommended):
def get_markets(
self,
market_type: MarketType,
limit: int = MAX_AVAILABLE_MARKETS,
sort_by: SortBy = SortBy.CLOSING_SOONEST,
filter_by: FilterBy = FilterBy.OPEN,
) -> t.Sequence[AgentMarket]:
"""
Get markets for arbitrage.
Note: This method always uses SortBy.HIGHEST_LIQUIDITY and FilterBy.OPEN,
regardless of the provided sort_by and filter_by parameters. This is to
ensure we fetch the most liquid markets, which are more likely to have
related markets for arbitrage opportunities.
"""
return super().get_markets(
market_type=market_type,
limit=limit,
sort_by=SortBy.HIGHEST_LIQUIDITY,
filter_by=FilterBy.OPEN,
)This change would improve the clarity of the method's behavior.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (2)
prediction_market_agent/agents/arbitrage_agent/data_models.py (1)
16-17: Consider improving the readability of the__repr__methodThe current implementation provides good information, but its readability could be enhanced. Consider using f-string formatting for better structure:
def __repr__(self) -> str: return ( f"CorrelatedMarketPair(\n" f" main_market: {self.main_market.question}\n" f" related_market: {self.related_market.question}\n" f" potential_profit: {self.potential_profit_per_bet_unit}\n" f")" )This format will make the output more readable, especially when dealing with long market questions.
prediction_market_agent/agents/arbitrage_agent/deploy.py (1)
144-171: LGTM: Trade building logic is sound. Consider adding error handling.The
build_trades_for_correlated_marketsmethod correctly implements the logic for building trades on correlated markets. It appropriately splits the bet amount between YES and NO trades.Minor suggestion: Consider adding error handling for potential issues with bet amount splitting or trade creation. For example:
@observe() def build_trades_for_correlated_markets( self, pair: CorrelatedMarketPair ) -> list[Trade]: try: market_to_bet_yes, market_to_bet_no = pair.main_market, pair.related_market amount_yes, amount_no = pair.split_bet_amount_between_yes_and_no( self.total_trade_amount.amount ) trades = [ Trade( trade_type=TradeType.BUY, outcome=True, amount=TokenAmount( amount=amount_yes, currency=market_to_bet_yes.currency ), ), Trade( trade_type=TradeType.BUY, outcome=False, amount=TokenAmount( amount=amount_no, currency=market_to_bet_no.currency ), ), ] logger.info(f"Placing arbitrage trades {trades}") return trades except Exception as e: logger.error(f"Error building trades for correlated markets: {str(e)}") return []This change would make the method more robust to potential errors during trade creation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prediction_market_agent/agents/arbitrage_agent/data_models.py (1 hunks)
- prediction_market_agent/agents/arbitrage_agent/deploy.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
prediction_market_agent/agents/arbitrage_agent/data_models.py (7)
7-9: LGTM:Correlationclass is well-definedThe
Correlationclass is correctly implemented using Pydantic'sBaseModel. It captures the necessary information about correlation with appropriate data types.
19-37: Remove LLM prompt reference and approve implementationThe
potential_profit_per_bet_unitproperty is well-implemented and correctly handles edge cases. However, as per a previous review comment, it's advisable to remove the reference to the LLM prompt in the comment.Please update the comment on line 28:
- # Negative correlations are not yet supported by the current LLM prompt, hence not handling those for now. + # Negative correlations are not yet supported, hence not handling those for now.The rest of the implementation looks good and correctly calculates the potential profit.
39-46: LGTM:market_to_bet_yesproperty is correctly implementedThe
market_to_bet_yesproperty correctly selects the market with the lower YES probability for placing a YES bet. This implementation aligns with the arbitrage strategy of betting YES on the market with the lower YES probability.
48-55: LGTM:market_to_bet_noproperty is correctly implementedThe
market_to_bet_noproperty is correctly implemented. It selects the market with the higher YES probability for placing a NO bet, which is the correct arbitrage strategy.Previous review comments suggesting to change this to compare
current_p_nowere incorrect. In an arbitrage scenario, you want to bet NO on the market with the higher YES probability (lower NO probability) to maximize profit potential.The current implementation is optimal and should be kept as is.
12-73: Overall assessment ofCorrelatedMarketPairclassThe
CorrelatedMarketPairclass is well-structured and implements the necessary logic for handling correlated market pairs in an arbitrage context. The computed properties and methods provide a clear and logical approach to determining bet placements and calculating potential profits.With the suggested improvements (enhancing the
__repr__method, removing the LLM prompt reference, and adding a division by zero check), the class will be more robust, maintainable, and less prone to runtime errors.The core logic, particularly in the
market_to_bet_yesandmarket_to_bet_noproperties, is correct and aligns with arbitrage strategies. Great job on the implementation!
1-73: Overall assessment ofdata_models.pyThe
data_models.pyfile successfully implements the necessary data models for the arbitrage agent. Both theCorrelationandCorrelatedMarketPairclasses are well-structured and provide the required functionality.Key points:
- The
Correlationclass is simple and effective.- The
CorrelatedMarketPairclass implements complex logic correctly, particularly for market selection and profit calculation.- Suggested improvements include enhancing readability, removing unnecessary comments, and adding error handling.
With the proposed changes, this file will serve as a robust foundation for the arbitrage agent implementation. Great work on creating a clear and logical structure for handling correlated markets!
57-73:⚠️ Potential issueAdd check for division by zero
The
split_bet_amount_between_yes_and_nomethod correctly implements the formula for splitting the bet amount. However, it's important to handle the case where the denominator could be zero to prevent potential runtime errors.Add a check to ensure the denominator is not zero before performing the division:
+ denominator = ( + self.market_to_bet_yes.current_p_yes + + self.market_to_bet_no.current_p_no + ) + if denominator == 0: + raise ValueError("Sum of probabilities is zero, cannot split bet amount") amount_to_bet_yes = ( total_bet_amount * self.market_to_bet_yes.current_p_yes - / ( - self.market_to_bet_yes.current_p_yes - + self.market_to_bet_no.current_p_no - ) + / denominator )This change will prevent division by zero errors and provide a clear error message when the bet amount cannot be split due to zero probabilities.
prediction_market_agent/agents/arbitrage_agent/deploy.py (2)
1-43: LGTM: Import statements are well-organized and relevant.The import statements are logically organized and include all necessary modules for the arbitrage agent functionality. There are no apparent unused imports, and the separation between standard library, third-party, and project-specific imports is clear.
1-187: Overall assessment: Good implementation with room for improvements.The
DeployableArbitrageAgentclass provides a solid foundation for arbitrage trading on correlated markets. The implementation includes methods for finding correlated markets, calculating correlations, and building trades. However, there are several areas where the code could be improved:
- Error handling: Add try-except blocks in methods that interact with external services or perform complex operations.
- Configurability: Make attributes like
model,total_trade_amount, andtemperaturemore configurable to increase flexibility.- Unused parameters: Address unused parameters in methods like
build_tradesandget_markets.- Documentation: Add more inline comments and docstrings to explain the purpose and behavior of complex methods.
Addressing these points will enhance the robustness, flexibility, and maintainability of the arbitrage agent implementation.
| class DeployableArbitrageAgent(DeployableTraderAgent): | ||
| """Agent that places mirror bets on Omen for (quasi) risk-neutral profit.""" | ||
|
|
||
| model = "gpt-4o" | ||
| # trade amount will be divided between correlated markets. | ||
| total_trade_amount = BetAmount(amount=0.1, currency=OmenAgentMarket.currency) | ||
| bet_on_n_markets_per_run = 5 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making class attributes more configurable.
-
The
modelattribute is hardcoded to "gpt-4o". Consider making this configurable to allow for easier testing with different models or updates in the future. -
The
total_trade_amountis set to a fixed value of 0.1. Consider making this configurable, possibly through the constructor, to allow for different trading strategies or risk levels. -
The
bet_on_n_markets_per_runattribute is defined but not used in the visible code. Either utilize this attribute in thebuild_tradesmethod or remove it if it's not needed.
| def run(self, market_type: MarketType) -> None: | ||
| if market_type != MarketType.OMEN: | ||
| raise RuntimeError( | ||
| "Can arbitrage only on Omen since related markets embeddings available only for Omen markets." | ||
| ) | ||
| self.subgraph_handler = OmenSubgraphHandler() | ||
| self.pinecone_handler = PineconeHandler() | ||
| self.chain = self._build_chain() | ||
| self.pinecone_handler.update_markets() | ||
| super().run(market_type=market_type) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for initialization steps.
The run method initializes important components (subgraph_handler, pinecone_handler, and chain) without any error handling. Consider wrapping these initializations in a try-except block to gracefully handle any potential errors during setup. For example:
def run(self, market_type: MarketType) -> None:
if market_type != MarketType.OMEN:
raise RuntimeError(
"Can arbitrage only on Omen since related markets embeddings available only for Omen markets."
)
try:
self.subgraph_handler = OmenSubgraphHandler()
self.pinecone_handler = PineconeHandler()
self.chain = self._build_chain()
self.pinecone_handler.update_markets()
except Exception as e:
logger.error(f"Error initializing arbitrage agent: {str(e)}")
raise
super().run(market_type=market_type)This change would improve the robustness of the initialization process and provide more informative error messages if something goes wrong.
| def get_markets( | ||
| self, | ||
| market_type: MarketType, | ||
| limit: int = MAX_AVAILABLE_MARKETS, | ||
| sort_by: SortBy = SortBy.CLOSING_SOONEST, | ||
| filter_by: FilterBy = FilterBy.OPEN, | ||
| ) -> t.Sequence[AgentMarket]: | ||
| return super().get_markets( | ||
| market_type=market_type, | ||
| limit=50, | ||
| sort_by=SortBy.HIGHEST_LIQUIDITY, | ||
| # Fetching most liquid markets since more likely they will have related markets | ||
| filter_by=FilterBy.OPEN, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve flexibility and clarity of the get_markets method.
-
The method ignores the provided
sort_byandfilter_byparameters, always usingSortBy.HIGHEST_LIQUIDITYandFilterBy.OPEN. Consider adding a comment explaining this behavior or updating the method signature to remove unused parameters. -
The method uses a hardcoded limit of 50 instead of the provided
limitparameter. Consider using the providedlimitor explain why a fixed limit is necessary.
Example implementation:
def get_markets(
self,
market_type: MarketType,
limit: int = MAX_AVAILABLE_MARKETS,
sort_by: SortBy = SortBy.CLOSING_SOONEST,
filter_by: FilterBy = FilterBy.OPEN,
) -> t.Sequence[AgentMarket]:
"""
Get markets for arbitrage.
Note: This method always uses SortBy.HIGHEST_LIQUIDITY and FilterBy.OPEN
to fetch the most liquid markets, which are more likely to have related
markets for arbitrage opportunities.
"""
return super().get_markets(
market_type=market_type,
limit=limit, # Use the provided limit
sort_by=SortBy.HIGHEST_LIQUIDITY,
filter_by=FilterBy.OPEN,
)This change would improve the clarity and flexibility of the method.
| def answer_binary_market(self, market: AgentMarket) -> ProbabilisticAnswer | None: | ||
| return ProbabilisticAnswer(p_yes=Probability(0.5), confidence=1.0) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement actual market analysis or add a TODO comment.
The answer_binary_market method currently returns a fixed 50% probability with full confidence for all markets. This doesn't provide any real market analysis. If this is a placeholder, consider adding a TODO comment explaining the intended future implementation. If not, implement actual market analysis logic.
Example:
def answer_binary_market(self, market: AgentMarket) -> ProbabilisticAnswer | None:
# TODO: Implement actual market analysis logic
# For now, returning a default 50% probability with full confidence
return ProbabilisticAnswer(p_yes=Probability(0.5), confidence=1.0)| def _build_chain(self) -> RunnableSerializable[t.Any, t.Any]: | ||
| llm = ChatOpenAI( | ||
| temperature=0, | ||
| model=self.model, | ||
| api_key=APIKeys().openai_api_key_secretstr_v1, | ||
| ) | ||
|
|
||
| parser = PydanticOutputParser(pydantic_object=Correlation) | ||
| prompt = PromptTemplate( | ||
| template=prompt_template, | ||
| input_variables=["main_market_question", "related_market_question"], | ||
| partial_variables={"format_instructions": parser.get_format_instructions()}, | ||
| ) | ||
|
|
||
| chain = prompt | llm | parser | ||
| return chain |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the temperature configurable.
The _build_chain method uses a hardcoded temperature of 0 for the ChatOpenAI model. To improve flexibility and allow for different levels of randomness in the model's output, consider making the temperature a configurable parameter. You could add it as a class attribute or a parameter to the method.
Example:
def _build_chain(self, temperature: float = 0) -> RunnableSerializable[t.Any, t.Any]:
llm = ChatOpenAI(
temperature=temperature,
model=self.model,
api_key=APIKeys().openai_api_key_secretstr_v1,
)
# ... rest of the methodThis change would allow for easier experimentation with different temperature settings without modifying the code.
| @observe() | ||
| def calculate_correlation_between_markets( | ||
| self, market: AgentMarket, related_market: AgentMarket | ||
| ) -> Correlation: | ||
| correlation: Correlation = self.chain.invoke( | ||
| { | ||
| "main_market_question": market.question, | ||
| "related_market_question": related_market.question, | ||
| } | ||
| ) | ||
| return correlation |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for chain invocation.
The calculate_correlation_between_markets method invokes the language model chain without any error handling. Consider adding a try-except block to catch and handle potential exceptions from the API call. This will improve the robustness of the method and prevent potential crashes due to network issues or API failures.
Example implementation:
@observe()
def calculate_correlation_between_markets(
self, market: AgentMarket, related_market: AgentMarket
) -> Correlation:
try:
correlation: Correlation = self.chain.invoke(
{
"main_market_question": market.question,
"related_market_question": related_market.question,
}
)
return correlation
except Exception as e:
logger.error(f"Error calculating correlation: {str(e)}")
# You might want to return a default correlation or re-raise the exception
raiseThis change would make the method more robust to potential failures in the chain invocation.
| @observe() | ||
| def get_correlated_markets(self, market: AgentMarket) -> list[CorrelatedMarketPair]: | ||
| # We try to find similar, open markets which point to the same outcome. | ||
| correlated_markets = [] | ||
| related = self.pinecone_handler.find_nearest_questions_with_threshold( | ||
| limit=10, text=market.question | ||
| ) | ||
|
|
||
| omen_markets = self.subgraph_handler.get_omen_binary_markets( | ||
| limit=len(related), | ||
| id_in=[i.market_address.lower() for i in related], | ||
| resolved=False, | ||
| ) | ||
| omen_markets = [m for m in omen_markets if m.id != market.id] | ||
| # Note that negative correlation is hard - e.g. for the US presidential election, markets on each candidate are not seen as -100% correlated. | ||
| for related_market in omen_markets: | ||
| result: Correlation = self.chain.invoke( | ||
| { | ||
| "main_market_question": market, | ||
| "related_market_question": related_market, | ||
| }, | ||
| config=get_langfuse_langchain_config(), | ||
| ) | ||
| if result.near_perfect_correlation: | ||
| related_agent_market = OmenAgentMarket.from_data_model(related_market) | ||
| correlated_markets.append( | ||
| CorrelatedMarketPair( | ||
| main_market=market, | ||
| related_market=related_agent_market, | ||
| ) | ||
| ) | ||
| return correlated_markets |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and fix arguments in get_correlated_markets.
- Add error handling for external API calls to improve robustness.
- Fix the arguments passed to
self.chain.invoke.
Here's an example of how you can implement these changes:
@observe()
def get_correlated_markets(self, market: AgentMarket) -> list[CorrelatedMarketPair]:
correlated_markets = []
try:
related = self.pinecone_handler.find_nearest_questions_with_threshold(
limit=10, text=market.question
)
omen_markets = self.subgraph_handler.get_omen_binary_markets(
limit=len(related),
id_in=[i.market_address.lower() for i in related],
resolved=False,
)
omen_markets = [m for m in omen_markets if m.id != market.id]
for related_market in omen_markets:
try:
result: Correlation = self.chain.invoke(
{
"main_market_question": market.question,
"related_market_question": related_market.question,
},
config=get_langfuse_langchain_config(),
)
if result.near_perfect_correlation:
related_agent_market = OmenAgentMarket.from_data_model(related_market)
correlated_markets.append(
CorrelatedMarketPair(
main_market=market,
related_market=related_agent_market,
)
)
except Exception as e:
logger.error(f"Error calculating correlation: {str(e)}")
continue
except Exception as e:
logger.error(f"Error fetching related markets: {str(e)}")
return correlated_marketsThese changes will make the method more robust to potential failures in the external API calls and fix the argument passing to self.chain.invoke.
| @observe() | ||
| def build_trades( | ||
| self, | ||
| market: AgentMarket, | ||
| answer: ProbabilisticAnswer, | ||
| existing_position: Position | None, | ||
| ) -> list[Trade]: | ||
| trades = [] | ||
| correlated_markets = self.get_correlated_markets(market=market) | ||
| for pair in correlated_markets: | ||
| if pair.potential_profit_per_bet_unit > 0: | ||
| trades_for_pair = self.build_trades_for_correlated_markets(pair) | ||
| trades.extend(trades_for_pair) | ||
|
|
||
| return trades |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider utilizing or removing unused parameters in build_trades.
The build_trades method currently ignores the answer and existing_position parameters. If these parameters are not needed for the arbitrage strategy, consider removing them to avoid confusion. If they could be useful, consider incorporating them into your trade building logic.
If the parameters are not needed:
@observe()
def build_trades(
self,
market: AgentMarket,
) -> list[Trade]:
trades = []
correlated_markets = self.get_correlated_markets(market=market)
for pair in correlated_markets:
if pair.potential_profit_per_bet_unit > 0:
trades_for_pair = self.build_trades_for_correlated_markets(pair)
trades.extend(trades_for_pair)
return tradesIf you want to utilize these parameters, consider adjusting the trade amount based on the answer's confidence or the existing position.
The overall logic of the method for building trades based on correlated markets with potential profit is correct.
-> Added Arbitrage agent for placing YES/NO or NO/YES trades between correlated markets
-> Added test
Some examples of correlated markets + expected profit