Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce fees into AgentMarket itself (not just OmenAgentMarket) #506

Merged
merged 7 commits into from
Oct 17, 2024

Conversation

kongzii
Copy link
Contributor

@kongzii kongzii commented Oct 16, 2024

No description provided.

@kongzii kongzii force-pushed the peter/rethink-fees branch 2 times, most recently from 9ef5060 to 4635f0a Compare October 17, 2024 07:53
@kongzii kongzii marked this pull request as ready for review October 17, 2024 08:28
Copy link

coderabbitai bot commented Oct 17, 2024

Walkthrough

The pull request introduces several modifications across multiple files, primarily focusing on the implementation of a new MarketFees class to manage betting fees more effectively. Key changes include updates to method signatures to incorporate the new fee structure, enhancements in error handling and logging, and the introduction of a structured approach to fee calculations in various betting strategies. Additionally, the .gitignore file has been updated to exclude files related to bet_strategy_benchmark, ensuring better organization of the project.

Changes

File Change Summary
.gitignore Added bet_strategy_benchmark* to ignore files and directories related to betting strategy benchmarks.
examples/monitor/match_bets_with_langfuse_traces.py Created an output directory bet_strategy_benchmark for organized results; updated file paths for CSV and markdown exports.
prediction_market_agent_tooling/deploy/betting_strategy.py Updated KellyBettingStrategy methods to handle MarketFees instead of a single fee parameter; adjusted method signatures for clarity.
prediction_market_agent_tooling/jobs/omen/omen_jobs.py Changed fee parameter to fees in OmenJobAgentMarket class methods, indicating multiple fees handling.
prediction_market_agent_tooling/markets/agent_market.py Added fees: MarketFees attribute; introduced handle_legacy_fee model validator for backward compatibility.
prediction_market_agent_tooling/markets/manifold/manifold.py Added fees: MarketFees variable to ManifoldAgentMarket class to manage betting fees.
prediction_market_agent_tooling/markets/market_fees.py Introduced MarketFees class with attributes for fee management and methods for fee calculations.
prediction_market_agent_tooling/markets/metaculus/metaculus.py Added fees: MarketFees variable initialized to MarketFees.get_zero_fees() in MetaculusAgentMarket class.
prediction_market_agent_tooling/markets/omen/omen.py Updated OmenAgentMarket to use MarketFees for fee calculations; modified multiple methods to reference new fee structure.
prediction_market_agent_tooling/markets/omen/omen_contracts.py Updated mergePositions and redeemPositions methods to include default parameter for parent_collection_id.
prediction_market_agent_tooling/markets/polymarket/polymarket.py Added fees: MarketFees variable to PolymarketAgentMarket class to manage fees.
prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py Updated get_kelly_bet_full function to use MarketFees instead of a float fee parameter.
prediction_market_agent_tooling/tools/betting_strategies/market_moving.py Modified get_market_moving_bet and _sanity_check_omen_market_moving_bet functions to utilize MarketFees.
prediction_market_agent_tooling/tools/langfuse_client_utils.py Enhanced error handling and logging in trace_to_omen_agent_market function.
prediction_market_agent_tooling/tools/utils.py Updated calculate_sell_amount_in_collateral function to use MarketFees for fee calculations.
tests/markets/omen/test_omen.py Changed parameter name from fee to fees in test_get_new_p_yes function.
tests/markets/test_betting_strategies.py Updated tests to use MarketFees for fee handling in various functions.
tests/markets/test_markets.py Updated AgentMarket instantiation in tests to include fees parameter.
tests/tools/test_utils.py Modified test functions to use MarketFees instead of a float fee parameter.
tests_integration/markets/omen/test_kelly.py Updated tests to utilize MarketFees for fee calculations and removed hardcoded fee values.

Possibly related PRs

