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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2e8bb2f
Rethink fees
kongzii Oct 17, 2024
36185a2
missing default value
kongzii Oct 17, 2024
0759a21
some more missing fees
kongzii Oct 17, 2024
95a93ef
use total_fee_absolute_value
kongzii Oct 17, 2024
994f0d9
Merge branch 'main' into peter/rethink-fees
kongzii Oct 17, 2024
e1ba292
try to fix test
kongzii Oct 17, 2024
70f0349
lint
kongzii Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,3 +162,5 @@ cython_debug/

tests_files/*
!tests_files/.gitkeep

bet_strategy_benchmark*
15 changes: 12 additions & 3 deletions examples/monitor/match_bets_with_langfuse_traces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from pathlib import Path
from typing import Any

import pandas as pd
Expand Down Expand Up @@ -89,6 +90,9 @@ def get_outcome_for_trace(


if __name__ == "__main__":
output_directory = Path("bet_strategy_benchmark")
output_directory.mkdir(parents=True, exist_ok=True)

# Get the private keys for the agents from GCP Secret Manager
agent_gcp_secret_map = {
"DeployablePredictionProphetGPT4TurboFinalAgent": "pma-prophetgpt4turbo-final",
Expand Down Expand Up @@ -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 😌

index=False,
)

sum_squared_errors = 0.0
Expand Down Expand Up @@ -297,7 +302,9 @@ def get_outcome_for_trace(
+ simulations_df.to_markdown(index=False)
)
# export details per agent
pd.DataFrame.from_records(details).to_csv(f"{agent_name}_details.csv")
pd.DataFrame.from_records(details).to_csv(
output_directory / f"{agent_name}_details.csv"
)

print(f"Correlation between p_yes mse and total profit:")
for strategy_name, mse_profit in strat_mse_profits.items():
Expand All @@ -306,5 +313,7 @@ def get_outcome_for_trace(
correlation = pd.Series(mse).corr(pd.Series(profit))
print(f"{strategy_name}: {correlation=}")

with open("match_bets_with_langfuse_traces_overall.md", "w") as overall_f:
with open(
output_directory / "match_bets_with_langfuse_traces_overall.md", "w"
) as overall_f:
overall_f.write(overall_md)
16 changes: 10 additions & 6 deletions prediction_market_agent_tooling/deploy/betting_strategy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from prediction_market_agent_tooling.gtypes import xDai
from prediction_market_agent_tooling.loggers import logger
from prediction_market_agent_tooling.markets.agent_market import AgentMarket
from prediction_market_agent_tooling.markets.agent_market import AgentMarket, MarketFees
from prediction_market_agent_tooling.markets.data_models import (
Currency,
Position,
Expand Down Expand Up @@ -178,7 +178,7 @@ def calculate_trades(
if self.max_price_impact:
# Adjust amount
max_price_impact_bet_amount = self.calculate_bet_amount_for_price_impact(
market, kelly_bet, 0
market, kelly_bet
)

# We just don't want Kelly size to extrapolate price_impact - hence we take the min.
Expand All @@ -196,15 +196,20 @@ def calculate_trades(
return trades

def calculate_price_impact_for_bet_amount(
self, buy_direction: bool, bet_amount: float, yes: float, no: float, fee: float
self,
buy_direction: bool,
bet_amount: float,
yes: float,
no: float,
fees: MarketFees,
) -> float:
Comment on lines +199 to 205
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.

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.

)

actual_price = bet_amount / tokens_to_buy
Expand All @@ -216,7 +221,6 @@ def calculate_bet_amount_for_price_impact(
self,
market: AgentMarket,
kelly_bet: SimpleBet,
fee: float,
) -> float:
def calculate_price_impact_deviation_from_target_price_impact(
bet_amount: xDai,
Expand All @@ -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.

)
# We return abs for the algorithm to converge to 0 instead of the min (and possibly negative) value.

Expand Down
2 changes: 1 addition & 1 deletion prediction_market_agent_tooling/jobs/omen/omen_jobs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

)


Expand Down
12 changes: 11 additions & 1 deletion prediction_market_agent_tooling/markets/agent_market.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from enum import Enum

from eth_typing import ChecksumAddress
from pydantic import BaseModel, field_validator
from pydantic import BaseModel, field_validator, model_validator
from pydantic_core.core_schema import FieldValidationInfo

from prediction_market_agent_tooling.config import APIKeys
Expand All @@ -16,6 +16,7 @@
ResolvedBet,
TokenAmount,
)
from prediction_market_agent_tooling.markets.market_fees import MarketFees
from prediction_market_agent_tooling.tools.utils import (
DatetimeUTC,
check_not_none,
Expand Down Expand Up @@ -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.


@field_validator("outcome_token_pool")
def validate_outcome_token_pool(
Expand All @@ -77,6 +79,14 @@ def validate_outcome_token_pool(
)
return outcome_token_pool

@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"])
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.

del data["fee"]
return data

Comment on lines +82 to +89
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.

@property
def current_p_no(self) -> Probability:
return Probability(1 - self.current_p_yes)
Expand Down
8 changes: 8 additions & 0 deletions prediction_market_agent_tooling/markets/manifold/manifold.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from prediction_market_agent_tooling.markets.agent_market import (
AgentMarket,
FilterBy,
MarketFees,
SortBy,
)
from prediction_market_agent_tooling.markets.data_models import BetAmount, Currency
Expand Down Expand Up @@ -33,6 +34,13 @@ class ManifoldAgentMarket(AgentMarket):
currency: t.ClassVar[Currency] = Currency.Mana
base_url: t.ClassVar[str] = MANIFOLD_BASE_URL

# 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.
)
Comment on lines +37 to +42
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


def get_last_trade_p_yes(self) -> Probability:
"""On Manifold, probablities aren't updated after the closure, so we can just use the current probability"""
return self.current_p_yes
Expand Down
36 changes: 36 additions & 0 deletions prediction_market_agent_tooling/markets/market_fees.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from pydantic import BaseModel, Field


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.

) # proportion of the bet, from 0 to 1
absolute: float # absolute value paid in the currency of the market

@staticmethod
def get_zero_fees(
bet_proportion: float = 0.0,
absolute: float = 0.0,
) -> "MarketFees":
return MarketFees(
bet_proportion=bet_proportion,
absolute=absolute,
)

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
Comment on lines +20 to +33
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


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 😅

return bet_amount * (1 - self.bet_proportion) - self.absolute
2 changes: 2 additions & 0 deletions prediction_market_agent_tooling/markets/metaculus/metaculus.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from prediction_market_agent_tooling.markets.agent_market import (
AgentMarket,
FilterBy,
MarketFees,
SortBy,
)
from prediction_market_agent_tooling.markets.metaculus.api import (
Expand All @@ -29,6 +30,7 @@ class MetaculusAgentMarket(AgentMarket):
description: str | None = (
None # Metaculus markets don't have a description, so just default to None.
)
fees: MarketFees = MarketFees.get_zero_fees() # No fees on Metaculus.

@staticmethod
def from_data_model(model: MetaculusQuestion) -> "MetaculusAgentMarket":
Expand Down
23 changes: 13 additions & 10 deletions prediction_market_agent_tooling/markets/omen/omen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from prediction_market_agent_tooling.markets.agent_market import (
AgentMarket,
FilterBy,
MarketFees,
SortBy,
)
from prediction_market_agent_tooling.markets.data_models import (
Expand Down Expand Up @@ -101,7 +102,6 @@ class OmenAgentMarket(AgentMarket):
finalized_time: DatetimeUTC | None
created_time: DatetimeUTC
close_time: DatetimeUTC
fee: float # proportion, from 0 to 1

_binary_market_p_yes_history: list[Probability] | None = None
description: str | None = (
Expand Down Expand Up @@ -240,7 +240,7 @@ def calculate_sell_amount_in_collateral(
shares_to_sell=amount.amount,
holdings=wei_to_xdai(pool_balance[self.get_index_set(sell_str)]),
other_holdings=wei_to_xdai(pool_balance[self.get_index_set(other_str)]),
fee=self.fee,
fees=self.fees,
)
return xDai(collateral)

Expand Down Expand Up @@ -352,7 +352,12 @@ def from_data_model(model: OmenMarket) -> "OmenAgentMarket":
url=model.url,
volume=wei_to_xdai(model.collateralVolume),
close_time=model.close_time,
fee=float(wei_to_xdai(model.fee)) if model.fee is not None else 0.0,
fees=MarketFees(
bet_proportion=(
float(wei_to_xdai(model.fee)) if model.fee is not None else 0.0
),
absolute=0,
),
outcome_token_pool={
model.outcomes[i]: wei_to_xdai(Wei(model.outcomeTokenAmounts[i]))
for i in range(len(model.outcomes))
Expand Down Expand Up @@ -598,7 +603,7 @@ def get_buy_token_amount(
buy_direction=direction,
yes_outcome_pool_size=outcome_token_pool[OMEN_TRUE_OUTCOME],
no_outcome_pool_size=outcome_token_pool[OMEN_FALSE_OUTCOME],
fee=self.fee,
fees=self.fees,
)
return TokenAmount(amount=amount, currency=self.currency)

Expand Down Expand Up @@ -628,10 +633,10 @@ def get_new_p_yes(self, bet_amount: BetAmount, direction: bool) -> Probability:
no_outcome_pool_size = outcome_token_pool[self.get_outcome_str_from_bool(False)]

new_yes_outcome_pool_size = yes_outcome_pool_size + (
bet_amount.amount * (1 - self.fee)
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)
Comment on lines +636 to +639
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)

)

received_token_amount = self.get_buy_token_amount(bet_amount, direction).amount
Expand Down Expand Up @@ -1104,7 +1109,6 @@ def omen_remove_fund_market_tx(
market_contract.removeFunding(api_keys=api_keys, remove_funding=shares, web3=web3)

conditional_tokens = OmenConditionalTokenContract()
parent_collection_id = build_parent_collection_id()
amount_per_index_set = get_conditional_tokens_balance_for_market(
market, from_address, web3
)
Expand All @@ -1116,7 +1120,6 @@ def omen_remove_fund_market_tx(
result = conditional_tokens.mergePositions(
api_keys=api_keys,
collateral_token_address=market.collateral_token_contract_address_checksummed,
parent_collection_id=parent_collection_id,
conditionId=market.condition.id,
index_sets=market.condition.index_sets,
amount=amount_to_merge,
Expand Down Expand Up @@ -1266,14 +1269,14 @@ def get_buy_outcome_token_amount(
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

) -> float:
"""
Calculates the amount of outcome tokens received for a given investment

Taken from https://github.com/gnosis/conditional-tokens-market-makers/blob/6814c0247c745680bb13298d4f0dd7f5b574d0db/contracts/FixedProductMarketMaker.sol#L264
"""
investment_amount_minus_fees = investment_amount * (1 - fee)
investment_amount_minus_fees = fees.get_bet_size_after_fees(investment_amount)
buy_token_pool_balance = (
yes_outcome_pool_size if buy_direction else no_outcome_pool_size
)
Expand Down
2 changes: 1 addition & 1 deletion prediction_market_agent_tooling/markets/omen/omen_contracts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,10 @@ def mergePositions(
self,
api_keys: APIKeys,
collateral_token_address: ChecksumAddress,
parent_collection_id: HexStr,
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)

web3: Web3 | None = None,
) -> TxReceipt:
return self.send(
Expand Down
7 changes: 7 additions & 0 deletions prediction_market_agent_tooling/markets/polymarket/polymarket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from prediction_market_agent_tooling.markets.agent_market import (
AgentMarket,
FilterBy,
MarketFees,
SortBy,
)
from prediction_market_agent_tooling.markets.data_models import BetAmount, Currency
Expand All @@ -26,6 +27,12 @@ class PolymarketAgentMarket(AgentMarket):
currency: t.ClassVar[Currency] = Currency.USDC
base_url: t.ClassVar[str] = POLYMARKET_BASE_URL

# Based on https://docs.polymarket.com/#fees, there are currently no fees, except for transactions fees.
# However they do have `maker_fee_base_rate` and `taker_fee_base_rate`, but impossible to test out our implementation without them actually taking the fees.
# But then in the new subgraph API, they have `fee: BigInt! (Percentage fee of trades taken by market maker. A 2% fee is represented as 2*10^16)`.
# TODO: Check out the fees while integrating the subgraph API or if we implement placing of bets on Polymarket.
fees: MarketFees = MarketFees.get_zero_fees()

@staticmethod
def from_data_model(model: PolymarketMarketWithPrices) -> "PolymarketAgentMarket":
return PolymarketAgentMarket(
Expand Down
10 changes: 8 additions & 2 deletions prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from prediction_market_agent_tooling.markets.market_fees import MarketFees
from prediction_market_agent_tooling.tools.betting_strategies.utils import SimpleBet


Expand Down Expand Up @@ -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)

) -> SimpleBet:
"""
Calculate the optimal bet amount using the Kelly Criterion for a binary outcome market.
Expand All @@ -86,9 +87,14 @@ def get_kelly_bet_full(
limitations under the License.
```
"""
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

)

check_is_valid_probability(estimated_p_yes)
check_is_valid_probability(confidence)
check_is_valid_probability(fee)

if max_bet == 0:
return SimpleBet(direction=True, size=0)
Expand Down
Loading
Loading