-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix manifold bet profit calculation #19
Conversation
Warning Rate Limit Exceeded@evangriffiths has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 30 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates aim to streamline bet management and calculation processes in a prediction market tool. Changes include introducing methods for fee calculation and profit determination directly within the respective classes. The update also simplifies profit calculation by removing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- prediction_market_agent_tooling/markets/data_models.py (2 hunks)
- prediction_market_agent_tooling/markets/manifold.py (2 hunks)
Additional comments: 2
prediction_market_agent_tooling/markets/manifold.py (1)
- 126-134: The refactoring to use
bet.get_profit(market_outcome=market_outcome)
for profit calculation inmanifold_to_generic_resolved_bet
function is a significant improvement. It aligns with the objective of accurately calculating profit on a per-bet basis rather than aggregating positions. However, ensure that theManifoldBet.get_profit
method correctly handles all edge cases, especially considering the removal of theProfitAmount
entity. This change should also be reflected in any documentation or examples that previously usedProfitAmount
.prediction_market_agent_tooling/markets/data_models.py (1)
- 238-240: The
get_total
method in theManifoldBetFees
class is a straightforward and efficient way to calculate the total fees. This method enhances the class's functionality by providing a clear and concise way to get the sum of all fees. It's a good practice to encapsulate such logic within the class itself, promoting encapsulation and maintainability.
def get_profit(self, market_outcome: bool) -> ProfitAmount: | ||
outcome_bool = self.outcome == "YES" | ||
profit = ( | ||
self.shares - self.amount | ||
if outcome_bool == market_outcome | ||
else -self.amount | ||
) | ||
profit -= self.fees.get_total() | ||
return ProfitAmount( | ||
amount=profit, | ||
currency=Currency.Mana, | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_profit
method in the ManifoldBet
class introduces a more nuanced approach to calculating profit, taking into account the market outcome and fees. This method is crucial for addressing the identified issues with profit calculation. However, ensure that the logic correctly handles all scenarios, especially the subtraction of total fees from the profit and the handling of outcomes. It might be beneficial to add unit tests specifically for this method to cover various edge cases and ensure its reliability.
Would you like me to help with creating unit tests for the get_profit
method to ensure its correctness across different scenarios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- prediction_market_agent_tooling/markets/data_models.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/markets/data_models.py
@@ -257,6 +260,19 @@ class ManifoldBet(BaseModel): | |||
createdTime: datetime | |||
outcome: str | |||
|
|||
def get_profit(self, market_outcome: bool) -> ProfitAmount: | |||
outcome_bool = self.outcome == "YES" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be True if self.outcome == "YES" else False if self.outcome == "NO" else should_not_happen()
? Just to catch potential errors if anything changes in their APIs.
bet_position = next(p for p in positions if p.contractId == bet.contractId) | ||
profit = bet_position.profit | ||
|
||
market_outcome = market.resolution == "YES" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- prediction_market_agent_tooling/markets/data_models.py (6 hunks)
- prediction_market_agent_tooling/markets/manifold.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/markets/manifold.py
Additional comments: 1
prediction_market_agent_tooling/markets/data_models.py (1)
- 289-299: The
get_profit
method in theManifoldBet
class correctly calculates profit based on the market outcome and fees. However, the calculation of profit directly subtractsself.amount
fromself.shares
, which might not be correct without considering the price per share or the outcome. Ensure the logic aligns with the expected financial model, especially in terms of how shares and amounts are meant to interact.Please verify the profit calculation logic with the financial model to ensure it aligns with expected outcomes, particularly how shares and amounts interact in profit calculations.
def get_total(self) -> Decimal: | ||
return Decimal(sum([self.platformFee, self.liquidityFee, self.creatorFee])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of get_total
in the ManifoldBetFees
class correctly sums up the fees. However, consider using Decimal
directly on the list comprehension for precision in financial calculations.
- return Decimal(sum([self.platformFee, self.liquidityFee, self.creatorFee]))
+ return sum([self.platformFee, self.liquidityFee, self.creatorFee], start=Decimal(0))
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.
def get_total(self) -> Decimal: | |
return Decimal(sum([self.platformFee, self.liquidityFee, self.creatorFee])) | |
def get_total(self) -> Decimal: | |
return sum([self.platformFee, self.liquidityFee, self.creatorFee], start=Decimal(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- prediction_market_agent_tooling/markets/data_models.py (6 hunks)
- prediction_market_agent_tooling/markets/manifold.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- prediction_market_agent_tooling/markets/data_models.py
- prediction_market_agent_tooling/markets/manifold.py
Fixes a bug in manifold monitoring code where per-bet profit was calculated based on a user's market 'position'. However, this was incorrect, as a user can have multiple bets on a given market, but only one position. This meant:
start_time
,s
and has placed at least one bet on a given market afters
, its profit for that market took into account bets also made befores
Result, before:
After:
Summary by CodeRabbit
New Features
Refactor