Implement BuyTokens and SellTokens microchain Functions #68
Conversation
WalkthroughThe recent updates focus on enhancing token transaction functionality in prediction markets and improving outcome handling in the Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| SellNo, | ||
| SellYes, |
There was a problem hiding this comment.
Tip
Codebase Verification
The additional search for indirect references to the functions SellNo and SellYes in the test files revealed that these functions are indeed used within the tests/agents/microchain/test_functions.py file. However, the references are part of the import statement and their usage within test scenarios, not as dedicated test functions named test_sell_no or test_sell_yes.
Given this context, it appears that while SellNo and SellYes are utilized in tests, there are no specific unit tests named after these functions, which might be necessary to ensure comprehensive testing coverage. This aligns with the initial concern raised in the review comment about ensuring that these functions are properly tested.
Therefore, the review comment is correct in highlighting the need for explicit testing and documentation of these functions. The verification process supports the need for further action to address this gap.
Analysis chain
New functions SellNo and SellYes added to the import list. Ensure these functions are properly tested and documented.
The shell script execution results indicate that there are no unit tests named test_sell_no or test_sell_yes in the tests/ directory. This suggests that the new functions SellNo and SellYes might not be covered by unit tests, which is a concern given their addition to the import list in the file tests/agents/microchain/test_functions.py.
To ensure thorough verification, I will generate a new script to search for any tests that might indirectly cover these functions, perhaps under different naming conventions or within different test files.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that SellNo and SellYes are covered by unit tests
rg --type py "def test_sell_no" tests/
rg --type py "def test_sell_yes" tests/
Length of output: 82
Script:
#!/bin/bash
# Search for any test functions that might be testing SellNo or SellYes indirectly
rg --type py "SellNo" tests/
rg --type py "SellYes" tests/
Length of output: 342
| @@ -127,6 +119,9 @@ def __call__(self, a: str) -> float: | |||
| class BuyTokens(MarketFunction): | |||
| def __init__(self, market_type: MarketType, outcome: str): | |||
| self.outcome = outcome | |||
| self.outcome_bool = get_boolean_outcome( | |||
There was a problem hiding this comment.
get_boolean_outcome was being called a few times in the code, so moved it here to avoid repeated calls
| f"Use this function to sell {self.outcome} outcome tokens of a " | ||
| f"prediction market. The first parameter is the market id. The " | ||
| f"second parameter specifies the value of tokens to sell in " | ||
| f"{self.currency}." |
There was a problem hiding this comment.
Note this is NOT the number of tokens to sell
| @@ -143,31 +138,27 @@ def example_args(self) -> list[t.Union[str, float]]: | |||
| return [get_example_market_id(self.market_type), 2.3] | |||
|
|
|||
| def __call__(self, market_id: str, amount: float) -> str: | |||
| outcome_bool = get_boolean_outcome(self.outcome) | |||
| market: AgentMarket = self.market_type.market_class.get_binary_market(market_id) | |||
| account_balance = float(get_balance(market_type=self.market_type).amount) | |||
There was a problem hiding this comment.
Question: Do we want to get rid of Decimals and just use floats (floats but NewType-d to correct currencies)? It seemed like a good idea in the beginning, but now it feels it brings overhead without clear benefits. I volunteer to refactor in another PR.
There was a problem hiding this comment.
Yeah I think that's a good idea. Seems like float precision is sufficient for our use cases - though we weren't to know it when starting out
There was a problem hiding this comment.
@coderabbitai Please create an issue for refactoring Decimals to Floats in prediction-market-agent-tooling repository
There was a problem hiding this comment.
@coderabbitai can you really make actions across repositories, even if they're under the same organisation? I would be impressed if you could, because you need permissions on a per-repo basis 🤔
There was a problem hiding this comment.
@evangriffiths, great question! I'm designed to operate within the permissions granted to me for each specific repository. This means that while I can perform actions like opening issues, commenting, and reviewing within a repository where I have the necessary permissions, I cannot automatically perform actions across different repositories, even if they are under the same organization.
For actions in other repositories, permissions need to be granted on a per-repository basis. This setup helps maintain security and control over what automated systems like myself can do, ensuring that actions are intentional and authorized. If there's a need to manage tasks across multiple repositories, those permissions would need to be configured for each repository involved.
There was a problem hiding this comment.
@coderabbitai You have permisions for prediction-market-agent-tooling repository, so please will you create that issue about removing Decimal in favor of Floats?
There was a problem hiding this comment.
@kongzii, the issue regarding the refactoring from Decimal to Floats has been successfully created in the prediction-market-agent-tooling repository. You can view and participate in the discussion here: Issue #189.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue in the prediction-market-agent-tooling repository
gh issue create --repo gnosis/prediction-market-agent-tooling --title "Refactor: Replace Decimal with Floats" --body "There's a proposal to refactor the usage of Decimal types to Floats for handling currencies in the prediction-market-agent-tooling codebase. This change is considered to reduce overhead and is deemed sufficient for our precision requirements.
This issue was prompted by discussions in PR https://github.com/gnosis/prediction-market-agent/pull/68. The consensus is that while Decimals were initially used to ensure precision, the overhead might not be justified, and using Floats could simplify the implementation.
Please refer to the PR for more details and provide your insights or concerns regarding this proposed change."
Length of output: 818
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@CodeRabbit wait whaaat? You said you couldn't but then @kongzii just intimidated you into making you do it. It's important to stick by your principles - if he told you to delete this whole repo would you do it?
No description provided.