Create OmenMarket out of blockchain events #441
Conversation
WalkthroughThe pull request introduces several modifications across multiple files within the prediction market agent tooling. Key changes include updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| language = "en" | ||
| distribution_hint = [ | ||
| omen_outcome_type(xdai_to_wei(xdai_type(0.05))), # 75% for yes | ||
| omen_outcome_type(xdai_to_wei(xdai_type(0.15))), # 25% for now |
There was a problem hiding this comment.
I needed to check if I calculated outcome tokens correctly, so this was a good opportunity to test out market creation with skewed odds: https://presagio.pages.dev/markets?id=0x21f2c0d28c760378282ade8bff1209e22ead59d9
| json.dump(from_subgraph_market.model_dump(), f, indent=2) | ||
|
|
||
| # Compare! | ||
| assert from_created_market == from_subgraph_market |
| outcome_token_marginal_prices = [ | ||
| xDai((total_amount - amount) / total_amount) | ||
| for amount in outcome_token_amounts | ||
| ] |
There was a problem hiding this comment.
I'm unsure if the order here matters or not.
If a market has outcomes [Yes,No] or [No,Yes], would this still work?
There was a problem hiding this comment.
This is just an ordered list of outcome tokens as retrieved from the event. It doesn't care about what outcomes are at these indices. If you switch Yes,No order, it needs to be handled at different places.
There was a problem hiding this comment.
What I guess I'm suggesting here is
-> Fetch the 2 outcomes (Yes, No) from the question.
-> Find the indices of Yes, No in the outcomes.
-> Having the indices, you can create a correctly-ordered list of outcomeTokenMarginalPrices with Yes, No in a known order.
It's also possible to find out downstream the order of outcomes, I just find that doing it here is a good spot.
There was a problem hiding this comment.
I just find that doing it here is a good spot
I disagree here. This here is doing the same thing that subgraph is doing, it's meant to be 1:1 OmenMarket that you'd get from subgraph otherwise (which it is). Subgraph also won't re-order these outcome tokens according to your specific needs.
That's why we have methods like get_outcome_index, so the right way to go about it, if you need outcome tokens for a specific outcome, is something like
x.outcome_token_marginal_prices[x.get_outcome_index("Yes")]
and then you don't need to care about the order of [Yes, No].
There was a problem hiding this comment.
Which we are actually alredy doing for example here
or here
so the codebase is ready for this 👌 (problem would be if we change the order of it now)
|
Looks like a couple of CI tests need fixing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (19)
tests/markets/omen/test_reality.py (1)
8-19: LGTM: Test function is well-structured, but could be more comprehensive.The test function
test_format_and_decode_questioneffectively checks the basic functionality of formatting and parsing a question. It creates aParsedQuestioninstance, formats it, parses it back, and verifies that the result matches the original.However, consider the following suggestions to improve the test coverage:
- Add more test cases with different question types, outcomes, languages, and categories to ensure the functions work correctly for various inputs.
- Include an explicit check for the formatted string's content to verify the formatting logic.
- Consider parameterizing the numeric identifier (currently hardcoded as 2) to test different scenarios.
- Add edge cases, such as empty outcomes or long question strings, to ensure robust handling of various inputs.
Here's an example of how you could expand the test:
import pytest @pytest.mark.parametrize("question_text, outcomes, language, category, identifier", [ ("How are you?", ["Cool", "Not cool"], "en", "life", 2), ("What's the weather?", ["Sunny", "Rainy", "Cloudy"], "es", "weather", 5), ("", ["Yes", "No"], "fr", "general", 10), # Edge case: empty question ("A" * 1000, ["Long", "Short"], "de", "text", 7), # Edge case: very long question ]) def test_format_and_decode_question(question_text, outcomes, language, category, identifier): question = ParsedQuestion( question=question_text, outcomes=outcomes, language=language, category=category, ) formatted = format_realitio_question( question.question, question.outcomes, question.category, question.language, identifier ) # Check the formatted string content assert question_text in formatted assert all(outcome in formatted for outcome in outcomes) assert language in formatted assert category in formatted parsed = parse_realitio_question(formatted, identifier) assert parsed == questionThis expanded test uses
pytest.mark.parametrizeto test multiple scenarios and includes checks for the formatted string's content.scripts/create_market_omen.py (2)
28-28: LGTM: Parameter renamed and default value updated correctlyThe
feeparameter has been appropriately renamed tofee_perc, which better reflects its purpose as a percentage. The default value has been correctly updated to useOMEN_DEFAULT_MARKET_FEE_PERC.Consider adding a type hint to explicitly indicate that
fee_percis a percentage:fee_perc: float = typer.Option(OMEN_DEFAULT_MARKET_FEE_PERC, min=0, max=100)This would provide additional clarity and validation for the input.
Line range hint
1-71: Summary: Changes align with PR objectives and improve code clarityThe modifications in this file contribute to the PR objective of creating OmenMarket out of blockchain events. The renaming of
feetofee_percthroughout the script enhances code clarity by explicitly indicating that the value represents a percentage. This change is consistently applied in imports, function parameters, and function calls.To fully complete this PR:
- Ensure that the
omen_create_market_txfunction signature has been updated to usefee_percinstead offee.- Update any documentation or comments that may reference the old
feeparameter.- Consider adding type hints and value range checks for the
fee_percparameter to prevent potential misuse.As this change affects a core parameter, ensure that all other parts of the system that interact with Omen market creation are updated to use the new
fee_percterminology. This may include:
- Other scripts or functions that call
create_market_omen.py- Any API endpoints that expose market creation functionality
- User-facing documentation or interfaces that allow setting the market fee
Maintaining consistency across the entire system will prevent confusion and potential bugs in the future.
tests_integration/markets/omen/test_reality.py (3)
42-42: LGTM: Updated logging statementThe logging statement has been correctly updated to use
question_event.question_id.hex(). This change is consistent with the variable renaming and maintains the functionality.Consider adding a log message about the entire
question_eventobject, as it might contain other useful information for debugging:logger.info(f"Question event: {question_event}") logger.info(f"Question ID: {question_event.question_id.hex()}")
Line range hint
51-65: Fix inconsistency insubmit_answercallThere's an inconsistency in how
question_idis passed to thesubmit_answermethod.Please update line 51 as follows:
- question_id=question_event.question_id, + question_id=question_event.question_id,The change on lines 64-65 looks good and is consistent with the variable renaming.
Line range hint
32-65: Overall feedback on changesThe renaming of
question_idtoquestion_eventimproves code clarity and suggests that the variable contains more information about the question event. Most changes are consistent with this renaming, which is good.However, there are a few points to consider:
- There's an inconsistency on line 51 that needs to be addressed (as mentioned in a previous comment).
- Consider adding more logging or assertions to take advantage of the additional information in
question_event. This could improve the test's ability to catch potential issues.- It might be helpful to add a comment explaining the structure of
question_eventfor future maintainers.Here's an example of how you could add more logging and a comment:
# question_event contains information about the asked question, # including its ID, content, and metadata logger.info(f"Question event: {question_event}") logger.info(f"Question content: {question_event.question}") logger.info(f"Question ID: {question_event.question_id.hex()}") # Add an assertion to ensure the question content is correct assert question_event.question == "Will GNO be above $1000 in 1 second from now?", "Unexpected question content"These changes would make the test more robust and easier to understand.
tests_integration/markets/omen/test_markets.py (1)
50-68: LGTM: Market creation and conversion are correctly implemented.The market creation and conversion to OmenMarket are well-implemented, using all the previously defined parameters.
Consider using a logging framework instead of a print statement for better control over debug output. You could replace line 67 with:
import logging logging.debug(f"Market created at {from_created_market.url}")This allows for more flexible control over log levels and output destinations.
tests_integration_with_local_chain/markets/omen/test_omen.py (1)
Line range hint
148-164: Ensurecreated_market.market_eventis valid before accessing attributesWhen accessing
created_market.market_event.fixed_product_market_maker_checksummed, please verify thatcreated_market.market_eventis notNoneto prevent potentialAttributeError. Adding a check can enhance the robustness of the test.Consider adding a check like:
assert created_market.market_event is not None, "Market event is None."prediction_market_agent_tooling/tools/contract.py (4)
Line range hint
75-77: Avoid mutable default class attributes to prevent shared state issuesDefining
_cacheas a mutable class attribute can lead to shared state across all instances ofContractBaseClass, causing unintended side effects. It's recommended to usePrivateAttrwith adefault_factoryto ensure each instance has its own cache.Apply this change to modify
_cache:-from pydantic import BaseModel, field_validator +from pydantic import BaseModel, field_validator, PrivateAttr class ContractBaseClass(BaseModel): """ Base class holding the basic requirements and tools used for every contract. """ CHAIN_ID: t.ClassVar[ChainID] CHAIN_RPC_URL: t.ClassVar[str] abi: ABI address: ChecksumAddress _abi_field_validator = field_validator("abi", mode="before")(abi_field_validator) - _cache: dict[str, t.Any] = {} + _cache: dict[str, t.Any] = PrivateAttr(default_factory=dict)
Line range hint
196-206: Ensure robust error handling indeposit_asset_tokenmethodWhile the
deposit_asset_tokenmethod correctly handles asset token approval and deposit, consider adding error handling to manage potential exceptions during the approval or deposit process. This will improve the reliability and user feedback of the method.For example, you might wrap the approval and deposit calls in try-except blocks:
def deposit_asset_token( self, asset_value: Wei, api_keys: APIKeys, web3: Web3 | None = None ) -> TxReceipt: for_address = api_keys.bet_from_address web3 = web3 or self.get_web3() asset_token_contract = self.get_asset_token_contract(web3=web3) try: # Approve vault to withdraw the ERC-20 token from the user. asset_token_contract.approve(api_keys, self.address, asset_value, web3=web3) except Exception as e: raise Exception(f"Token approval failed: {e}") try: # Deposit asset token (ERC-20) and receive shares in the vault. receipt = self.deposit(api_keys, asset_value, for_address, web3=web3) except Exception as e: raise Exception(f"Token deposit failed: {e}") return receipt
Line range hint
246-258: Avoid hardcoding contract addresses for flexibilityHardcoding the contract address in the
DebuggingContractclass limits flexibility and can cause issues when deploying to different environments or networks. Consider making the address configurable via environment variables or configuration files.For example, modify the class to accept the address as an argument:
class DebuggingContract(ContractOnGnosisChain): # Contract ABI taken from the contract source. abi: ABI = abi_field_validator( os.path.join( os.path.dirname(os.path.realpath(__file__)), "../abis/debuggingcontract.abi.json", ) ) - address: ChecksumAddress = Web3.to_checksum_address( - "0x5Aa82E068aE6a6a1C26c42E5a59520a74Cdb8998" - ) + address: ChecksumAddress + def __init__(self, address: ChecksumAddress, **data): + super().__init__(address=address, **data)And instantiate it with the desired address:
debugging_contract = DebuggingContract(address=CONFIG.DEBUGGING_CONTRACT_ADDRESS)
Line range hint
260-278: Ensure consistent method naming conventionsThe methods
getNowandget_nowin theDebuggingContractclass use different naming styles. For consistency and adherence to Python's PEP 8 naming conventions, consider renaminggetNowtoget_now.If
getNowmatches a function name in the smart contract ABI and must be called as such, you can keep the method name but alias it for Python conventions:def getNow( self, web3: Web3 | None = None, ) -> int: now: int = self.call( function_name="getNow", web3=web3, ) return now + def get_now( + self, + web3: Web3 | None = None, + ) -> int: + return self.getNow(web3=web3)Clients of the class can then use
get_nowfor consistency.prediction_market_agent_tooling/markets/omen/omen_contracts.py (4)
Line range hint
258-279: Handle potential empty event logs inprepareCondition.In the
prepareConditionmethod, you are accessingevent_logs[0]without verifying ifevent_logsis not empty. If the event is not emitted, this will raise anIndexError.Add a check to ensure that
event_logscontains the expected event:receipt_tx = self.send( api_keys=api_keys, function_name="prepareCondition", function_params=[ oracle_address, question_id, outcomes_slot_count, ], tx_params=tx_params, web3=web3, ) +if not event_logs: + raise ValueError("No ConditionPreparationEvent found in transaction receipt") event_logs = ( self.get_web3_contract(web3=web3) .events.ConditionPreparation() .process_receipt(receipt_tx) ) cond_event = ConditionPreparationEvent(**event_logs[0]["args"])
432-432: Improve constant naming for clarity.The constant
OMEN_DEFAULT_MARKET_FEE_PERCmight cause confusion. The suffix_PERCsuggests a percentage, but in the code, fees are sometimes treated asWeivalues. Consistency in naming helps prevent misunderstandings.Consider renaming the constant for clarity:
-OMEN_DEFAULT_MARKET_FEE_PERC = 0.02 # 2% fee from the buying shares amount. +OMEN_DEFAULT_MARKET_FEE_PERCENTAGE = 0.02 # 2% fee from the buying shares amount.
Line range hint
453-471: Clarify the unit and type of thefeeparameter.In the
create2FixedProductMarketMakermethod, thefeeparameter is annotated asWei, but the comment indicates it's a percentage where "100% == 1 xDai". This discrepancy can lead to incorrect usage of the function.Adjust the type annotation and clarify the expected value:
def create2FixedProductMarketMaker( self, api_keys: APIKeys, condition_id: HexBytes, initial_funds_wei: Wei, collateral_token_address: ChecksumAddress, - fee: Wei, # This is actually fee in %, 'where 100% == 1 xDai'. + fee: float, # Fee percentage (e.g., 0.02 for 2%) distribution_hint: list[OmenOutcomeToken] | None = None, tx_params: t.Optional[TxParams] = None, web3: Web3 | None = None, ) -> tuple[ OmenFixedProductMarketMakerCreationEvent, FPMMFundingAddedEvent, TxReceipt ]:Ensure that when passing
feeto the contract function, it is converted to the correct unit expected by the smart contract.
567-577: Consider validating inputs inaskQuestion.In the
askQuestionmethod, the parametersquestion,category,outcomes, andlanguageare critical for forming therealitio_question. To prevent issues at runtime, consider adding validation to ensure these inputs meet expected formats and constraints.You could add checks to validate that:
questionis a non-empty string.outcomesis a non-empty list of strings.languageis a supported language code.prediction_market_agent_tooling/markets/omen/omen.py (3)
333-334: Add docstring for the newfrom_created_marketmethodThe newly added
from_created_marketmethod lacks a docstring. Adding a docstring would improve readability and maintainability by explaining the purpose and usage of this method.Consider adding a docstring like:
@staticmethod def from_created_market(model: "CreatedMarket") -> "OmenAgentMarket": """ Create an OmenAgentMarket instance from a CreatedMarket model. :param model: The CreatedMarket model instance. :return: An instance of OmenAgentMarket. """ return OmenAgentMarket.from_data_model(OmenMarket.from_created_market(model))
929-934: Unpack returned tuple with explicit variable namesWhile unpacking the returned tuple from
create2FixedProductMarketMaker, ensure that the variable names accurately represent the returned values. Additionally, consider adding error handling to catch any exceptions during market creation.Verify that the variables
market_event,funding_event, andreceipt_txcorrectly correspond to the returned values.
Line range hint
843-959: Ensure consistent fee units throughout the codeIn line 929,
feeis calculated usingxdai_to_wei, but in line 957,feeis passed directly without conversion. This could lead to inconsistencies if the units are not handled correctly.Confirm whether
feeshould be inWeior a float, and adjust accordingly. For example:fee = xdai_to_wei(xdai_type(fee_perc)) return CreatedMarket( ... - fee=fee, + fee=fee_perc, ... )Ensure that the
feefield inCreatedMarketmatches the expected unit and type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- .gitignore (1 hunks)
- prediction_market_agent_tooling/markets/omen/data_models.py (4 hunks)
- prediction_market_agent_tooling/markets/omen/omen.py (10 hunks)
- prediction_market_agent_tooling/markets/omen/omen_contracts.py (12 hunks)
- prediction_market_agent_tooling/tools/contract.py (2 hunks)
- prediction_market_agent_tooling/tools/web3_utils.py (1 hunks)
- scripts/create_market_omen.py (3 hunks)
- tests/markets/omen/test_reality.py (1 hunks)
- tests/markets/test_betting_strategies.py (0 hunks)
- tests_integration/markets/omen/test_markets.py (1 hunks)
- tests_integration/markets/omen/test_reality.py (4 hunks)
- tests_integration_with_local_chain/markets/omen/test_omen.py (9 hunks)
- tests_integration_with_local_chain/markets/omen/test_reality.py (1 hunks)
💤 Files with no reviewable changes (1)
- tests/markets/test_betting_strategies.py
🧰 Additional context used
📓 Learnings (1)
tests_integration_with_local_chain/markets/omen/test_omen.py (1)
Learnt from: kongzii PR: gnosis/prediction-market-agent-tooling#139 File: tests/markets/omen/test_omen.py:144-154 Timestamp: 2024-03-28T15:05:28.580Z Learning: A GitHub issue was created to track the implementation of the `create_market_fund_market_remove_funding` test function in `test_omen.py` for the Gnosis prediction market agent tooling project. The issue number is 143.
🪛 Ruff
prediction_market_agent_tooling/markets/omen/omen.py
846-846: Do not perform function call
WrappedxDaiContractin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
prediction_market_agent_tooling/markets/omen/omen_contracts.py
189-189: Do not perform function call
build_parent_collection_idin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
🔇 Additional comments (24)
tests/markets/omen/test_reality.py (1)
1-5: LGTM: Imports are appropriate and well-structured.The imported components (
ParsedQuestion,format_realitio_question, andparse_realitio_question) are relevant to the test function and follow good import practices.scripts/create_market_omen.py (2)
15-15: LGTM: Import statement updated correctlyThe import statement has been updated to reflect the renaming of
OMEN_DEFAULT_MARKET_FEEtoOMEN_DEFAULT_MARKET_FEE_PERC. This change is consistent and appropriate.
58-58: LGTM: Function argument updated correctlyThe
feeargument has been correctly renamed tofee_percin theomen_create_market_txfunction call, maintaining consistency with the parameter renaming in themainfunction.To ensure full consistency, let's verify that the
omen_create_market_txfunction signature has been updated accordingly:✅ Verification successful
Action Required: Verify the
omen_create_market_txfunction signature correctlyThe previous verification attempt failed because
pythonwas not recognized as a valid file type. Let's correct the file type and re-run the verification:
LGTM: Function argument and signature updated correctly
The
feeargument has been correctly renamed tofee_percin theomen_create_market_txfunction call, and the function signature has been updated accordingly to reflect this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the signature of omen_create_market_tx function # Test: Search for the function definition rg --type python -A 10 'def omen_create_market_tx'Length of output: 83
Script:
#!/bin/bash # Description: Verify the signature of omen_create_market_tx function # Correcting the file type from 'python' to 'py' rg --type py -A 10 'def omen_create_market_tx'Length of output: 949
tests_integration/markets/omen/test_reality.py (1)
Line range hint
32-41: LGTM: Improved variable namingThe change from
question_idtoquestion_eventis a good improvement. It suggests that the variable contains more information about the question event, not just the ID. This naming is more descriptive and aligns better with the actual content of the variable..gitignore (3)
163-164: LGTM! Good practice for managing test files.These changes improve the management of test-related files in version control:
tests_files/*ignores all files within thetests_filesdirectory, which is useful for excluding test-generated files that don't need to be tracked.!tests_files/.gitkeepensures that the.gitkeepfile is not ignored, allowing thetests_filesdirectory structure to be maintained in the repository even if it's empty.This approach keeps the repository clean while preserving the necessary directory structure for tests.
163-164: Please clarify the relationship between these changes and the PR objectives.While the changes to .gitignore are good practices for managing test files, they seem tangential to the main objective of creating OmenMarket from blockchain events. Could you please clarify:
- Are these changes necessary for the OmenMarket implementation?
- Have there been changes to the test file structure as part of this PR?
- Could these .gitignore changes be related to the CI test failures mentioned in the comments?
Understanding the context will help ensure that these changes are intentional and not inadvertently causing issues with the CI process.
163-164: Ensure no sensitive data intests_filesWhile updating .gitignore is generally safe, it's good practice to verify that the
tests_filesdirectory doesn't contain any sensitive information. Please confirm that no private keys, API tokens, or other confidential data are present in this directory.To check for potential sensitive data, you can run the following command:
If this command returns any results, please review them carefully to ensure no sensitive data is being exposed.
tests_integration/markets/omen/test_markets.py (3)
1-30: LGTM: Imports and test function declaration are well-structured.The imports are comprehensive and appropriate for the test's requirements. The
@pytest.mark.skipifdecorator is correctly used to prevent unintentional execution of paid tests.
33-48: LGTM: Market creation setup is comprehensive and well-documented.The market parameters are clearly defined, including the intentionally skewed distribution hint. The assertion on line 46-48 provides a good safeguard to ensure the distribution hint is set up correctly.
As noted in a previous comment, this skewed distribution was intentionally set up to test market creation with uneven odds.
1-86: Overall, the integration test is well-structured and comprehensive.This test effectively validates the creation of an Omen market and its correspondence with the subgraph data. The implementation covers all necessary steps, from market creation to data verification.
While the test is functional, the suggested improvements would enhance its reliability, debugging capabilities, and error reporting. These include:
- Implementing a more robust subgraph synchronization mechanism.
- Improving debug data management.
- Enhancing the final assertion for better error reporting.
These changes would make the test more resilient and easier to maintain in the long run.
prediction_market_agent_tooling/tools/web3_utils.py (2)
Line range hint
1-344: Summary of changes and suggestionsThe addition of the
get_receipt_block_timestampfunction enhances the utility of this module by providing a way to retrieve block timestamps for transaction receipts. This aligns well with the existing functionality in the file.Key points from the review:
- The new function has been implemented correctly but could benefit from improved error handling, type hinting, and documentation.
- The imports should be verified and potentially reorganized for clarity and maintainability.
Once these suggestions are addressed, the changes will be ready for approval. Great job on expanding the functionality of the
web3_utils.pymodule!
Line range hint
1-27: Verify and organize importsThe new function
get_receipt_block_timestampuses types that are already imported. However, it's a good practice to review and organize the imports to ensure clarity and maintainability.Please run the following script to check the imports:
This script will help you verify the current imports, check for any unused imports, and suggest an organized import structure. Please review the results and make any necessary adjustments to keep the imports clean and well-organized.
tests_integration_with_local_chain/markets/omen/test_omen.py (5)
44-44: LGTM!The addition of
OMEN_DEFAULT_MARKET_FEE_PERCto the import statement is appropriate and ensures that the constant is available for use in the code.
Line range hint
175-192: Ensure validity ofcreated_market.market_eventbefore attribute accessSimilar to the previous comment, ensure that
created_market.market_eventis notNonebefore accessingfixed_product_market_maker_checksummed. This will prevent potential exceptions if the market event is not properly initialized.
272-274: Confirm the availability ofcollateral_token_contractEnsure that
market.get_contract().get_collateral_token_contract(local_web3)returns a valid contract instance. If there's a possibility of it returningNone, consider adding an assertion or handling the case to avoid exceptions.You can verify the existence of the contract with:
assert collateral_token_contract is not None, "Collateral token contract is None."
377-379: Confirm the availability ofcollateral_token_contractSame as the previous comment, ensure that
market.get_contract().get_collateral_token_contract(local_web3)returns a valid contract instance in this context as well.
80-83: Verify the usage offee_percparameter inomen_create_market_txThe inclusion of
fee_perc=OMEN_DEFAULT_MARKET_FEE_PERCin theomen_create_market_txfunction call is correct. Please ensure that theomen_create_market_txfunction accepts thefee_percparameter and handles it appropriately.Run the following script to confirm that
omen_create_market_txsupports thefee_percparameter:prediction_market_agent_tooling/tools/contract.py (3)
9-9: ImportingWeb3Contractis necessary for type hintingThe addition of the import statement for
Web3Contractis appropriate, as it is used for type annotations in theget_web3_contractmethod.
81-83: Addition ofget_web3_contractmethod enhances code reuseThe
get_web3_contractmethod encapsulates the creation of a Web3 contract instance, improving code readability and reusability across the contract classes.
Line range hint
191-194: Methodget_asset_token_balancecorrectly retrieves asset token balanceThe
get_asset_token_balancemethod is properly implemented to obtain the balance of the asset token for a given address, enhancing the functionality ofContractERC4626BaseClass.prediction_market_agent_tooling/markets/omen/data_models.py (1)
380-433: Verify data consistency infrom_created_marketmethodEnsure that all fields mapped from
CreatedMarkettoOmenMarketin thefrom_created_marketmethod are correctly assigned and that the source data is consistent. Consider adding validation checks to ensure that critical fields such as IDs, timestamps, and outcome lists are notNoneand have expected values.prediction_market_agent_tooling/markets/omen/omen_contracts.py (1)
91-92:⚠️ Potential issueDo not call functions in default argument values.
Using
build_parent_collection_id()as a default value for theparent_collection_idparameter inredeemPositionsis not recommended because default arguments are evaluated only once at function definition time. This can lead to unexpected behavior if the function has side effects or relies on mutable data.Apply this diff to fix the issue:
def redeemPositions( self, api_keys: APIKeys, collateral_token_address: HexAddress, condition_id: HexBytes, index_sets: t.List[int], - parent_collection_id: HexStr = build_parent_collection_id(), + parent_collection_id: HexStr = None, web3: Web3 | None = None, ) -> TxReceipt: + if parent_collection_id is None: + parent_collection_id = build_parent_collection_id()Likely invalid or redundant comment.
prediction_market_agent_tooling/markets/omen/omen.py (2)
950-959: Include missing fields inCreatedMarketinitialization if necessaryVerify that all necessary fields are included when creating a
CreatedMarketinstance. If theCreatedMarketclass requires additional fields not provided here, this could lead to errors.Check the
CreatedMarketclass definition to ensure all required fields are supplied.
843-848: Update documentation and downstream code for changed return typeThe return type of
omen_create_market_txhas changed fromChecksumAddresstoCreatedMarket. Ensure that all calls to this function and any dependent code are updated to handle the new return type.Run the following script to find all usages of
omen_create_market_txand check how the return value is used:#!/bin/bash # Description: Find all usages of 'omen_create_market_tx' and check return value handling. rg --type py 'omen_create_market_tx' -A 3 -B 3This will help identify any code that may need to be updated due to the changed return type.
🧰 Tools
🪛 Ruff
846-846: Do not perform function call
WrappedxDaiContractin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
| @@ -20,6 +20,7 @@ def test_ask_question(local_web3: Web3, test_keys: APIKeys) -> None: | |||
| language="en", | |||
| arbitrator=Arbitrator.KLEROS, | |||
| opening=utcnow() + timedelta(minutes=2), | |||
| timeout=timedelta(seconds=5), | |||
There was a problem hiding this comment.
💡 Codebase verification
The askQuestion method does not support a timeout parameter.
Adding a timeout=timedelta(seconds=5) to the askQuestion method call in test_reality.py is inconsistent with the method's current implementation, which does not accept a timeout parameter. This can lead to runtime errors or unexpected behavior during tests.
Recommendations:
- Remove the
timeoutparameter from theaskQuestioncall intest_reality.pyunless the method is updated to handle it. - Update the method definition of
askQuestioninomen_contracts.pyto accept and properly handle thetimeoutparameter if it is necessary for your testing framework. - Ensure consistency across the codebase by verifying that any new parameters added to method calls are supported by their definitions.
🔗 Analysis chain
Consider the implications of the short timeout value.
The addition of a timeout parameter to the askQuestion method is a good practice for controlling the duration of blockchain operations. However, the current value of 5 seconds might be too short for a blockchain transaction to complete, especially in a test environment where network conditions may vary.
Consider the following suggestions:
- Increase the timeout value to a more realistic duration, such as 30 seconds or 1 minute.
- Make the timeout value configurable, allowing it to be adjusted based on the test environment.
Example implementation:
timeout = timedelta(seconds=config.get('OMEN_ASK_QUESTION_TIMEOUT', 30))To ensure this change doesn't affect other parts of the codebase, let's verify the OmenRealitioContract class implementation and its usage:
This will help us understand if the timeout parameter is consistently used across the codebase and if any adjustments are needed in other files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the OmenRealitioContract class definition
echo "OmenRealitioContract class definition:"
rg --type python -A 10 "class OmenRealitioContract"
# Search for other usages of the askQuestion method
echo "\nOther usages of askQuestion method:"
rg --type python "askQuestion\(" --glob '!test_reality.py'
Length of output: 348
Script:
#!/bin/bash
# Search for the OmenRealitioContract class definition
echo "OmenRealitioContract class definition:"
rg "class OmenRealitioContract" --glob "*.py" -A 10
# Search for other usages of askQuestion method excluding test_reality.py
echo "\nOther usages of askQuestion method:"
rg "askQuestion\(" --glob "*.py" --glob "!tests_integration_with_local_chain/markets/omen/test_reality.py"
Length of output: 1771
Script:
#!/bin/bash
# Search for the OmenRealitioContract class definition
echo "OmenRealitioContract class definition:"
rg "class OmenRealitioContract" --glob "*.py" -A 10
# Search for other usages of askQuestion method excluding test_reality.py
echo "\nOther usages of askQuestion method:"
rg "askQuestion\(" --glob "*.py" --glob "!tests_integration/markets/omen/test_reality.py"
Length of output: 1768
Script:
#!/bin/bash
# Search for the OmenRealitioContract class definition
echo "OmenRealitioContract class definition:"
rg "class OmenRealitioContract" --glob "*.py" -A 10
# Search for other usages of askQuestion method excluding all test_reality.py files
echo "\nOther usages of askQuestion method:"
rg "askQuestion\(" --glob "*.py" --glob "!**/test_reality.py"
Length of output: 1627
Script:
#!/bin/bash
# Inspect the askQuestion method definition for the timeout parameter
echo "askQuestion method definition:"
rg "def askQuestion" --glob "*.py" prediction_market_agent_tooling/markets/omen/omen_contracts.py -A 5
# Search all calls to askQuestion to check for the timeout parameter
echo "\nCalls to askQuestion method with timeout parameter:"
rg "askQuestion\(" --glob "*.py" | grep "timeout="
# Check for configuration settings related to OMEN_ASK_QUESTION_TIMEOUT
echo "\nConfiguration settings for askQuestion timeout:"
rg "OMEN_ASK_QUESTION_TIMEOUT" --glob "*.py" .
Length of output: 636
| time.sleep(30) | ||
|
|
||
| # Load from subgraph. | ||
| from_subgraph_market = OmenSubgraphHandler().get_omen_market_by_market_id( | ||
| from_created_market.id | ||
| ) | ||
|
|
||
| # Save because otherwise it's hard to debug | ||
| with open("tests_files/created_market.json", "w") as f: | ||
| json.dump(created_market.model_dump(), f, indent=2) | ||
| with open("tests_files/from_created_market.json", "w") as f: | ||
| json.dump(from_created_market.model_dump(), f, indent=2) | ||
| with open("tests_files/from_subgraph_market.json", "w") as f: | ||
| json.dump(from_subgraph_market.model_dump(), f, indent=2) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving subgraph synchronization and debug data management.
While the current implementation works, there are a couple of areas that could be improved:
-
Subgraph synchronization: The fixed 30-second sleep might not always be sufficient for the subgraph to update. Consider implementing a polling mechanism with a timeout instead.
-
Debug data management: Saving data to files in the test directory is helpful for debugging but might lead to clutter. Consider using a temporary directory or a dedicated debug folder.
Here's a suggestion for implementing a polling mechanism:
import tempfile
from pathlib import Path
def wait_for_subgraph_update(market_id, timeout=60, interval=5):
start_time = time.time()
while time.time() - start_time < timeout:
if OmenSubgraphHandler().get_omen_market_by_market_id(market_id):
return True
time.sleep(interval)
return False
# Replace the sleep with:
if not wait_for_subgraph_update(from_created_market.id):
pytest.fail("Subgraph did not update within the expected timeframe")
# Use a temporary directory for debug files:
with tempfile.TemporaryDirectory() as tmpdir:
debug_dir = Path(tmpdir)
(debug_dir / "created_market.json").write_text(json.dumps(created_market.model_dump(), indent=2))
(debug_dir / "from_created_market.json").write_text(json.dumps(from_created_market.model_dump(), indent=2))
(debug_dir / "from_subgraph_market.json").write_text(json.dumps(from_subgraph_market.model_dump(), indent=2))
print(f"Debug files saved in {tmpdir}")This approach provides more reliable subgraph synchronization and keeps your test directory clean.
| json.dump(from_subgraph_market.model_dump(), f, indent=2) | ||
|
|
||
| # Compare! | ||
| assert from_created_market == from_subgraph_market |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the final assertion for better error reporting.
While the current assertion is correct, it might not provide enough information if the test fails. To make debugging easier, consider using a more detailed assertion that shows the differences between the two objects.
Here's a suggestion for a more informative assertion:
import deepdiff
diff = deepdiff.DeepDiff(from_created_market.model_dump(), from_subgraph_market.model_dump(), ignore_order=True)
assert not diff, f"Differences found between created market and subgraph market:\n{diff.pretty()}"This approach will provide a detailed breakdown of any differences between the two market objects, making it easier to identify and fix any discrepancies.
| def get_receipt_block_timestamp(receipt_tx: TxReceipt, web3: Web3) -> int: | ||
| block_number = receipt_tx["blockNumber"] | ||
| block = web3.eth.get_block(block_number) | ||
| block_timestamp: int = block["timestamp"] | ||
| return block_timestamp |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the get_receipt_block_timestamp function with error handling, type hinting, and documentation.
The function looks good, but could benefit from the following improvements:
- Add error handling for potential exceptions during the Web3 call.
- Include explicit return type hinting.
- Add a docstring to explain the function's purpose and parameters.
Here's a suggested implementation with these improvements:
from web3.exceptions import BlockNotFound
def get_receipt_block_timestamp(receipt_tx: TxReceipt, web3: Web3) -> int:
"""
Retrieve the block timestamp for a given transaction receipt.
Args:
receipt_tx (TxReceipt): The transaction receipt.
web3 (Web3): The Web3 instance to use for blockchain interactions.
Returns:
int: The timestamp of the block containing the transaction.
Raises:
BlockNotFound: If the block associated with the transaction cannot be found.
ValueError: If the block doesn't contain a timestamp.
"""
block_number = receipt_tx["blockNumber"]
try:
block = web3.eth.get_block(block_number)
except BlockNotFound:
raise BlockNotFound(f"Block {block_number} not found")
block_timestamp = block.get("timestamp")
if block_timestamp is None:
raise ValueError(f"Block {block_number} does not contain a timestamp")
return int(block_timestamp)This implementation includes error handling for potential issues, explicit type hinting for the return value, and a comprehensive docstring explaining the function's purpose, parameters, return value, and potential exceptions.
| logger.debug(f"Market created at address: {created_market.market_event}") | ||
| # ToDo - Fix call here (subgraph will not update on localchain). Retrieve data directly from contract. | ||
| market = omen_subgraph_handler.get_omen_market_by_market_id(market_address) | ||
| market = omen_subgraph_handler.get_omen_market_by_market_id( | ||
| created_market.market_event.fixed_product_market_maker_checksummed | ||
| ) |
There was a problem hiding this comment.
Address the TODO: Retrieve data directly from the contract
There's a TODO comment indicating that the subgraph will not update on the local chain and that data should be retrieved directly from the contract. It's recommended to implement this change to ensure the test functions correctly in a local environment.
Do you need assistance in modifying the code to fetch market data directly from the contract? I can help implement this change or open a GitHub issue to track this task.
| cond_event: ConditionPreparationEvent | None = None | ||
| condition_id = conditional_token_contract.getConditionId( | ||
| question_id=question_id, | ||
| question_id=question_event.question_id, | ||
| oracle_address=oracle_contract.address, | ||
| outcomes_slot_count=len(outcomes), | ||
| web3=web3, | ||
| ) | ||
| if not conditional_token_contract.does_condition_exists(condition_id, web3=web3): | ||
| conditional_token_contract.prepareCondition( | ||
| cond_event = conditional_token_contract.prepareCondition( |
There was a problem hiding this comment.
Handle existing condition in omen_create_market_tx
In the omen_create_market_tx function, if the condition already exists, cond_event remains None. Ensure that subsequent code can handle this scenario without errors.
Consider adding a comment or handling to clarify this behavior:
# If the condition already exists, `cond_event` will be None.
if not conditional_token_contract.does_condition_exists(condition_id, web3=web3):
cond_event = conditional_token_contract.prepareCondition(
api_keys=api_keys,
question_id=question_event.question_id,
oracle_address=oracle_contract.address,
outcomes_slot_count=len(outcomes),
web3=web3,
)
# Else, proceed knowing that `cond_event` is None.Ensure that any code that uses cond_event can handle the None value appropriately.
| @@ -751,7 +759,7 @@ | |||
|
|
|||
| market_contract: OmenFixedProductMarketMakerContract = market.get_contract() | |||
| conditional_token_contract = OmenConditionalTokenContract() | |||
| collateral_token_contract = market_contract.get_collateral_token_contract() | |||
| collateral_token_contract = market_contract.get_collateral_token_contract(web3) | |||
There was a problem hiding this comment.
Handle potential None value for web3 parameter
Similar to line 685, ensure that web3 is not None before calling get_collateral_token_contract(web3).
Add a check or initialize web3 if necessary:
collateral_token_contract = market_contract.get_collateral_token_contract(web3)
+if web3 is None:
+ web3 = market_contract.get_web3()Committable suggestion was skipped due to low confidence.
| @@ -674,7 +682,7 @@ | |||
| amount_wei = xdai_to_wei(amount) | |||
|
|
|||
| market_contract: OmenFixedProductMarketMakerContract = market.get_contract() | |||
| collateral_token_contract = market_contract.get_collateral_token_contract() | |||
| collateral_token_contract = market_contract.get_collateral_token_contract(web3) | |||
There was a problem hiding this comment.
Handle potential None value for web3 parameter
The variable web3 may be None, which could lead to errors when calling get_collateral_token_contract(web3). Ensure that web3 is initialized before this call.
Consider adding a check or default initialization:
collateral_token_contract = market_contract.get_collateral_token_contract(web3)
+if web3 is None:
+ web3 = market_contract.get_web3()Alternatively, ensure that web3 is passed correctly when the function is called.
Committable suggestion was skipped due to low confidence.
| finalization_timeout: timedelta = timedelta(days=1), | ||
| fee_perc: float = OMEN_DEFAULT_MARKET_FEE_PERC, | ||
| distribution_hint: list[OmenOutcomeToken] | None = None, | ||
| collateral_token_address: ChecksumAddress = WrappedxDaiContract().address, |
There was a problem hiding this comment.
Avoid function calls in default argument values
Using a function call WrappedxDaiContract() in the default value of the collateral_token_address parameter can lead to unintended behavior because the function is evaluated at function definition time, not at call time. This could cause issues if the function relies on runtime state.
To fix this, set the default value to None and initialize it within the function if it's None:
def omen_create_market_tx(
api_keys: APIKeys,
initial_funds: xDai,
question: str,
closing_time: datetime,
category: str,
language: str,
outcomes: list[str],
auto_deposit: bool,
- finalization_timeout: timedelta = timedelta(days=1),
- fee_perc: float = OMEN_DEFAULT_MARKET_FEE_PERC,
- distribution_hint: list[OmenOutcomeToken] | None = None,
- collateral_token_address: ChecksumAddress = WrappedxDaiContract().address,
web3: Web3 | None = None,
) -> CreatedMarket:
+ finalization_timeout: timedelta = timedelta(days=1),
+ fee_perc: float = OMEN_DEFAULT_MARKET_FEE_PERC,
+ distribution_hint: list[OmenOutcomeToken] | None = None,
+ collateral_token_address: ChecksumAddress | None = None,
+ web3: Web3 | None = None,
):
+ if collateral_token_address is None:
+ collateral_token_address = WrappedxDaiContract().addressThis change ensures that WrappedxDaiContract() is called at runtime when the function is invoked.
[static analysis hint addressed]
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Ruff
846-846: Do not perform function call
WrappedxDaiContractin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
| market_creation_timestamp=get_receipt_block_timestamp(receipt_tx, web3), | ||
| market_event=market_event, | ||
| funding_event=funding_event, | ||
| condition_id=condition_id, | ||
| question_event=question_event, | ||
| condition_event=cond_event, | ||
| initial_funds=initial_funds_wei, | ||
| fee=fee, | ||
| distribution_hint=distribution_hint, | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Missing Proper Handling for cond_event When It Is None
The analysis indicates that cond_event can be None, but there are no conditional checks handling this case. This may lead to potential issues when the CreatedMarket object is used elsewhere.
- File:
prediction_market_agent_tooling/markets/omen/omen.py- Lines: 950-959
🔗 Analysis chain
Ensure all required fields are correctly initialized in CreatedMarket
When returning the CreatedMarket instance, verify that all fields are properly initialized, especially cond_event, which may be None if the condition already exists. This could lead to issues when the CreatedMarket object is used elsewhere.
Run the following script to check where cond_event is used and ensure it can handle None values:
This script searches for all occurrences of cond_event in Python files and provides context to verify proper handling.
Please run the following corrected script to find usages of cond_event and verify None handling:
Please run the following script to verify that cond_event is properly handled when it is None:
Please run the following corrected script to verify that cond_event is properly handled when it is None:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of 'cond_event' and check for None handling.
# Search for 'cond_event' assignments and usages.
rg --type py --py 'cond_event\s*='
rg --type py --py '\bcond_event\b' -A 5 -B 5
Length of output: 6863
Script:
#!/bin/bash
# Description: Corrected script to find usages of 'cond_event' and check for None handling.
# Search for 'cond_event' assignments
rg -t py 'cond_event\s*='
# Search for 'cond_event' usages with context lines
rg -t py '\bcond_event\b' -C 5
Length of output: 4320
Script:
#!/bin/bash
# Description: Check for conditional handling of 'cond_event' being None.
# Search for conditions where 'cond_event' is compared to None
rg -t py 'cond_event\s*is\s*None'
rg -t py 'cond_event\s*!\=\s*None'
rg -t py 'if\s+cond_event\b'
rg -t py '\bcond_event\b\s*\.'
Length of output: 240
Script:
#!/bin/bash
# Description: Corrected script to check conditional handling of 'cond_event' being None.
# Search for conditions where 'cond_event' is compared to None
rg -t py 'cond_event\s*is\s*None'
rg -t py 'cond_event\s*!=\s*None'
# Search for conditional statements involving 'cond_event'
rg -t py 'if\s+cond_event\b'
rg -t py '\bcond_event\b\s*\.'
Length of output: 135
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/markets/omen/test_omen.py (1)
284-296: LGTM: Well-structured test function for calculate_marginal_prices.The new test function
test_calculate_marginal_pricesis well-designed and covers multiple scenarios efficiently using pytest's parametrize decorator. It includes edge cases and provides clear type hints, enhancing code readability and maintainability.Consider adding a test case for an invalid input scenario to ensure proper error handling.
Here's a suggested additional test case:
([100], None), # Invalid input: less than 2 outcome token amountsThis case would test how the
calculate_marginal_pricesfunction handles invalid inputs, ensuring robust error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prediction_market_agent_tooling/markets/omen/data_models.py (4 hunks)
- tests/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 (3)
tests/markets/omen/test_omen.py (3)
9-21: LGTM: New imports added for enhanced functionality.The new imports from the
prediction_market_agent_toolingpackage are appropriate for the added test function and enhance the overall functionality of the test suite.
282-283: LGTM: Improved readability with empty lines.The addition of empty lines enhances the readability of the test file by clearly separating the new test function from the existing ones.
Line range hint
1-296: Summary: Enhancements to the Omen market test suite.The changes to this file improve the test coverage and readability of the Omen market test suite. Key improvements include:
- Addition of new imports to support enhanced functionality.
- Improved code structure with better separation between test functions.
- Implementation of a new test function
test_calculate_marginal_pricesthat efficiently covers multiple scenarios for thecalculate_marginal_pricesfunction.These changes contribute to a more robust and comprehensive test suite for the Omen market functionality.
No description provided.