Suggested reviewers

  • evangriffiths

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@@ -226,7 +230,7 @@ def calculate_price_impact_deviation_from_target_price_impact(
bet_amount,
yes_outcome_pool_size,
no_outcome_pool_size,
fee,
MarketFees.get_zero_fees(), # TODO: Use market.fees
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is bunch of TODOs like this, going to do them in #507, to separate fees introduction to Kelly in case of issues.

Will also run the bet benchmark there.

@@ -60,6 +61,7 @@ class AgentMarket(BaseModel):
current_p_yes: Probability
url: str
volume: float | None # Should be in currency of `currency` above.
fees: MarketFees
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leveled the fees to AgentMarket itself, but because Manifold has an absolute, constant fee, it's now a model with absolute and bet_proportion fields.

def handle_legacy_fee(cls, data: dict[str, t.Any]) -> dict[str, t.Any]:
# Backward compatibility for older `AgentMarket` without `fees`.
if "fees" not in data and "fee" in data:
data["fees"] = MarketFees(absolute=0.0, bet_proportion=data["fee"])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixed running of match_bets_with_langfuse script, otherwise there would be 0 matched bets, which is a shame.


class MarketFees(BaseModel):
bet_proportion: float = Field(
..., ge=0.0, lt=1.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fancy way to. validate bet propotion, so I removed it in other places of the code, where MarketFees is now accepted.

@kongzii kongzii changed the title Rethink fees Introduce fees into AgentMarket itself (not just OmenAgentMarket) Oct 17, 2024
@@ -240,7 +244,8 @@ def get_outcome_for_trace(

details.sort(key=lambda x: x["sim_profit"], reverse=True)
pd.DataFrame.from_records(details).to_csv(
f"{agent_name} - {strategy} - all bets.csv", index=False
output_directory / f"{agent_name} - {strategy} - all bets.csv",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, a happy directory tree now 😌

fee = fees.bet_proportion
if fees.absolute > 0:
raise RuntimeError(
f"Kelly works only with bet-proportional fees, but the fees are {fees=}."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had a look at the below formula. Without trying to re-derive, I wonder if, for the numerator, you replace all instances of b * f with b * (1 - f_proportional) - f_absolute.

Not sure about the denominator though

Comment on lines +20 to +33
def total_fee_absolute_value(self, bet_amount: float) -> float:
"""
Returns the total fee in absolute terms, including both proportional and fixed fees.
"""
return self.bet_proportion * bet_amount + self.absolute

def total_fee_relative_value(self, bet_amount: float) -> float:
"""
Returns the total fee relative to the bet amount, including both proportional and fixed fees.
"""
if bet_amount == 0:
return 0.0
total_fee = self.total_fee_absolute_value(bet_amount)
return total_fee / bet_amount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these are unused. Planning to use in later PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total_fee_absolute_value is now 😄 and removed total_fee_relative_value

total_fee = self.total_fee_absolute_value(bet_amount)
return total_fee / bet_amount

def get_bet_size_after_fees(self, bet_amount: float) -> float:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This is a really neat replacement for the fee: float args before. I thought it would be more complicated than this 😅

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🧹 Outside diff range and nitpick comments (21)
prediction_market_agent_tooling/markets/market_fees.py (2)

4-8: LGTM: Well-structured MarketFees class with appropriate constraints.

The MarketFees class is well-defined using Pydantic's BaseModel. The bet_proportion constraint (0 ≤ x < 1) is correctly implemented.

Consider adding a constraint to ensure the absolute fee is non-negative:

absolute: float = Field(..., ge=0.0)  # absolute value paid in the currency of the market

This would prevent potential issues with negative fee values, unless there's a specific reason to allow them.


1-36: LGTM: Well-structured and comprehensive MarketFees implementation.

The MarketFees class is well-designed and provides a robust foundation for handling market fees in the AgentMarket. It aligns perfectly with the PR objectives and introduces a flexible, validated approach to fee management.

For future enhancements, consider:

  1. Adding methods to compare different fee structures.
  2. Implementing serialization/deserialization methods if needed for API interactions.
  3. Including additional validation or business logic methods if specific fee rules need to be enforced.

These suggestions are not necessary for the current implementation but might be useful as the project evolves.

tests/markets/test_markets.py (3)

36-36: LGTM: Addition of fees parameter in test_valid_token_pool

The addition of the fees parameter with MarketFees.get_zero_fees() is appropriate for this test case. It ensures that the AgentMarket is initialized with a neutral fee structure, which is suitable for testing the token pool functionality.

Consider adding an assertion to verify that the fees are indeed zero in this test case. This would make the test more robust and explicit about the fee structure being used.


57-57: LGTM: Addition of fees parameter in test_invalid_token_pool

The addition of the fees parameter with MarketFees.get_zero_fees() is consistent with the changes in test_valid_token_pool and appropriate for this test case.

To improve consistency and reduce duplication, consider extracting MarketFees.get_zero_fees() to a constant or a fixture that can be reused across multiple test cases. This would make it easier to update the fee structure for all tests if needed in the future.


Line range hint 1-89: Suggestion: Add specific tests for the fees functionality

While the changes appropriately update the existing tests to accommodate the new fees parameter, there are no specific tests for the fees functionality itself.

Consider adding new test cases that:

  1. Verify the behavior of AgentMarket with non-zero fees.
  2. Test edge cases related to fee calculations.
  3. Ensure that the fees parameter is correctly applied in various market operations.

This would improve the overall test coverage and help catch any potential issues related to the new fees feature.

tests/tools/test_utils.py (1)

44-50: LGTM: Fee handling correctly updated to use MarketFees. Consider minor improvement.

The changes to use MarketFees and bet_proportion_fee are consistent with the new fee structure. The logic of the test remains intact, still verifying that the value of sold shares decreases as the fee increases.

Consider renaming c1 and c2 to more descriptive names like collateral_low_fee and collateral_high_fee for improved readability:

-c1 = get_collateral(bet_proportion_fee=0.1)
-c2 = get_collateral(bet_proportion_fee=0.35)
-assert c1 > c2
+collateral_low_fee = get_collateral(bet_proportion_fee=0.1)
+collateral_high_fee = get_collateral(bet_proportion_fee=0.35)
+assert collateral_low_fee > collateral_high_fee

Also applies to: 53-54

prediction_market_agent_tooling/markets/polymarket/polymarket.py (2)

30-34: Excellent documentation on fee structure and future considerations.

The added comments provide valuable context about the current fee structure and potential future changes. They explain why zero fees are currently used and include a TODO for future implementation.

Consider adding a link to the Polymarket documentation for easier reference:

-# Based on https://docs.polymarket.com/#fees, there are currently no fees, except for transactions fees.
+# Based on Polymarket documentation (https://docs.polymarket.com/#fees), there are currently no fees, except for transaction fees.

34-34: LGTM: New fees class variable is correctly implemented.

The fees class variable is appropriately added and initialized with zero fees, which is consistent with the current Polymarket fee structure as explained in the comments.

For consistency with other class variables, consider adding a type annotation:

-fees: MarketFees = MarketFees.get_zero_fees()
+fees: t.ClassVar[MarketFees] = MarketFees.get_zero_fees()
prediction_market_agent_tooling/markets/metaculus/metaculus.py (1)

33-33: LGTM: New fees variable is correctly implemented.

The addition of the fees class variable is consistent with the PR objective and correctly initialized for Metaculus.

Consider slightly modifying the comment for clarity:

-    fees: MarketFees = MarketFees.get_zero_fees()  # No fees on Metaculus.
+    fees: MarketFees = MarketFees.get_zero_fees()  # Metaculus doesn't charge fees, so we use zero fees.
prediction_market_agent_tooling/jobs/omen/omen_jobs.py (1)

Line range hint 1-110: Overall file review

The change in this file is minimal but potentially impactful. It reflects a shift from a single fee to multiple fees in the OmenJobAgentMarket class. While the change itself is straightforward, it's crucial to ensure that this modification is part of a consistent update across the entire codebase.

To maintain code quality and prevent potential issues:

  1. Verify that all related classes and methods (especially in OmenAgentMarket) have been updated to support multiple fees.
  2. Update any documentation or comments that might reference the fee structure.
  3. Ensure that any tests involving OmenJobAgentMarket or fee calculations are updated to reflect this change.

Consider creating a dedicated Fees class or data structure if one doesn't already exist. This could help encapsulate fee-related logic and make future changes or additions to the fee structure easier to manage.

tests_integration/markets/omen/test_kelly.py (1)

109-109: Approve temporary use of zero fees, but address TODO

The use of MarketFees.get_zero_fees() is an improvement over hardcoded values. However, the TODO comment suggests that this is a temporary solution.

Please create a follow-up task to implement omen_agent_market.fees as indicated by the TODO comment. This will ensure that realistic fee structures are used in future tests.

prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (1)

20-20: LGTM: Function signature updated for new fee structure.

The change from fee: float = 0.0 to fees: MarketFees is appropriate for the new fee structure. This allows for more complex fee calculations and is consistent with the changes described in the summary.

Consider adding a type hint for the return value to improve code readability:

def get_market_moving_bet(
    yes_outcome_pool_size: float,
    no_outcome_pool_size: float,
    market_p_yes: float,
    target_p_yes: float,
    fees: MarketFees,
    max_iters: int = 100,
) -> SimpleBet:
prediction_market_agent_tooling/tools/langfuse_client_utils.py (2)

96-99: Improved error logging, consider adding log levels.

The addition of warning logs for missing input or arguments in the trace enhances the function's robustness and aids in debugging. This is a good practice for improving traceability of issues.

Consider using different log levels for these warnings. For instance, you might want to use logger.error for missing input, as it's likely a more severe issue than missing arguments. This would allow for more granular log filtering in production environments.

if not trace.input:
    logger.error(f"No input in the trace: {trace}")
    return None
if not trace.input["args"]:
    logger.warning(f"No args in the trace: {trace}")
    return None

106-107: Improved exception handling, consider consistent naming.

The addition of specific exception handling and logging for market parsing errors is a good improvement. It enhances the function's resilience and provides more detailed error information for debugging.

For consistency with the previous log messages, consider including the trace object in this log message as well. This would provide more context for debugging:

except Exception as e:
    logger.warning(f"Market not parsed from langfuse because: {e}. Trace: {trace}")
    return None
tests/markets/test_betting_strategies.py (3)

125-125: LGTM: Fee structure updated correctly.

The change from a direct float value to MarketFees.get_zero_fees(bet_proportion=0.02) is consistent with the new fee structure implementation. This maintains the same fee value while using the new MarketFees class.

Consider extracting the bet_proportion value to a constant at the top of the file for easier maintenance and consistency across tests.


186-186: LGTM: Fee structure updated correctly.

The change to fees=OmenAgentMarket.from_data_model(omen_market).fees ensures consistency with the new fee structure implementation and the OmenAgentMarket class.

Consider extracting the OmenAgentMarket.from_data_model(omen_market).fees to a variable before the function call for improved readability:

market_fees = OmenAgentMarket.from_data_model(omen_market).fees
bet = get_market_moving_bet(
    # ... other parameters ...
    fees=market_fees,
)

264-264: LGTM: Fee structure updated correctly.

The change to fees=MarketFees.get_zero_fees() ensures consistency with the new fee structure implementation and appropriately uses zero fees for the Kelly bet calculation in this test case.

For consistency with the test_minimum_bet_to_win function, consider using MarketFees.get_zero_fees(bet_proportion=0.0) to explicitly show that zero fees are being used.

examples/monitor/match_bets_with_langfuse_traces.py (3)

93-95: LGTM: Output directory creation is well-implemented.

The creation of the output directory is implemented correctly using the Path object. The use of parents=True and exist_ok=True ensures that the directory is created safely.

Consider adding a comment explaining the purpose of the bet_strategy_benchmark directory for better code documentation.


247-248: LGTM: File path updates are consistent and correct.

The changes to file paths correctly utilize the new output_directory variable, ensuring all output files are saved in the designated directory. The use of the forward slash for path joining is appropriate when using Path objects.

For consistency, consider using an f-string for the file path on lines 305-307, similar to the approach used on lines 247-248:

output_directory / f"{agent_name}_details.csv"

This would make the path construction consistent across all file operations.

Also applies to: 305-307, 316-318


Line range hint 1-318: Overall, the changes improve file organization and are well-implemented.

The introduction of a dedicated output directory for bet strategy benchmarks enhances the script's file management. The changes are consistent throughout the file and improve the organization of output files without introducing any apparent issues. The use of pathlib.Path for directory and file path handling is a good practice that enhances cross-platform compatibility.

Consider adding a configuration variable for the output directory name (e.g., BET_STRATEGY_BENCHMARK_DIR) at the top of the file or in a separate configuration file. This would make it easier to change the output location in the future if needed, following the principle of separating configuration from code.

prediction_market_agent_tooling/markets/omen/omen_contracts.py (1)

Line range hint 590-600: LGTM! Consider adding a docstring for clarity.

The new submit_answer_invalid method is a valuable addition, providing a convenient way to submit an invalid answer. The implementation correctly wraps the existing submitAnswer method and uses the INVALID_ANSWER_HEX_BYTES constant.

To improve clarity, consider adding a brief docstring explaining the purpose of this method:

def submit_answer_invalid(
    self,
    api_keys: APIKeys,
    question_id: HexBytes,
    bond: Wei,
    max_previous: Wei | None = None,
    web3: Web3 | None = None,
) -> TxReceipt:
    """
    Submit an invalid answer for the given question.

    This method is a convenience wrapper around `submitAnswer` that automatically
    uses the `INVALID_ANSWER_HEX_BYTES` constant as the answer.
    """
    return self.submitAnswer(
        api_keys=api_keys,
        question_id=question_id,
        answer=INVALID_ANSWER_HEX_BYTES,
        bond=bond,
        max_previous=max_previous,
        web3=web3,
    )
🧰 Tools
🪛 Ruff

169-169: Do not perform function call build_parent_collection_id in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 8fd2577 and 994f0d9.

📒 Files selected for processing (20)
  • .gitignore (1 hunks)
  • examples/monitor/match_bets_with_langfuse_traces.py (5 hunks)
  • prediction_market_agent_tooling/deploy/betting_strategy.py (4 hunks)
  • prediction_market_agent_tooling/jobs/omen/omen_jobs.py (1 hunks)
  • prediction_market_agent_tooling/markets/agent_market.py (4 hunks)
  • prediction_market_agent_tooling/markets/manifold/manifold.py (2 hunks)
  • prediction_market_agent_tooling/markets/market_fees.py (1 hunks)
  • prediction_market_agent_tooling/markets/metaculus/metaculus.py (2 hunks)
  • prediction_market_agent_tooling/markets/omen/omen.py (6 hunks)
  • prediction_market_agent_tooling/markets/omen/omen_contracts.py (1 hunks)
  • prediction_market_agent_tooling/markets/polymarket/polymarket.py (2 hunks)
  • prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (3 hunks)
  • prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (4 hunks)
  • prediction_market_agent_tooling/tools/langfuse_client_utils.py (1 hunks)
  • prediction_market_agent_tooling/tools/utils.py (3 hunks)
  • tests/markets/omen/test_omen.py (1 hunks)
  • tests/markets/test_betting_strategies.py (7 hunks)
  • tests/markets/test_markets.py (3 hunks)
  • tests/tools/test_utils.py (5 hunks)
  • tests_integration/markets/omen/test_kelly.py (4 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent_tooling/markets/omen/omen_contracts.py

169-169: Do not perform function call build_parent_collection_id in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py

65-65: Do not perform function call MarketFees.get_zero_fees in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

🔇 Additional comments (39)
prediction_market_agent_tooling/markets/market_fees.py (3)

1-1: LGTM: Imports are appropriate and minimal.

The imports from Pydantic (BaseModel and Field) are correctly used in the class definition.


10-18: LGTM: Useful static method for creating zero-fee instances.

The get_zero_fees static method provides a convenient way to create MarketFees instances with zero fees. It's well-implemented and uses appropriate default values.


20-36: LGTM: Comprehensive and well-implemented fee calculation methods.

The fee calculation methods are well-designed and cover all necessary scenarios:

  1. total_fee_absolute_value: Correctly calculates the total fee in absolute terms.
  2. total_fee_relative_value: Properly handles the case of zero bet_amount to avoid division by zero.
  3. get_bet_size_after_fees: Accurately calculates the effective bet size after fees.

These methods provide a robust foundation for fee-related operations in the market.

tests/markets/test_markets.py (1)

7-7: LGTM: Import of MarketFees

The import of MarketFees is correctly placed and necessary for the changes in the test functions.

tests/tools/test_utils.py (6)

4-4: LGTM: Import statement for MarketFees is correctly added.

The new import statement for the MarketFees class is necessary and correctly placed. It follows Python import conventions.


17-17: LGTM: Fee parameter correctly updated to use MarketFees.

The change from fee to fees=MarketFees.get_zero_fees() is consistent with the new fee structure. Using get_zero_fees() is appropriate for this test case as it's testing a scenario without fees. The rest of the function remains unchanged, preserving the original test logic.


28-28: LGTM: Fee parameters correctly updated to use MarketFees in both test cases.

The changes from fee to fees=MarketFees.get_zero_fees() are consistent with the new fee structure. Using get_zero_fees() is appropriate for these test cases as they're testing scenarios without fees. The rest of the function remains unchanged, preserving the original test logic for both near-zero and near-one value scenarios.

Also applies to: 37-37


60-66: LGTM: Fee handling and error checking correctly updated.

The changes to use MarketFees and bet_proportion_fee are consistent with the new fee structure. The updated error messages are more specific and aligned with the new implementation. The logic of the test remains intact, still verifying proper error handling for invalid fee values.

Also applies to: 70-71, 74-75


84-84: LGTM: Fee parameter correctly updated to use MarketFees.

The change from fee to fees=MarketFees.get_zero_fees() is consistent with the new fee structure. Using get_zero_fees() is appropriate for this test case as it's testing error handling unrelated to fees. The rest of the function remains unchanged, preserving the original test logic for checking the error condition.


Line range hint 1-87: Overall: Excellent implementation of the new fee structure in test cases.

The changes in this file consistently implement the new fee structure using the MarketFees class across all test functions. The logic of the tests remains intact, with only the fee handling being updated. Error messages and assertions have been appropriately updated where necessary.

These changes improve the consistency and clarity of fee handling in the tests and align well with the PR objective of introducing fees into the system.

.gitignore (1)

166-166: LGTM: Appropriate addition to .gitignore

The addition of bet_strategy_benchmark* to the .gitignore file is a good practice. This will prevent any files or directories starting with "bet_strategy_benchmark" from being tracked by version control. These files are likely generated during testing or benchmarking of betting strategies and don't need to be committed to the repository.

This change helps keep the repository clean and focused on source code and essential files, which aligns well with the PR's objective of introducing fees into the AgentMarket.

prediction_market_agent_tooling/markets/polymarket/polymarket.py (2)

6-6: LGTM: Import statement addition is correct.

The MarketFees import is correctly added to the existing import statement from prediction_market_agent_tooling.markets.agent_market. This import is necessary for the new fees class variable.


Line range hint 6-34: Overall, the changes look good and align with the PR objectives.

The introduction of the fees variable, along with the detailed comments, successfully implements the groundwork for managing fees in the PolymarketAgentMarket class. The changes are well-documented and provide a clear path for future improvements.

To ensure consistency across the codebase, let's verify if similar fee-related changes have been made in other market implementations:

This will help ensure that the fee implementation is consistent across different market types.

prediction_market_agent_tooling/markets/metaculus/metaculus.py (2)

8-8: LGTM: Import statement addition is correct.

The addition of MarketFees to the import statement is necessary and correctly placed with other imports from the same module.


33-33: Verify the intended usage of the fees variable.

The fees variable has been added to the class, but it's not used in any of the class methods. While this might be intentional since Metaculus doesn't charge fees, it's worth confirming that this is the desired behavior.

Could you please confirm if there are any plans to use the fees variable in the class methods, or if its presence is solely for consistency with other market classes?

To help verify this, you can run the following script to check for any usage of self.fees in the class methods:

prediction_market_agent_tooling/markets/manifold/manifold.py (1)

9-9: LGTM: Import statement added correctly.

The addition of MarketFees to the import list is consistent with its usage in the class. This change is appropriate and necessary for the new functionality being introduced.

tests_integration/markets/omen/test_kelly.py (4)

5-9: LGTM: Import statements updated correctly

The addition of MarketFees to the imports and the reformatting of the import statement improve code organization and readability. These changes are consistent with the modifications in the rest of the file.


64-64: Please clarify the reason for changing the limit

The limit parameter in get_omen_binary_markets_simple has been changed from 2 to 1. This reduces the number of markets fetched for testing. Could you please explain the rationale behind this change? It's important to ensure that this modification doesn't negatively impact the test coverage.


124-128: LGTM: Consistent fee handling implementation

The changes to the assert_price_impact function are consistent with the modifications made elsewhere in the file. The addition of the fees parameter and the use of MarketFees.get_zero_fees() align with the new fee handling approach.


94-102: ⚠️ Potential issue

Address TODO and verify impact of removed fees parameter

  1. There's a TODO comment to uncomment the fees parameter in the get_kelly_bet_full function. Please ensure this is addressed before merging the PR.

  2. The fees parameter has been removed from the calculate_bet_amount_for_price_impact method call. This change might affect the accuracy of the test. Can you verify if this removal is intentional and doesn't impact the test's effectiveness?

To help verify the impact of these changes, you could run the following script:

prediction_market_agent_tooling/tools/betting_strategies/market_moving.py (3)

6-9: LGTM: Import changes are consistent with new fee structure.

The addition of MarketFees import is appropriate for the subsequent changes in the file, aligning with the new fee structure implementation.


112-126: LGTM: Sanity check updated to account for new fee structure.

The introduction of bet_to_check_size_after_fees and its usage in calculating new pool sizes ensures that the sanity check accurately reflects the impact of fees. This is consistent with the new fee structure implementation.

To ensure the correctness of these changes, please verify that market.fees is properly initialized as an instance of MarketFees. You can run the following script to check its usage:

#!/bin/bash
# Description: Verify the initialization and usage of market.fees

# Test: Search for market.fees usage
rg --type python -A 5 $'market.fees'

# Test: Search for OmenAgentMarket initialization with fees
ast-grep --lang python --pattern $'OmenAgentMarket(
  $$$
  fees=$_,
  $$$
)'

53-53: LGTM: Fee calculation updated to use new MarketFees class.

The change to use fees.get_bet_size_after_fees(bet_amount) is consistent with the new fee structure and improves modularity by encapsulating fee calculations within the MarketFees class.

To ensure the correctness of this change, please verify that the MarketFees.get_bet_size_after_fees() method is implemented correctly and returns the expected values. You can run the following script to check its implementation:

prediction_market_agent_tooling/tools/langfuse_client_utils.py (1)

Line range hint 1-158: Overall improvements in error handling and logging.

The changes made to this file focus on enhancing error handling and logging within the trace_to_omen_agent_market function. These improvements will significantly aid in debugging and tracing issues in production environments. The modifications are consistent with good software engineering practices and do not alter the core functionality of the code.

tests/markets/omen/test_omen.py (1)

245-245: LGTM! Verify consistency across the codebase.

The change from fee to fees in the get_market_moving_bet function call is correct and aligns with the PR objectives of introducing fees into AgentMarket.

To ensure consistency, let's verify if this change has been applied uniformly across the codebase:

This script will help identify any inconsistencies in the usage of fee vs fees in get_market_moving_bet calls across the codebase.

✅ Verification successful

Let's re-run the verification with corrected search parameters.


Let's verify the parameter names in the get_market_moving_bet function definition.


<!-- [monologue]
[task finalize_verification_1/1]
The shell scripts confirmed that there are no remaining instances of the `fee` parameter in `get_market_moving_bet` function calls outside of the specified test file. Additionally, the function definition correctly uses `fees` instead of `fee`.
[observations]
- No instances of `fee` parameter found in `get_market_moving_bet` calls outside `tests/markets/omen/test_omen.py`.
- The function `get_market_moving_bet` is defined with the `fees` parameter as intended.
[analysis]
- The change from `fee` to `fees` has been consistently applied.
- No further verification steps are necessary.
[actions]
- Conclude the verification process as successful.
-->

Verification Successful: 'fee' parameter consistently updated to 'fees'.

The change from fee to fees in the get_market_moving_bet function call has been applied consistently across the codebase, and the function definition correctly uses fees as intended.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of 'fee' parameter in get_market_moving_bet calls

# Search for get_market_moving_bet function calls with 'fee' parameter
echo "Searching for get_market_moving_bet calls with 'fee' parameter:"
rg --type python 'get_market_moving_bet\(.*fee\s*=' -g '!tests/markets/omen/test_omen.py'

# Search for get_market_moving_bet function definition
echo "Searching for get_market_moving_bet function definition:"
rg --type python 'def get_market_moving_bet\('

Length of output: 468


Script:

# Description: Check for any remaining instances of 'fee' parameter in get_market_moving_bet calls

# Search for get_market_moving_bet function calls with 'fee' parameter
echo "Searching for get_market_moving_bet calls with 'fee' parameter:"
rg 'get_market_moving_bet\(.*fee\s*=' -g '*.py' --ignore 'tests/markets/omen/test_omen.py'

# Search for get_market_moving_bet function definition
echo "Searching for get_market_moving_bet function definition:"
rg 'def get_market_moving_bet\(' -g '*.py'

Length of output: 502


Script:

# Description: Verify 'get_market_moving_bet' function definition uses 'fees' instead of 'fee'

# Extract the function definition for 'get_market_moving_bet' with context lines
echo "Extracting 'get_market_moving_bet' function definition:"
rg 'def get_market_moving_bet\(' prediction_market_agent_tooling/tools/betting_strategies/market_moving.py -A 5

Length of output: 406

tests/markets/test_betting_strategies.py (4)

28-31: LGTM: Import statement updated correctly.

The addition of MarketFees to the import statement is consistent with the changes made in the rest of the file, reflecting the new fee structure implementation.


213-213: LGTM: Fee structure updated correctly.

The change to fees=market.fees ensures consistency with the new fee structure implementation and the OmenAgentMarket class. This approach correctly uses the fees attribute of the market object.


286-286: LGTM: Fee structure updated correctly.

The changes to fees=market.fees in both the get_market_moving_bet and get_kelly_bet_full function calls ensure consistency with the new fee structure implementation and the OmenAgentMarket class. This approach correctly uses the fees attribute of the market object in both cases.

Also applies to: 296-296


Line range hint 1-307: Overall assessment: Changes implement new fee structure correctly and consistently.

The modifications in this file successfully integrate the new MarketFees class across various test functions. The changes maintain the functionality of the tests while adapting to the new fee handling approach. Minor suggestions for improvements have been made, primarily focusing on readability and consistency.

examples/monitor/match_bets_with_langfuse_traces.py (1)

1-1: LGTM: Import statement for Path is appropriate.

The addition of from pathlib import Path is necessary for the new directory management functionality and follows Python best practices for file path handling.

prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (1)

93-93: Ensure f-string compatibility with Python versions

The f-string syntax {fees=} is a feature introduced in Python 3.8 that outputs the expression text and its value. If your codebase needs to support earlier Python versions (e.g., Python 3.6 or 3.7), this syntax will raise a SyntaxError.

Please verify the minimum Python version requirement for your project. If compatibility with earlier versions is needed, consider modifying the f-string:

-            raise RuntimeError(
-                f"Kelly works only with bet-proportional fees, but the fees are {fees=}."
-            )
+            raise RuntimeError(
+                f"Kelly works only with bet-proportional fees, but the fees are {fees}."
+            )
prediction_market_agent_tooling/tools/utils.py (1)

21-21: Import statement for MarketFees added appropriately

The import of MarketFees is necessary for handling the updated fee structure in the function below.

prediction_market_agent_tooling/markets/agent_market.py (2)

64-64: Verify initialization of the new 'fees' attribute

The addition of the fees: MarketFees attribute in the AgentMarket class may affect the instantiation of this class throughout the codebase. Ensure that all instances where AgentMarket is created are updated appropriately, and that any missing fees data is handled correctly.

Use the following script to find all instantiations of AgentMarket that may need updating:

#!/bin/bash
# Description: Find all instances where 'AgentMarket' is instantiated without 'fees'

# Search for AgentMarket instantiation lines
rg --type py --fixed-strings 'AgentMarket('

5-5: Ensure compatibility with Pydantic version 2

The use of model_validator indicates that the code relies on Pydantic version 2 features. Please confirm that the project's environment and dependencies have been updated to use Pydantic v2 to avoid any runtime issues.

You can verify the Pydantic version with the following script:

✅ Verification successful

Pydantic version 2.x.x confirmed

The project is using Pydantic version 2.6.1 as specified in pyproject.toml, ensuring compatibility with the use of model_validator.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the installed Pydantic version.

# Test: Display Pydantic version. Expect: Version 2.x.x
pip show pydantic | grep Version

Length of output: 1910


Script:

#!/bin/bash
# Description: Check the installed Pydantic version.

# Test: Display Pydantic version. Expect: Version 2.x.x
pip show pydantic | grep Version

Length of output: 105


Script:

#!/bin/bash
# Description: Check the installed Pydantic version using python -m pip.

# Test: Display Pydantic version. Expect: Version 2.x.x
python -m pip show pydantic | grep Version

Length of output: 118


Script:

#!/bin/bash
# Description: Attempt to check the installed Pydantic version using pip3 or python3. If not found, search dependency files.

# Attempt to use pip3
if command -v pip3 >/dev/null 2>&1; then
    pip3 show pydantic | grep Version
    exit 0
fi

# Attempt to use python3
if command -v python3 >/dev/null 2>&1; then
    python3 -m pip show pydantic | grep Version
    exit 0
fi

# Search common dependency files for Pydantic version
# Search requirements.txt
if fd --type f --fixed-strings 'requirements.txt' | grep .; then
    grep -E 'pydantic[=<>!~]' requirements.txt
fi

# Search pyproject.toml
if fd --type f --fixed-strings 'pyproject.toml' | grep .; then
    grep -E 'pydantic\s*=' pyproject.toml
fi

# Search Pipfile.lock
if fd --type f --fixed-strings 'Pipfile.lock' | grep .; then
    jq '.default.pydantic.version' Pipfile.lock
fi

Length of output: 275

prediction_market_agent_tooling/deploy/betting_strategy.py (1)

7-7: Importing MarketFees is appropriate

The addition of MarketFees to the import statement is necessary for handling market fees within the betting strategies.

prediction_market_agent_tooling/markets/omen/omen.py (4)

25-25: Importing MarketFees for enhanced fee management

The addition of MarketFees to the imports ensures that the new fee structure is available within this module.


243-243: Passing fees to calculate_sell_amount_in_collateral

Including fees=self.fees in the call to calculate_sell_amount_in_collateral ensures that fee calculations are correctly applied when calculating the sell amount in collateral.


355-360: Initializing MarketFees correctly in from_data_model

The fees attribute is instantiated using MarketFees, with bet_proportion calculated from model.fee. Ensure that model.fee accurately represents the fee percentage and that the conversion to a float is appropriate.


606-606: Including fees in get_buy_token_amount

Passing fees=self.fees to the get_buy_outcome_token_amount function ensures that fees are correctly accounted for when calculating the buy token amount.

@@ -75,7 +75,7 @@ def from_omen_agent_market(market: OmenAgentMarket) -> "OmenJobAgentMarket":
market_maker_contract_address_checksummed=market.market_maker_contract_address_checksummed,
condition=market.condition,
finalized_time=market.finalized_time,
fee=market.fee,
fees=market.fees,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Inconsistent Renaming of fee to fees Detected

The change from fee to fees has been partially applied. Several instances still use fee, which can lead to inconsistencies and potential bugs. Please address the following:

  • Files still using fee:
    • prediction_market_agent_tooling/markets/omen/omen_contracts.py
    • prediction_market_agent_tooling/markets/omen/omen.py
    • prediction_market_agent_tooling/markets/omen/data_models.py
    • Various test and script files under tests/ and scripts/ directories

Ensure that all instances of fee are consistently renamed to fees across the entire codebase.

🔗 Analysis chain

Approve change and suggest improvements

The change from fee to fees looks good, as it likely reflects a shift from a single fee to multiple fees. However, there are a couple of points to consider:

  1. Consider adding a type annotation for fees to improve code clarity and maintainability.
  2. Ensure that this change is consistent with the rest of the codebase, particularly in places where OmenJobAgentMarket is instantiated or where fees are accessed.

To verify the impact of this change, you can run the following script:

This script will help identify any inconsistencies or places where further updates might be needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for occurrences of 'fee' and 'fees' related to OmenJobAgentMarket or OmenAgentMarket

# Search for 'fee' occurrences
echo "Occurrences of 'fee':"
rg --type python -i '\bfee\b' --context 3

echo -e "\nOccurrences of 'fees':"
rg --type python -i '\bfees\b' --context 3

echo -e "\nOccurrences of 'OmenJobAgentMarket':"
rg --type python 'OmenJobAgentMarket' --context 3

echo -e "\nOccurrences of 'OmenAgentMarket':"
rg --type python 'OmenAgentMarket' --context 3

Length of output: 619


Script:

#!/bin/bash
# Description: Check for occurrences of 'fee' and 'fees' related to OmenJobAgentMarket or OmenAgentMarket in Python files

# Search for 'fee' occurrences
echo "Occurrences of 'fee':"
rg -i '\bfee\b' --context 3 --glob "*.py"

echo -e "\nOccurrences of 'fees':"
rg -i '\bfees\b' --context 3 --glob "*.py"

echo -e "\nOccurrences of 'OmenJobAgentMarket':"
rg 'OmenJobAgentMarket' --context 3 --glob "*.py"

echo -e "\nOccurrences of 'OmenAgentMarket':"
rg 'OmenAgentMarket' --context 3 --glob "*.py"

Length of output: 108510

Comment on lines +37 to +42
# Manifold has additional fees than `platform_absolute`, but they don't expose them in the API before placing the bet, see https://docs.manifold.markets/api.
# So we just consider them as 0, which anyway is true for all markets I randomly checked on Manifold.
fees: MarketFees = MarketFees(
bet_proportion=0,
absolute=0.25, # For doing trades via API.
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Potential issues with the assumption of zero bet_proportion fees.

The search revealed references to proportional fees in the codebase, such as in market_fees.py and kelly_criterion.py. This indicates that proportional fees might be present in some cases, contrary to the assumption that they are always zero.

Consider reviewing how proportional fees are handled across different markets to ensure consistency and accuracy.

🔗 Analysis chain

Approve the addition of fees, but consider potential edge cases.

The introduction of the fees variable with MarketFees type is a good addition for structured fee handling. The comments provide clear reasoning for the chosen values.

However, there's a potential concern:

The comment states that the zero bet_proportion fee is "true for all markets I randomly checked on Manifold." This assumption might not hold for all cases. Consider adding a verification step or a more robust way to handle potential variations in fee structures across different markets.

To verify this assumption, we can run the following script to check for any mentions of non-zero proportional fees in the codebase:

If this search returns results, it might indicate that there are cases where proportional fees are considered, and we should review the assumption made in this code.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for mentions of non-zero proportional fees in the codebase

# Test: Look for mentions of proportional fees
rg --type python "proportional.*fee|fee.*proportional"

Length of output: 87


Script:

#!/bin/bash
# Description: Search for mentions of non-zero proportional fees in Python files

rg "proportional.*fee|fee.*proportional" --glob '*.py'

Length of output: 523

conditionId: HexBytes,
index_sets: t.List[int],
amount: Wei,
parent_collection_id: HexStr = build_parent_collection_id(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider moving the default value calculation inside the function body.

While providing a default value for parent_collection_id improves usability, calling build_parent_collection_id() in the default argument can lead to unexpected behavior. This is because the function is evaluated only once at function definition time, not each time mergePositions is called.

To address this, consider the following approach:

def mergePositions(
    self,
    api_keys: APIKeys,
    collateral_token_address: ChecksumAddress,
    conditionId: HexBytes,
    index_sets: t.List[int],
    amount: Wei,
    parent_collection_id: HexStr | None = None,
    web3: Web3 | None = None,
) -> TxReceipt:
    if parent_collection_id is None:
        parent_collection_id = build_parent_collection_id()
    
    # Rest of the function implementation...

This approach ensures that build_parent_collection_id() is called each time the function is invoked with no parent_collection_id provided, which is likely the intended behavior.

🧰 Tools
🪛 Ruff

169-169: Do not perform function call build_parent_collection_id in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

@@ -61,7 +62,7 @@ def get_kelly_bet_full(
estimated_p_yes: float,
confidence: float,
max_bet: float,
fee: float = 0.0, # proportion, 0 to 1
fees: MarketFees = MarketFees.get_zero_fees(), # TODO: Remove default value.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid function calls in default arguments

Using a function call like MarketFees.get_zero_fees() in a function's default argument is not recommended because the function call is evaluated only once at the time of function definition. This can lead to unexpected behavior, especially if MarketFees is mutable.

As indicated by the TODO comment and the static analysis hint (B008), consider removing the default value from the function signature and setting it inside the function instead.

Here's how you can modify the code:

 def get_kelly_bet_full(
     yes_outcome_pool_size: float,
     no_outcome_pool_size: float,
     estimated_p_yes: float,
     confidence: float,
     max_bet: float,
-    fees: MarketFees = MarketFees.get_zero_fees(),  # TODO: Remove default value.
+    fees: MarketFees = None,  # TODO: Remove default value.
 ) -> SimpleBet:
+    if fees is None:
+        fees = MarketFees.get_zero_fees()
📝 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.

Suggested change
fees: MarketFees = MarketFees.get_zero_fees(), # TODO: Remove default value.
fees: MarketFees = None, # TODO: Remove default value.
) -> SimpleBet:
if fees is None:
fees = MarketFees.get_zero_fees()
🧰 Tools
🪛 Ruff

65-65: Do not perform function call MarketFees.get_zero_fees in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable

(B008)

for v in [shares_to_sell, holdings, other_holdings]:
if v <= 0:
raise ValueError("All share args must be greater than 0")

def f(r: float) -> float:
R = r / (1 - fee)
R = (r + fees.absolute) / (1 - fees.bet_proportion)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure fees.bet_proportion is less than 1 to prevent division by zero

In the calculation R = (r + fees.absolute) / (1 - fees.bet_proportion), if fees.bet_proportion is equal to or greater than 1, this will cause a division by zero or a negative denominator, which may lead to calculation errors. Please add validation to ensure that fees.bet_proportion is within the range [0, 1).

Apply this diff to add validation:

 def f(r: float) -> float:
+    if fees.bet_proportion >= 1:
+        raise ValueError("fees.bet_proportion must be less than 1")
     R = (r + fees.absolute) / (1 - fees.bet_proportion)
     first_term = other_holdings - R

Committable suggestion was skipped due to low confidence.

Comment on lines +82 to +89
@model_validator(mode="before")
def handle_legacy_fee(cls, data: dict[str, t.Any]) -> dict[str, t.Any]:
# Backward compatibility for older `AgentMarket` without `fees`.
if "fees" not in data and "fee" in data:
data["fees"] = MarketFees(absolute=0.0, bet_proportion=data["fee"])
del data["fee"]
return data

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Add tests for handling legacy 'fee' data

The handle_legacy_fee method is crucial for backward compatibility with data using the old fee attribute. It's important to have unit tests that cover this logic to prevent regressions.

Consider adding or updating unit tests to verify that markets with the legacy fee attribute are correctly converted to use the new fees structure.

Comment on lines +199 to 205
self,
buy_direction: bool,
bet_amount: float,
yes: float,
no: float,
fees: MarketFees,
) -> float:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling to prevent division by zero

In the method calculate_price_impact_for_bet_amount, there is a potential division by zero when calculating expected_price if total_outcome_tokens equals zero. Consider adding a check to ensure total_outcome_tokens is not zero before performing the division.

Apply this diff to add the check:

 def calculate_price_impact_for_bet_amount(
     self,
     buy_direction: bool,
     bet_amount: float,
     yes: float,
     no: float,
     fees: MarketFees,
 ) -> float:
     total_outcome_tokens = yes + no
+    if total_outcome_tokens == 0:
+        raise ValueError("Total outcome tokens cannot be zero")
     expected_price = (
         no / total_outcome_tokens if buy_direction else yes / total_outcome_tokens
     )
     tokens_to_buy = get_buy_outcome_token_amount(
         bet_amount, buy_direction, yes, no, fees
     )
     actual_price = bet_amount / tokens_to_buy
     # price_impact should always be > 0
     price_impact = (actual_price - expected_price) / expected_price
     return price_impact

Committable suggestion was skipped due to low confidence.

) -> float:
total_outcome_tokens = yes + no
expected_price = (
no / total_outcome_tokens if buy_direction else yes / total_outcome_tokens
)

tokens_to_buy = get_buy_outcome_token_amount(
bet_amount, buy_direction, yes, no, fee
bet_amount, buy_direction, yes, no, fees
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure tokens_to_buy is not zero to prevent division by zero

When calculating actual_price, there is a potential risk of division by zero if tokens_to_buy is zero. Consider verifying that tokens_to_buy is not zero before performing the division.

Apply this diff to add the check:

     tokens_to_buy = get_buy_outcome_token_amount(
         bet_amount, buy_direction, yes, no, fees
     )
+    if tokens_to_buy == 0:
+        raise ValueError("Tokens to buy cannot be zero")
     actual_price = bet_amount / tokens_to_buy

Committable suggestion was skipped due to low confidence.

Comment on lines +636 to +639
self.fees.get_bet_size_after_fees(bet_amount.amount)
)
new_no_outcome_pool_size = no_outcome_pool_size + (
bet_amount.amount * (1 - self.fee)
self.fees.get_bet_size_after_fees(bet_amount.amount)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential error in updating outcome pool sizes in get_new_p_yes

The same amount self.fees.get_bet_size_after_fees(bet_amount.amount) is added to both new_yes_outcome_pool_size and new_no_outcome_pool_size. This may not correctly reflect the adjustment of pool sizes when placing a bet on a specific outcome. Typically, the amount after fees should be added only to the pool corresponding to the bet direction.

Apply this diff to correct the pool size updates:

 def get_new_p_yes(self, bet_amount: BetAmount, direction: bool) -> Probability:
     if not self.has_token_pool():
         raise ValueError("Outcome token pool is required to calculate new p_yes.")

     outcome_token_pool = check_not_none(self.outcome_token_pool)
     yes_outcome_pool_size = outcome_token_pool[self.get_outcome_str_from_bool(True)]
     no_outcome_pool_size = outcome_token_pool[self.get_outcome_str_from_bool(False)]

-    new_yes_outcome_pool_size = yes_outcome_pool_size + (
-        self.fees.get_bet_size_after_fees(bet_amount.amount)
-    )
-    new_no_outcome_pool_size = no_outcome_pool_size + (
-        self.fees.get_bet_size_after_fees(bet_amount.amount)
-    )
+    if direction:
+        new_yes_outcome_pool_size = yes_outcome_pool_size + self.fees.get_bet_size_after_fees(bet_amount.amount)
+        new_no_outcome_pool_size = no_outcome_pool_size
+    else:
+        new_yes_outcome_pool_size = yes_outcome_pool_size
+        new_no_outcome_pool_size = no_outcome_pool_size + self.fees.get_bet_size_after_fees(bet_amount.amount)

     received_token_amount = self.get_buy_token_amount(bet_amount, direction).amount
     if direction:
         new_yes_outcome_pool_size -= received_token_amount
     else:
         new_no_outcome_pool_size -= received_token_amount

     new_p_yes = new_no_outcome_pool_size / (
         new_yes_outcome_pool_size + new_no_outcome_pool_size
     )
     return Probability(new_p_yes)
📝 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.

Suggested change
self.fees.get_bet_size_after_fees(bet_amount.amount)
)
new_no_outcome_pool_size = no_outcome_pool_size + (
bet_amount.amount * (1 - self.fee)
self.fees.get_bet_size_after_fees(bet_amount.amount)
if direction:
new_yes_outcome_pool_size = yes_outcome_pool_size + self.fees.get_bet_size_after_fees(bet_amount.amount)
new_no_outcome_pool_size = no_outcome_pool_size
else:
new_yes_outcome_pool_size = yes_outcome_pool_size
new_no_outcome_pool_size = no_outcome_pool_size + self.fees.get_bet_size_after_fees(bet_amount.amount)

@@ -1266,14 +1269,14 @@
buy_direction: bool,
yes_outcome_pool_size: float,
no_outcome_pool_size: float,
fee: float,
fees: MarketFees,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Missing fees argument in get_buy_outcome_token_amount function call

An instance of get_buy_outcome_token_amount was found without the fees parameter:

  • prediction_market_agent_tooling/deploy/betting_strategy.py: tokens_to_buy = get_buy_outcome_token_amount(

Please update this call to include the fees argument to prevent any TypeError.

🔗 Analysis chain

Updating get_buy_outcome_token_amount function signature

The function get_buy_outcome_token_amount now includes a new parameter fees: MarketFees. Ensure that all calls to this function across the codebase have been updated to pass the fees argument to prevent any TypeError.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all calls to 'get_buy_outcome_token_amount' missing the new 'fees' parameter.

# Test: Search for function calls without 'fees' argument. Expect: No occurrences.
rg --type python --fixed-strings 'get_buy_outcome_token_amount(' --not -g 'omen.py' --no-heading -A 0 -B 0 | grep -v 'fees='

Length of output: 555


Script:

#!/bin/bash
# Description: Find all calls to 'get_buy_outcome_token_amount' missing the new 'fees' parameter.

# Corrected: Use '--glob' to exclude 'omen.py' instead of the invalid '--not' flag.
rg --type python --fixed-strings 'get_buy_outcome_token_amount(' --glob '!omen.py' --no-heading | grep -v 'fees='

Length of output: 144


Script:

#!/bin/bash
# Description: Find all calls to 'get_buy_outcome_token_amount' missing the new 'fees' parameter.

# Corrected: Use '--type py' instead of 'python'.
rg --type py --fixed-strings 'get_buy_outcome_token_amount(' --glob '!omen.py' --no-heading | grep -v 'fees='

Length of output: 222

@kongzii
Copy link
Contributor Author

kongzii commented Oct 17, 2024

test_place_bet_with_autodeposit if failing but it passes locally and shouldn't be related at all to this PR, will merge this and look at it later

@kongzii kongzii merged commit 5ed2712 into main Oct 17, 2024
11 of 14 checks passed
@kongzii kongzii deleted the peter/rethink-fees branch October 17, 2024 11:24
@coderabbitai coderabbitai bot mentioned this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants