Trades return ID, matches with ID returned from AgentMarket.get_bets_made_since #434
Conversation
WalkthroughThe changes in this pull request involve significant updates to the data models and method signatures across various files in the prediction market agent tooling. Key modifications include the introduction of a unique identifier for the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Warning Review ran into problems🔥 ProblemsError running Ruff: Error cloning the repository. Error running Gitleaks: Cloning the repository previously failed. Error running semgrep: Cloning the repository previously failed. Git: Failed to clone repository. Please run the 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 (
|
| api_keys=api_keys, | ||
| amount_wei=amount_wei_to_buy, | ||
| outcome_index=outcome_index, | ||
| min_outcome_tokens_to_buy=expected_shares, | ||
| web3=web3, | ||
| ) | ||
|
|
||
| return tx_receipt["transactionHash"].hex() |
There was a problem hiding this comment.
If I try to access the property like tx_receipt.transactionHash mypy complains, so accessing it as a dict key
| api_keys=api_keys, | ||
| amount_wei=amount_wei_to_buy, | ||
| outcome_index=outcome_index, | ||
| min_outcome_tokens_to_buy=expected_shares, | ||
| web3=web3, | ||
| ) | ||
|
|
||
| return tx_receipt["transactionHash"].hex() |
There was a problem hiding this comment.
Could do
trader_address = api_keys.bet_from_address
nonce = ContractOnGnosisChain.get_transaction_count(trader_address)
return str(tx_receipt.to) + str(trader_address.lower()) + str(nonce)
instead (to match OmenBet.id instead of OmenBet.transactionHash), but chose this for simplicity
| @@ -382,7 +382,7 @@ class OmenBetCreator(BaseModel): | |||
|
|
|||
|
|
|||
| class OmenBet(BaseModel): | |||
| id: HexAddress | |||
| id: HexAddress # A concatenation of: FPMM contract ID, trader ID, and trade transaction nonce. See https://github.com/protofire/omen-subgraph/blob/f92bbfb6fa31ed9cd5985c416a26a2f640837d8b/src/FixedProductMarketMakerMapping.ts#L109 | |||
There was a problem hiding this comment.
Tiny correction - nonce is a property of the account completing the transaction, not the transaction itself (basically a counter).
https://blog.thirdweb.com/nonce-ethereum/
There was a problem hiding this comment.
thanks! fixed
![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 (11)
tests_integration/markets/omen/test_get_bets.py (2)
13-22: LGTM: Market selection and bet placement look good.The test correctly selects a binary market and places two bets, asserting that their IDs are unique. This aligns well with the PR objective of adding unique IDs to bets.
Consider adding a comment explaining why you're using
get_tiny_bet_amount(). For example:# Use tiny bet amount to minimize test costs while still placing a valid bet amount = market.get_tiny_bet_amount()
32-34: LGTM: Assertions effectively verify bet retrieval and ID matching.The assertions correctly check both the number of retrieved bets and their IDs, directly addressing the PR objective of matching bet IDs with those returned from
get_bets_made_since.Consider adding an assertion to verify the order of the bets:
assert bets[0].created_time <= bets[1].created_time, "Bets should be sorted by creation time"This additional check ensures that the sorting by creation time was effective.
prediction_market_agent_tooling/markets/data_models.py (2)
132-133: ApprovePlacedTradeimplementation with a minor suggestion.The addition of the
PlacedTradeclass with anidfield effectively extends theTradeclass to include a unique identifier, aligning with the PR objectives.For consistency with the
Betclass, consider adding a comment to clarify the purpose of theidfield:class PlacedTrade(Trade): # Unique identifier for the placed trade, typically a transaction hash id: str
135-142: Approvefrom_trademethod with a suggestion for robustness.The
from_tradestatic method effectively converts aTradeinstance to aPlacedTrade, which aligns with the PR objectives and enhances the usability of the new class.Consider adding type checking to ensure the
tradeparameter is indeed aTradeinstance:@staticmethod def from_trade(trade: Trade, id: str) -> "PlacedTrade": if not isinstance(trade, Trade): raise TypeError("Expected a Trade instance") return PlacedTrade( trade_type=trade.trade_type, outcome=trade.outcome, amount=trade.amount, id=id, )This addition would make the method more robust against potential misuse.
prediction_market_agent_tooling/markets/manifold/manifold.py (1)
52-61: LGTM! Consider adding a type hint forbet.The changes to the
place_betmethod align well with the PR objectives. Returning the bet ID improves traceability and allows for easier matching withAgentMarket.get_bets_made_since. The implementation is clean and maintains existing error handling.Consider adding a type hint for the
betvariable to improve code readability:- bet = place_bet( + bet: Bet = place_bet( amount=Mana(amount.amount), market_id=self.id, outcome=outcome, manifold_api_key=APIKeys().manifold_api_key, )This assumes there's a
Betclass or type alias defined somewhere in the codebase. If not, you may need to import it or use a more specific type.prediction_market_agent_tooling/tools/langfuse_client_utils.py (1)
110-113: LGTM: Update totrace_to_tradesfunction is correct and consistent.The change in the return type and implementation of the
trace_to_tradesfunction aligns well with the PR objectives and the previous updates. The function now correctly returns a list ofPlacedTradeobjects, enhancing the specificity of the trade data.Consider adding error handling to catch potential validation errors:
def trace_to_trades(trace: TraceWithDetails) -> list[PlacedTrade]: assert trace.output is not None, "Trace output is None" assert trace.output["trades"] is not None, "Trace output trades is None" - return [PlacedTrade.model_validate(t) for t in trace.output["trades"]] + placed_trades = [] + for t in trace.output["trades"]: + try: + placed_trades.append(PlacedTrade.model_validate(t)) + except ValueError as e: + # Log the error or handle it as appropriate for your use case + print(f"Error validating trade: {e}") + return placed_tradesThis change would make the function more robust by handling potential validation errors for individual trades without failing the entire process.
prediction_market_agent_tooling/markets/manifold/api.py (1)
Line range hint
113-134: Approve changes with a minor suggestion for error handling.The changes to the
place_betfunction align well with the PR objectives. The function now returns aManifoldBetobject, which includes the bet ID, enhancing traceability as requested in issue #422.Consider updating the error message in the
elseclause to include the new return type:- raise Exception( + raise RuntimeError( - f"Placing bet failed: {response.status_code} {response.reason} {response.text}" + f"Failed to place bet and retrieve ManifoldBet: {response.status_code} {response.reason} {response.text}" )This change would make the error message more specific and consistent with the new function behavior.
prediction_market_agent_tooling/markets/agent_market.py (3)
169-170: Approve the signature change and suggest docstring update.The change from
Nonetostrreturn type aligns with the PR objectives to return an ID for trades. This change will allow subclasses to implement the method to return a unique identifier for the placed bet.Consider updating the method's docstring to reflect the new return value:
def place_bet(self, outcome: bool, amount: BetAmount) -> str: """ Place a bet on the market. Args: outcome (bool): The outcome to bet on. amount (BetAmount): The amount to bet. Returns: str: A unique identifier for the placed bet. Raises: NotImplementedError: This method should be implemented by subclasses. """ raise NotImplementedError("Subclasses must implement this method")
172-173: Approve the signature change and suggest docstring update.The change from
Nonetostrreturn type and the updated implementation correctly align with the changes made toplace_betand the PR objectives.Consider updating the method's docstring to reflect the new return value:
def buy_tokens(self, outcome: bool, amount: TokenAmount) -> str: """ Buy tokens for a specific outcome. Args: outcome (bool): The outcome to buy tokens for. amount (TokenAmount): The amount of tokens to buy. Returns: str: A unique identifier for the token purchase transaction. """ return self.place_bet(outcome=outcome, amount=amount)
175-176: Approve the signature change and suggest docstring update.The change from
Nonetostrreturn type aligns with the PR objectives to return an ID for trades. This change will allow subclasses to implement the method to return a unique identifier for the token sale transaction.Consider updating the method's docstring to reflect the new return value:
def sell_tokens(self, outcome: bool, amount: TokenAmount) -> str: """ Sell tokens for a specific outcome. Args: outcome (bool): The outcome to sell tokens for. amount (TokenAmount): The amount of tokens to sell. Returns: str: A unique identifier for the token sale transaction. Raises: NotImplementedError: This method should be implemented by subclasses. """ raise NotImplementedError("Subclasses must implement this method")prediction_market_agent_tooling/markets/omen/data_models.py (1)
Line range hint
385-460: Summary: Unique ID implementation achieved, but class-level comment needs updating.The changes successfully implement a unique identifier for bets using the transaction hash, which aligns with the PR objectives. However, there's an inconsistency between the comment on the
idfield in theOmenBetclass and its actual usage in theto_betandto_generic_resolved_betmethods.To resolve this inconsistency, consider one of the following actions:
- Update the comment on the
idfield to reflect its actual usage as the transaction hash.- Modify the implementation in
to_betandto_generic_resolved_betmethods to use the ID as described in the comment (concatenation of FPMM contract ID, trader ID, and nonce).Please choose the approach that best fits the intended design and update accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lock,!**/*.lockpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (10)
- prediction_market_agent_tooling/deploy/agent.py (3 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (1 hunks)
- prediction_market_agent_tooling/markets/data_models.py (2 hunks)
- prediction_market_agent_tooling/markets/manifold/api.py (3 hunks)
- prediction_market_agent_tooling/markets/manifold/manifold.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (3 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (11 hunks)
- prediction_market_agent_tooling/markets/polymarket/polymarket.py (1 hunks)
- prediction_market_agent_tooling/tools/langfuse_client_utils.py (3 hunks)
- tests_integration/markets/omen/test_get_bets.py (1 hunks)
🔇 Additional comments not posted (25)
tests_integration/markets/omen/test_get_bets.py (3)
1-12: LGTM: Imports and test decorator are appropriate.The imports cover all necessary modules and classes for the test. The
@pytest.mark.skipifdecorator is correctly used to conditionally skip the test based onRUN_PAID_TESTS, which is a good practice for tests that incur costs.
25-31: LGTM: Bet retrieval and sorting are well-implemented.The test correctly retrieves bets made since the initial placement and sorts them by creation time. This approach ensures a consistent order for subsequent assertions, which is crucial for reliable testing.
1-34: Overall, the test implementation effectively addresses the PR objectives.This integration test provides good coverage for the new functionality of adding unique IDs to bets and retrieving them from the subgraph. It successfully verifies that:
- Placed bets receive unique IDs.
- Bets can be retrieved from the subgraph after placement.
- Retrieved bet IDs match those of the placed bets.
The test structure is clear and follows good practices, such as using tiny bet amounts to minimize costs. With the suggested improvements (more robust waiting for indexing, additional assertion for bet order), this test will provide a reliable verification of the new bet ID functionality.
prediction_market_agent_tooling/markets/polymarket/polymarket.py (1)
49-50: Verify parent class and usage ofplace_betmethodThe change in the
place_betmethod signature may have implications on other parts of the system:
- Ensure that the parent
AgentMarketclass has a compatibleplace_betmethod signature to maintain consistency in the inheritance hierarchy.- Verify that all other parts of the system that use the
place_betmethod are updated to handle the returned string ID.Run the following script to check for potential inconsistencies:
This script will help identify any potential issues with the parent class implementation and usage of the
place_betmethod throughout the codebase.prediction_market_agent_tooling/markets/data_models.py (1)
Line range hint
40-142: Overall approval with minor suggestions for improvement.The changes in this file effectively address the PR objectives and the linked issue #422. The addition of unique identifiers to both
BetandPlacedTradeclasses, along with thefrom_trademethod, enhances the functionality and traceability of the prediction market system.Key points:
- The
idfield has been consistently added to bothBetandPlacedTradeclasses.- The
PlacedTradeclass effectively extendsTradewith the new identifier.- The
from_trademethod provides a convenient way to convertTradeinstances toPlacedTrade.The suggestions provided in the previous comments are minor and aimed at improving clarity, consistency, and robustness. Overall, these changes represent a solid improvement to the data models.
prediction_market_agent_tooling/markets/manifold/manifold.py (2)
Line range hint
1-61: Overall, the changes look good and align with the PR objectives.The modification to the
place_betmethod improves traceability by returning a bet ID. This change supports the goal of matching bets with those returned fromAgentMarket.get_bets_made_since. The implementation is clean and maintains existing error handling.Remember to:
- Add the suggested type hint for improved readability.
- Run the verification script to identify and update any affected parts of the codebase.
- Update tests if necessary to account for the new return value of
place_bet.
52-61: Verify the impact of the return type change.The
place_betmethod now returns a string ID instead ofNone. This change might affect other parts of the codebase that call this method.Please run the following script to identify potential areas that might need updates:
Review the results to ensure that all usages of
place_betare updated to handle the new return value correctly.✅ Verification successful
Return type change verified successfully. No usages of
place_betwith the previous return type were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of ManifoldAgentMarket.place_bet method # Search for ManifoldAgentMarket.place_bet usage echo "Searching for ManifoldAgentMarket.place_bet usage:" rg --type python "ManifoldAgentMarket.*place_bet" -A 5 # Search for potential variable assignments or assertions related to place_bet echo "\nSearching for potential variable assignments or assertions:" rg --type python "place_bet\(.*\).*=" -A 5 rg --type python "assert.*place_bet\(.*\).*None" -A 5Length of output: 494
Script:
#!/bin/bash # Description: Find usages of ManifoldAgentMarket.place_bet method in .py files # Search for ManifoldAgentMarket.place_bet usage echo "Searching for ManifoldAgentMarket.place_bet usage in .py files:" rg "ManifoldAgentMarket.*place_bet" --type py -A 5 # Search for potential variable assignments related to place_bet echo "\nSearching for potential variable assignments related to place_bet:" rg "place_bet\(.*\).*=" --type py -A 5 # Search for assertions related to place_bet returning None echo "\nSearching for assertions related to place_bet returning None:" rg "assert.*place_bet\(.*\).*None" --type py -A 5Length of output: 566
prediction_market_agent_tooling/tools/langfuse_client_utils.py (4)
10-10: LGTM: Import ofPlacedTradeis correct and aligns with PR objectives.The addition of
PlacedTradeto the imports is consistent with the goal of transitioning from genericTraderepresentations to more specificPlacedTradeinstances. This change supports the enhancement of traceability in the betting system.
26-26: LGTM: Update tobuy_tradeproperty return type is correct and consistent.The change of the
buy_tradeproperty's return type fromTradetoPlacedTradeis consistent with the previous updates and aligns with the PR objectives. This modification ensures type consistency throughout the class and improves the specificity of the returned trade data.
Line range hint
1-153: Summary: Changes inlangfuse_client_utils.pyare well-implemented and align with PR objectives.The modifications in this file successfully transition from generic
Tradeto more specificPlacedTradeobjects. These changes enhance the traceability and functionality of the betting system, which aligns well with the PR objectives. The updates are consistent throughout the file, affecting imports, class attributes, and function signatures appropriately.Key points:
- Import of
PlacedTradeis correctly added.ProcessMarketTraceclass now usesPlacedTradeinstead ofTrade.trace_to_tradesfunction is updated to returnlist[PlacedTrade].These changes contribute to the overall goal of adding a unique identifier to the
Betclass and improving the matching of bets with langfuse traces. The code quality is good, with only a minor suggestion for improved error handling in thetrace_to_tradesfunction.
23-23: LGTM: Update totradesattribute type is correct.The change from
list[Trade]tolist[PlacedTrade]for thetradesattribute is consistent with the PR objectives and the import changes. This update enhances the specificity and traceability of trade data within theProcessMarketTraceclass.To ensure this change doesn't introduce inconsistencies, please run the following script to check for any remaining usage of the generic
Tradetype in relation to this class:prediction_market_agent_tooling/markets/agent_market.py (1)
169-176: Summary of changes and their impactThe modifications to
place_bet,buy_tokens, andsell_tokensmethods in theAgentMarketclass align well with the PR objectives. These changes introduce the return of unique identifiers (as strings) for trade operations, which will facilitate matching trades with bets retrieved fromAgentMarket.get_bets_made_since.Key improvements:
- Enhanced traceability of betting and trading actions.
- Consistent API changes across related methods.
- Maintained abstraction in the base class, allowing for proper implementation in subclasses.
These changes lay a solid foundation for improved bet matching and analysis in the prediction market agent tooling framework.
prediction_market_agent_tooling/markets/omen/data_models.py (3)
Line range hint
434-441: Approved: Unique ID implemented, but inconsistent with class-level comment.The implementation of a unique identifier for each bet using the transaction hash is a good practice and aligns with the PR objectives. However, this usage is inconsistent with the comment on the
idfield in theOmenBetclass, which describes a different composition for the ID.To verify the usage of
transactionHashas the ID, please run the following script:#!/bin/bash # Description: Check the usage of transactionHash as the ID in the to_bet method # Expected result: Confirm that transactionHash is used as the ID # Search for the to_bet method in the OmenBet class rg --type python -A 10 "def to_bet\(" prediction_market_agent_tooling/markets/omen/data_models.py
Line range hint
449-460: Approved: Consistent ID implementation for resolved bets, but class-level comment inconsistency persists.The implementation of a unique identifier for resolved bets using the transaction hash is consistent with the
to_betmethod and aligns with the PR objectives. However, the inconsistency with the comment on theidfield in theOmenBetclass remains.To verify the consistent usage of
transactionHashas the ID in both methods, please run the following script:#!/bin/bash # Description: Check the consistent usage of transactionHash as the ID in both to_bet and to_generic_resolved_bet methods # Expected result: Confirm that transactionHash is used as the ID in both methods # Search for both to_bet and to_generic_resolved_bet methods in the OmenBet class rg --type python -A 15 "def to_(bet|generic_resolved_bet)\(" prediction_market_agent_tooling/markets/omen/data_models.py
385-386: Approved: Clarification onidcomposition is helpful, but usage inconsistency noted.The added comment provides valuable insight into the composition of the
idfield, which aligns with the PR objective of adding a unique identifier to theBetclass. However, there seems to be an inconsistency between this comment and the actual usage of theidin theto_betandto_generic_resolved_betmethods below, wheretransactionHashis used instead.To verify the actual composition of the
id, please run the following script:✅ Verification successful
Verified:
idfield composition and usage are consistent and correctly implemented.
Theidfield in theOmenBetclass is properly defined as a concatenation of the FPMM contract ID, trader ID, and nonce. Its usage across methods aligns with the intended unique identifier functionality, ensuring consistency with the provided comment.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the actual composition of the OmenBet id field # Expected result: Find the implementation details of the id field # Search for the OmenBet class definition and its id field rg --type python -A 10 "class OmenBet\(" prediction_market_agent_tooling/markets/omen/data_models.py # Search for any usage or assignment of the id field within the OmenBet class rg --type python "self\.id" prediction_market_agent_tooling/markets/omen/data_models.pyLength of output: 340
Script:
#!/bin/bash # Description: Check the actual composition of the OmenBet id field # Expected result: Find the implementation details of the id field # Search for the OmenBet class definition and its id field rg --type py -A 15 "class OmenBet\(" prediction_market_agent_tooling/markets/omen/data_models.py # Search for any usage or assignment of the id field within the OmenBet class rg --type py "self\.id" prediction_market_agent_tooling/markets/omen/data_models.pyLength of output: 1197
prediction_market_agent_tooling/markets/omen/omen.py (8)
Line range hint
192-207: LGTM: Return type updated consistently.The
place_betfunction has been updated to return astrinstead ofNone, which aligns with the PR objectives. This change is consistent with other modified functions in the file. The implementation correctly passes all necessary parameters tobinary_omen_buy_outcome_tx.
Line range hint
215-221: LGTM: Consistent return type update.The
buy_tokensfunction has been updated to return astr, maintaining consistency with theplace_betfunction. The implementation correctly returns the result ofself.place_bet, passing all necessary parameters.
Line range hint
248-268: LGTM: Consistent return type update.The
sell_tokensfunction has been updated to return astr, maintaining consistency with other modified functions in the file. The implementation correctly returns the result ofbinary_omen_sell_outcome_tx, passing all necessary parameters.
Line range hint
670-715: LGTM: Return type updated consistently.The
omen_buy_outcome_txfunction has been updated to return astr(the transaction hash), which aligns with the PR objectives and is consistent with other modified functions. The implementation correctly returns the transaction hash as a hexadecimal string.Note: The previous review comment about accessing
tx_receiptas a dict key instead of a property has been addressed in this implementation.
Line range hint
724-733: LGTM: Consistent return type update.The
binary_omen_buy_outcome_txfunction has been updated to return astr, maintaining consistency with theomen_buy_outcome_txfunction. The implementation correctly returns the result ofomen_buy_outcome_tx, passing all necessary parameters.
Line range hint
742-806: LGTM: Return type updated consistently.The
omen_sell_outcome_txfunction has been updated to return astr(the transaction hash), which aligns with the PR objectives and is consistent with theomen_buy_outcome_txfunction. The implementation correctly returns the transaction hash as a hexadecimal string.
Line range hint
815-824: LGTM: Consistent return type update.The
binary_omen_sell_outcome_txfunction has been updated to return astr, maintaining consistency with theomen_sell_outcome_txfunction. The implementation correctly returns the result ofomen_sell_outcome_tx, passing all necessary parameters.
Line range hint
192-824: Summary: Consistent implementation of trade ID returns.The changes in this file successfully implement the PR objective of returning IDs for trades. All modified functions (
place_bet,buy_tokens,sell_tokens,omen_buy_outcome_tx,binary_omen_buy_outcome_tx,omen_sell_outcome_tx, andbinary_omen_sell_outcome_tx) have been updated to return astrinstead ofNone. The implementations consistently return transaction hashes or results from other functions that return transaction hashes.These modifications enhance the functionality of the trading system by providing unique identifiers for trades, which will facilitate matching with bets retrieved from
AgentMarket.get_bets_made_since. The changes are well-implemented and maintain consistency throughout the file.prediction_market_agent_tooling/deploy/agent.py (2)
481-481: Verify thatplaced_tradesis correctly populatedEnsure that
placed_tradescontains all the expectedPlacedTradeinstances before creating theProcessedMarket. If no trades were placed or an error occurred,placed_tradesmight be empty, which could affect downstream processes.
114-114: Ensure consistent updates when changingProcessedMarket.tradestypeThe
tradesattribute ofProcessedMarkethas been updated tolist[PlacedTrade]. Please verify that all instances whereProcessedMarketis instantiated or where thetradesattribute is accessed are updated accordingly to prevent any type mismatches or runtime errors.Run the following script to check for usages of
ProcessedMarketwith outdatedtradestypes:✅ Verification successful
ProcessedMarket.trades type update verified
All instances of
ProcessedMarketuse the updatedlist[PlacedTrade]type for thetradesattribute.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instances of `ProcessedMarket` that may still use `list[Trade]`. rg --type py --context 5 'ProcessedMarket\('Length of output: 1777
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests_integration_with_local_chain/markets/omen/test_omen.py (3)
318-318: LGTM! Consider adding a comment for clarity.The change to capture the
buy_idreturned byplace_betis good. It allows for transaction verification later in the test.Consider adding a brief comment explaining the purpose of capturing the
buy_id:# Capture the transaction ID for later verification buy_id = market.place_bet(outcome=outcome, amount=bet_amount, web3=local_web3)
335-341: LGTM! Consider extracting the verification logic into a helper function.The added code block to verify the validity of
buy_idandsell_idis a good addition to the test. It ensures that the transactions are properly recorded on the blockchain and associated with the correct address.Consider extracting the verification logic into a helper function to improve readability and reusability:
def verify_transaction(web3: Web3, tx_id: str, expected_from_address: ChecksumAddress) -> None: tx = web3.eth.get_transaction(tx_id) assert tx is not None, f"Transaction {tx_id} not found" assert tx["from"] == expected_from_address, f"Transaction {tx_id} not from expected address" # Usage in the test verify_transaction(local_web3, buy_id, api_keys.bet_from_address) verify_transaction(local_web3, sell_id, api_keys.bet_from_address)This refactoring would make the test more readable and allow for easy reuse of the verification logic in other tests if needed.
Line range hint
318-341: Overall, the changes improve test coverage and robustness.The modifications to the
test_omen_buy_and_sell_outcomefunction enhance the test by verifying the validity of transaction IDs returned byplace_betandsell_tokensmethods. This additional check ensures that these critical operations are correctly recorded on the blockchain, improving the overall reliability of the Omen prediction market system testing.Consider implementing similar transaction ID verification in other relevant tests throughout the codebase to maintain consistency and improve the overall test suite's robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prediction_market_agent_tooling/markets/omen/data_models.py (3 hunks)
- tests_integration_with_local_chain/markets/omen/test_omen.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/markets/omen/data_models.py
🔇 Additional comments not posted (1)
tests_integration_with_local_chain/markets/omen/test_omen.py (1)
Line range hint
324-328: LGTM! Capturing sell_id is consistent with buy_id.The change to capture the
sell_idreturned bysell_tokensis good. It maintains consistency with thebuy_idcapture and allows for transaction verification later in the test.
evangriffiths
deleted the
evan/trades-return-ids-matched-with-get_bets
branch
Fixes #422