Withdraw to keeping erc20 token on missing places #628
Conversation
WalkthroughThis pull request introduces a new method in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant OmenPos as OmenPosition
participant ContractInit as ContractERC20OnGnosisChain
participant GnosisHelper as to_gnosis_chain_contract
Caller->>OmenPos: get_collateral_token_contract(web3?)
alt web3 provided
OmenPos->>OmenPos: Use provided web3 instance
else no web3
OmenPos->>ContractInit: get_web3()
end
OmenPos->>OmenPos: init_collateral_token_contract(checksummed address, web3)
OmenPos->>GnosisHelper: to_gnosis_chain_contract(result)
GnosisHelper-->>OmenPos: Contract instance
OmenPos-->>Caller: Return ContractERC20OnGnosisChain instance
sequenceDiagram
participant WithdrawFunc as auto_withdraw_collateral_token
participant Logger as Logger
participant Contract as collateral_token_contract
WithdrawFunc->>WithdrawFunc: Check if amount_wei == 0
alt amount_wei is 0
WithdrawFunc->>Logger: Log warning (no withdrawal)
WithdrawFunc-->>Caller: Return early
else amount_wei not 0
WithdrawFunc->>Contract: Check token type
alt token is KEEPING_ERC20_TOKEN
WithdrawFunc->>Logger: Log info (no withdrawal will occur)
else
WithdrawFunc->>Logger: Log info (executing withdrawal)
alt Contract is ContractERC20BaseClass
WithdrawFunc->>Logger: Log info (swapping tokens with details)
end
end
end
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (11)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/create_markets_from_sheet_omen.py (1)
34-34: Changed default behavior for auto_deposit to TrueThis change enables automatic deposits by default when creating markets from a spreadsheet. This provides consistency with other market creation scripts and improves the user experience by automating token handling.
Note that the example command on lines 40-46 still includes the
--auto-depositflag which is now redundant since it's the default behavior.- --auto-deposit \prediction_market_agent_tooling/markets/omen/omen.py (1)
1099-1099: Enhanced position redemption with balance tracking and auto-withdrawalAdded balance tracking before and after position redemption, along with logging and automatic withdrawal of the collateral token if
auto_withdrawis enabled. This ensures users can see how much was redeemed and automatically have those tokens withdrawn to their account.The implementation:
- Tracks original balance before redemption
- Calculates the difference after redemption
- Logs the amount redeemed with the token symbol
- Automatically withdraws the redeemed amount if enabled
Consider adding error handling around the auto-withdrawal process in case it fails, to ensure the redemption operation itself is still considered successful even if the withdrawal encounters issues.
Also applies to: 1107-1120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (9)
prediction_market_agent_tooling/markets/omen/data_models.py(2 hunks)prediction_market_agent_tooling/markets/omen/omen.py(10 hunks)prediction_market_agent_tooling/tools/omen/sell_positions.py(1 hunks)prediction_market_agent_tooling/tools/tokens/auto_withdraw.py(3 hunks)scripts/bet_omen.py(2 hunks)scripts/create_market_omen.py(1 hunks)scripts/create_market_seer.py(1 hunks)scripts/create_markets_from_sheet_omen.py(1 hunks)scripts/sell_all_omen.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: pytest - Python 3.12.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.12.x - Unit Tests
- GitHub Check: pytest - Python 3.11.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.11.x - Unit Tests
- GitHub Check: pytest - Python 3.10.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.10.x - Unit Tests
🔇 Additional comments (25)
scripts/create_market_omen.py (1)
32-32: Changed default behavior for auto_deposit to TrueThis change makes automatic deposits the default behavior when creating a market on Omen. Users won't need to explicitly enable auto-deposit anymore, but can still disable it if needed.
prediction_market_agent_tooling/tools/omen/sell_positions.py (1)
16-16: Changed default behavior for auto_withdraw to TrueThis change makes automatic withdrawals the default behavior when selling positions. This aligns with similar changes throughout the codebase to automate token management by default, improving user experience.
scripts/sell_all_omen.py (1)
13-13: Changed default behavior for auto_withdraw to TrueThis change enables automatic withdrawals by default when selling all positions, consistent with the changes in other files. This improves usability by automating token handling.
scripts/bet_omen.py (2)
26-26: Defaultauto_depositvalue changed toTrueThe default value for
auto_deposithas been changed fromFalsetoTrue, making automatic deposit the default behavior when buying outcomes on Omen.This change aligns with similar changes in other scripts, providing a consistent user experience across the codebase.
64-64: Defaultauto_withdrawvalue changed toTrueThe default value for
auto_withdrawhas been changed fromFalsetoTrue, making automatic withdrawal the default behavior when selling outcomes on Omen.This change improves user experience by automatically withdrawing tokens after selling positions, eliminating the need for a separate withdrawal step in most cases.
prediction_market_agent_tooling/markets/omen/data_models.py (2)
26-30: Added necessary imports for ERC20 contract handlingAdded imports for working with ERC20 token contracts on the Gnosis chain, which will be used by the new
get_collateral_token_contractmethod.
176-185: Added method to retrieve collateral token contractNew method
get_collateral_token_contracthas been added to theOmenPositionclass to easily access the collateral token contract, with appropriate web3 handling.This method enhances the class by providing convenient access to the collateral token contract, which will be useful for operations like withdrawals. The method properly handles the case when no web3 instance is provided by falling back to a default instance.
prediction_market_agent_tooling/tools/tokens/auto_withdraw.py (6)
5-5: Added logger import for enhanced loggingAdded import for the logger module to provide better feedback during token withdrawal operations.
16-19: Added wei_to_xdai import for amount conversion in logsAdded
wei_to_xdaiimport to convert Wei amounts to xDai for more readable logging messages.
35-40: Added early return for zero withdrawal amountsAdded a check to return early if the withdrawal amount is zero, preventing unnecessary operations and providing appropriate warnings.
This is a good defensive programming practice that avoids wasting gas on zero-value transactions and provides clear feedback to the user.
43-45: Added informative logging for keeping token scenarioAdded logging to inform when the collateral token matches the keeping token, explaining why no withdrawal action is being taken.
This improves transparency and helps users understand the system's behavior when tokens are already in the desired format.
53-55: Added logging for ERC4626 token withdrawalsAdded informative logging when withdrawing from an ERC4626 token directly into the keeping token, showing the amount and token symbols.
This provides valuable information for debugging and monitoring token movements.
62-64: Added logging for token swapsAdded informative logging when swapping tokens through DEX, showing the amount and token symbols involved in the transaction.
This enhances observability of token swapping operations, making it easier to track and debug transactions.
prediction_market_agent_tooling/markets/omen/omen.py (12)
83-83: Added new import for KEEPING_ERC20_TOKENThis import is required for the new implementation of the
get_trade_balancemethod at line 411-412, which now uses theKEEPING_ERC20_TOKEN.balanceOfmethod instead of a previously used implementation.
197-197: Changed default behavior to auto-withdraw tokensThe
auto_withdrawparameter is now set toTrueby default in theliquidate_existing_positionsmethod, which will automatically withdraw collateral tokens after selling positions. This change is consistent with the PR's purpose of ensuring tokens are properly withdrawn in all relevant places.
262-262: Changed default behavior to auto-withdraw tokensThe
auto_withdrawparameter is now set toTrueby default in thesell_tokensmethod. This ensures collateral tokens are automatically withdrawn after selling, improving the user experience by eliminating the need for an explicit withdrawal step.
411-412: Updated trade balance implementation to use KEEPING_ERC20_TOKENThe
get_trade_balancemethod now usesKEEPING_ERC20_TOKEN.balanceOffor retrieving the balance. This change makes the implementation more consistent with the token management approach used throughout the PR.
1063-1063: Added auto-withdraw parameter with default value TrueAdded the
auto_withdrawparameter to theomen_redeem_full_position_txmethod with a default value ofTrue. This is consistent with the other methods modified in this PR and ensures tokens are automatically withdrawn after redemption.
1075-1075: Added retrieval of collateral token contractNow retrieving the collateral token contract in the
omen_redeem_full_position_txmethod, which is necessary for the balance tracking and auto-withdrawal functionality added later in the method.
1159-1159: Added auto-withdraw parameter with default value TrueAdded the
auto_withdrawparameter to theomen_remove_fund_market_txmethod with a default value ofTrue. This change is consistent with the other methods and ensures tokens are automatically withdrawn after removing funds from the market.
1166-1166: Updated docstring for better clarityUpdated the documentation to clarify how the collateral token works when removing funding. This improves the understanding of the token flow in the system.
1210-1210: Enhanced fund removal with balance tracking and auto-withdrawalSimilar to the position redemption enhancement, this modification:
- Properly types the balance difference as
wei_type- Improves logging by including the token symbol
- Adds automatic withdrawal of the collateral token if enabled
This ensures users can see how much was withdrawn from liquidity and automatically have those tokens transferred to their account.
Also applies to: 1214-1214, 1217-1224
1229-1229: Added auto-withdraw parameter with default value TrueAdded the
auto_withdrawparameter to theredeem_from_all_user_positionsmethod with a default value ofTrue. This change is consistent with the other methods and ensures tokens are automatically withdrawn after redeeming from all user positions.
1255-1257: Added collateral token contract retrievalAdded code to retrieve the collateral token contract from the position, which is necessary for the balance tracking and auto-withdrawal functionality. This makes the implementation more robust by using the position-specific token contract.
1259-1259: Enhanced user position redemption with balance tracking and auto-withdrawalAdded balance tracking, logging improvements, and auto-withdrawal functionality to the process of redeeming from all user positions:
- Tracks original balance before redemption
- Calculates the difference after redemption
- Logs the amount redeemed with the token symbol
- Automatically withdraws the redeemed amount if enabled
This ensures users can track their redeemed amounts and have them automatically withdrawn without manual intervention.
Also applies to: 1267-1269, 1271-1271, 1274-1281
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/create_market_seer.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
scripts/create_market_seer.py
26-27: SyntaxError: Expected ')', found newline
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest - Python 3.12.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.12.x - Unit Tests
- GitHub Check: pytest - Python 3.11.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.11.x - Unit Tests
- GitHub Check: pytest - Python 3.10.x - Integration with Local Chain
- GitHub Check: pytest - Python 3.10.x - Integration Tests
- GitHub Check: pytest - Python 3.10.x - Unit Tests
🔇 Additional comments (1)
scripts/create_market_seer.py (1)
46-46: 🛠️ Refactor suggestionDefault
auto_depositvalue changed toTrueThe default value for
auto_deposithas been changed fromFalsetoTrue, making automatic deposit of funds the default behavior when creating a market on Seer. However, the docstring still states the default isFalse.Update the docstring to reflect the correct default value:
- auto_deposit (bool, optional): Whether to automatically deposit funds. Defaults to False. + auto_deposit (bool, optional): Whether to automatically deposit funds. Defaults to True.
scripts/create_market_seer.py
Outdated
| auto_deposit: bool = typer.Option(True), | ||
|
|
||
| """ | ||
| Parameters: | ||
| auto_deposit (bool, optional): Whether to automatically deposit funds. Defaults to True. | ||
| """ |
There was a problem hiding this comment.
Critical syntax error: Incorrect placement of docstring.
The docstring is incorrectly placed inside the parameter list, which will cause a syntax error. Docstrings should be placed after the function signature, not within the parameter list.
Fix the syntax by moving the docstring outside the parameter list:
def main(
question: str = typer.Option(),
opening_time: datetime = typer.Option(),
category: str = typer.Option(),
initial_funds: str = typer.Option(),
from_private_key: str = typer.Option(),
safe_address: str = typer.Option(None),
min_bond_xdai: int = typer.Option(0.01),
language: str = typer.Option("en_US"),
outcomes: list[str] = typer.Option(OMEN_BINARY_MARKET_OUTCOMES),
- auto_deposit: bool = typer.Option(True),
-
- """
- Parameters:
- auto_deposit (bool, optional): Whether to automatically deposit funds. Defaults to True.
- """
+ auto_deposit: bool = typer.Option(True),
) -> None:
"""
Creates a market on Seer.
Args:
question (str): The question for the market.
opening_time (datetime): The opening time for the market.
category (str): The category of the market.
initial_funds (str): The initial funds for the market.
from_private_key (str): The private key to use for transactions.
safe_address (str, optional): The safe address for transactions. Defaults to None.
min_bond_xdai (int, optional): The minimum bond in xDai. Defaults to 0.01 xDai.
language (str, optional): The language of the market. Defaults to "en".
outcomes (list[str], optional): The outcomes for the market. Defaults to OMEN_BINARY_MARKET_OUTCOMES.
auto_deposit (bool, optional): Whether to automatically deposit funds. Defaults to False.
Returns:
None
"""Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
26-27: SyntaxError: Expected ')', found newline
deploy please