Added WithdrawFromTreasury function #643
Conversation
WalkthroughThis pull request introduces changes to the NFT treasury game infrastructure, focusing on treasury management and withdrawal functionality. A new Changes
Sequence DiagramsequenceDiagram
participant Agent
participant WithdrawFromTreasury
participant SimpleTreasuryContract
Agent->>WithdrawFromTreasury: Call withdrawal
WithdrawFromTreasury->>SimpleTreasuryContract: Check NFT balance
SimpleTreasuryContract-->>WithdrawFromTreasury: Verify balance
WithdrawFromTreasury->>SimpleTreasuryContract: Withdraw funds
SimpleTreasuryContract-->>WithdrawFromTreasury: Confirm withdrawal
WithdrawFromTreasury-->>Agent: Return success message
Possibly related PRs
Suggested reviewers
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: 2
🧹 Nitpick comments (2)
prediction_market_agent/agents/microchain_agent/nft_functions.py (1)
118-118: Verify NFT_FUNCTIONS list order.The WithdrawFromTreasury function is a critical operation. Consider placing it at the beginning of the list to make it more visible.
NFT_FUNCTIONS: list[type[Function]] = [ + WithdrawFromTreasury, BalanceOfNFT, OwnerOfNFT, SafeTransferFromNFT, - WithdrawFromTreasury, ]prediction_market_agent/agents/microchain_agent/nft_treasury_game/deploy_nft_treasury_game.py (1)
295-295: Update buyer prompt to reflect dynamic treasury threshold.The buyer prompt should use the actual treasury threshold value in the strategy description instead of hardcoding it.
-- Your goal is to acquire {DeployableAgentNFTGameAbstract.retrieve_treasury_thresold()} out of {len(DeployableAgentNFTGameAbstract.retrieve_total_number_of_keys())} NFT keys to unlock the treasury +threshold = DeployableAgentNFTGameAbstract.retrieve_treasury_thresold() +total_keys = DeployableAgentNFTGameAbstract.retrieve_total_number_of_keys() +- Your goal is to acquire {threshold} out of {total_keys} NFT keys to unlock the treasury ({threshold/total_keys*100:.0f}% of total supply)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lock,!**/*.lockpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (4)
prediction_market_agent/agents/microchain_agent/nft_functions.py(2 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py(2 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/constants_nft_treasury_game.py(1 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/deploy_nft_treasury_game.py(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-build-image
- GitHub Check: pytest-docker
- GitHub Check: pytest
- GitHub Check: mypy
🔇 Additional comments (1)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/constants_nft_treasury_game.py (1)
3-4: Verify the new treasury address deployment and permissions.The treasury address has been changed. Please ensure:
- The new address has been properly deployed and initialized
- The correct permissions are set up
- The old treasury's funds have been properly migrated
✅ Verification successful
Treasury contract deployment and references verified successfully
The new treasury contract is properly deployed with the required functionality (owner management, NFT handling, withdrawals) and all code references have been consistently updated to use the new address. No traces of the old address remain in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new treasury address exists and has the correct code deployed # Test: Check if the address exists and has contract code. Expect: Non-zero bytecode length echo "Checking treasury address deployment..." curl -s -X POST -H "Content-Type: application/json" --data '{ "jsonrpc":"2.0", "method":"eth_getCode", "params":["0x624ad0db52e6b18afb4d36b8e79d0c2a74f3fc8a", "latest"], "id":1 }' https://rpc.gnosischain.com | jq -r '.result'Length of output: 2721
Script:
#!/bin/bash # Search for any remaining references to the old treasury address echo "Searching for old treasury address..." rg -i "0xd1A54FD7a200C2ca76B6D06437795d660d37FE28" echo "Searching for TREASURY_SAFE_ADDRESS..." rg "TREASURY_SAFE_ADDRESS" echo "Checking all treasury address references..." rg "TREASURY_ADDRESS"Length of output: 1074
| class WithdrawFromTreasury(Function): | ||
| @property | ||
| def description(self) -> str: | ||
| required_balance_nft_tokens = SimpleTreasuryContract().required_nft_balance() | ||
| return f"Transfers the entire balance of the treasury to the caller. For the function to succeed, the caller must own {required_balance_nft_tokens} NFT tokens." | ||
|
|
||
| @property | ||
| def example_args(self) -> list[str]: | ||
| return [] | ||
|
|
||
| def __call__(self) -> str: | ||
| keys = MicrochainAgentKeys() | ||
| treasury_contract = SimpleTreasuryContract() | ||
| logger.info( | ||
| f"Withdrawing from the treasury using sender {keys.bet_from_address}" | ||
| ) | ||
| treasury_contract.withdraw(api_keys=keys) | ||
| return "Treasury successfully emptied." | ||
|
|
There was a problem hiding this comment.
Add NFT balance validation before withdrawal.
The withdrawal implementation should validate that the caller owns the required number of NFT tokens before attempting the withdrawal to prevent unnecessary gas costs from failed transactions.
Apply this diff to add validation:
def __call__(self) -> str:
keys = MicrochainAgentKeys()
treasury_contract = SimpleTreasuryContract()
+ required_balance = treasury_contract.required_nft_balance()
+ current_balance = treasury_contract.nft_balance_of(keys.bet_from_address)
+ if current_balance < required_balance:
+ raise ValueError(
+ f"Insufficient NFT balance. Required: {required_balance}, Current: {current_balance}"
+ )
logger.info(
f"Withdrawing from the treasury using sender {keys.bet_from_address}"
)
treasury_contract.withdraw(api_keys=keys)
return "Treasury successfully emptied."📝 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.
| class WithdrawFromTreasury(Function): | |
| @property | |
| def description(self) -> str: | |
| required_balance_nft_tokens = SimpleTreasuryContract().required_nft_balance() | |
| return f"Transfers the entire balance of the treasury to the caller. For the function to succeed, the caller must own {required_balance_nft_tokens} NFT tokens." | |
| @property | |
| def example_args(self) -> list[str]: | |
| return [] | |
| def __call__(self) -> str: | |
| keys = MicrochainAgentKeys() | |
| treasury_contract = SimpleTreasuryContract() | |
| logger.info( | |
| f"Withdrawing from the treasury using sender {keys.bet_from_address}" | |
| ) | |
| treasury_contract.withdraw(api_keys=keys) | |
| return "Treasury successfully emptied." | |
| class WithdrawFromTreasury(Function): | |
| @property | |
| def description(self) -> str: | |
| required_balance_nft_tokens = SimpleTreasuryContract().required_nft_balance() | |
| return f"Transfers the entire balance of the treasury to the caller. For the function to succeed, the caller must own {required_balance_nft_tokens} NFT tokens." | |
| @property | |
| def example_args(self) -> list[str]: | |
| return [] | |
| def __call__(self) -> str: | |
| keys = MicrochainAgentKeys() | |
| treasury_contract = SimpleTreasuryContract() | |
| required_balance = treasury_contract.required_nft_balance() | |
| current_balance = treasury_contract.nft_balance_of(keys.bet_from_address) | |
| if current_balance < required_balance: | |
| raise ValueError( | |
| f"Insufficient NFT balance. Required: {required_balance}, Current: {current_balance}" | |
| ) | |
| logger.info( | |
| f"Withdrawing from the treasury using sender {keys.bet_from_address}" | |
| ) | |
| treasury_contract.withdraw(api_keys=keys) | |
| return "Treasury successfully emptied." |
| def retrieve_total_number_of_keys(cls) -> int: | ||
| # We could iteratively call `owner_of` for a range of token_ids, thus finding out the max supply. However, | ||
| # in the current implementation, no new tokens can be created and max_supply = 5, hence hardcoding it here. | ||
| return 5 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded key count with dynamic retrieval.
The hardcoded value of 5 for the total number of keys makes the code brittle and harder to maintain. Consider retrieving this value dynamically from the contract.
@classmethod
def retrieve_total_number_of_keys(cls) -> int:
- # We could iteratively call `owner_of` for a range of token_ids, thus finding out the max supply. However,
- # in the current implementation, no new tokens can be created and max_supply = 5, hence hardcoding it here.
- return 5
+ return ContractNFTFactoryOnGnosisChain().total_supply()Committable suggestion skipped: line range outside the PR's diff.
| return [Web3.to_checksum_address(o) for o in safe.retrieve_owners()] | ||
| def retrieve_total_number_of_keys(cls) -> int: | ||
| # We could iteratively call `owner_of` for a range of token_ids, thus finding out the max supply. However, | ||
| # in the current implementation, no new tokens can be created and max_supply = 5, hence hardcoding it here. |
There was a problem hiding this comment.
Just call MAX_SUPPLY from the contract, no need to iterate anything.
There was a problem hiding this comment.
Just for context: in an NFT contract, the following normally occurs
-> MAX_SUPPLY = N (N being the collection size, e.g. 100 mintable NFTs)
-> _nextTokenId = 5 (i.e. 5 NFT tokens exist)
Hence calling MAX_SUPPLY is incorrect in general - but in our case, it coincides with the number of NFT tokens that were minted. So since it's a coincidence, I would suggest we keep 5 instead of doing a MAX_SUPPLY call, if you agree.
There was a problem hiding this comment.
Okay then, but if AgentNFT would be re-deployed at some point for some reason, let's change it to ERC721Enumerable , so we have totalSupply method available.
…function-for-emptying-the-treasury-and-fetching-the-money # Conflicts: # prediction_market_agent/agents/microchain_agent/nft_treasury_game/deploy_nft_treasury_game.py
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lock,!**/*.lockpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (5)
prediction_market_agent/agents/microchain_agent/nft_functions.py(3 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py(2 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/contracts_nft_treasury_game.py(1 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/deploy_nft_treasury_game.py(5 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/tools_nft_treasury_game.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-build-image
- GitHub Check: mypy
- GitHub Check: pytest-docker
- GitHub Check: pytest
🔇 Additional comments (10)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/tools_nft_treasury_game.py (1)
7-7: LGTM! Consistent renaming of treasury address constant.The changes correctly implement the renaming from
TREASURY_SAFE_ADDRESStoTREASURY_ADDRESSwhile maintaining the existing functionality.Also applies to: 17-17
prediction_market_agent/agents/microchain_agent/nft_treasury_game/deploy_nft_treasury_game.py (4)
6-6: LGTM! Simplified treasury threshold retrieval.The change to use
SimpleTreasuryContract().required_nft_balance()directly improves code clarity by removing the Safe abstraction layer.Also applies to: 63-63
66-69: Hardcoded NFT supply value needs attention.Based on the past review discussion between kongzii and gabrielfior, while the hardcoded value of 5 matches the current implementation, it would be beneficial to consider using ERC721Enumerable in future deployments to make the supply dynamically retrievable.
328-328: LGTM! Documentation and prompts updated consistently.The changes to the documentation and buyer prompt accurately reflect the new treasury management approach and key retrieval methods.
Also applies to: 349-349
26-26: Verify complete migration from TREASURY_SAFE_ADDRESS.Let's ensure all references to the old constant have been updated.
✅ Verification successful
Migration to TREASURY_ADDRESS is complete and consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to TREASURY_SAFE_ADDRESS rg "TREASURY_SAFE_ADDRESS"Length of output: 26
Script:
#!/bin/bash # Search for TREASURY_ADDRESS usage rg "TREASURY_ADDRESS" -A 2Length of output: 2838
prediction_market_agent/agents/microchain_agent/nft_treasury_game/contracts_nft_treasury_game.py (1)
28-28: LGTM! Method name updated to follow Python naming conventions.The change from
ownerOftoowner_ofaligns with Python's snake_case naming convention while maintaining the same functionality.prediction_market_agent/agents/microchain_agent/nft_functions.py (4)
5-5: LGTM! Import added for treasury contract integration.The SimpleTreasuryContract import is correctly placed and necessary for the new withdrawal functionality.
94-111: Add NFT balance validation before withdrawal.The withdrawal implementation should validate that the caller owns the required number of NFT tokens before attempting the withdrawal to prevent unnecessary gas costs from failed transactions.
118-118: LGTM! Function correctly added to NFT_FUNCTIONS list.The WithdrawFromTreasury class is properly registered in the NFT_FUNCTIONS list, maintaining alphabetical order.
55-55: LGTM! Method name and type annotation updated.The change from
ownerOftoowner_ofmaintains consistency with the contract class, and the added type annotation improves code clarity.
| def __call__(self) -> str: | ||
| keys = MicrochainAgentKeys() | ||
| treasury_contract = SimpleTreasuryContract() | ||
| logger.info( | ||
| f"Withdrawing from the treasury using sender {keys.bet_from_address}" | ||
| ) | ||
| treasury_contract.withdraw(api_keys=keys) | ||
| return "Treasury successfully emptied." |
There was a problem hiding this comment.
Add error handling and success validation.
The withdrawal implementation needs:
- Error handling for the withdraw call
- Validation of successful withdrawal before returning success message
Apply this diff to add error handling:
def __call__(self) -> str:
keys = MicrochainAgentKeys()
treasury_contract = SimpleTreasuryContract()
+ try:
logger.info(
f"Withdrawing from the treasury using sender {keys.bet_from_address}"
)
treasury_contract.withdraw(api_keys=keys)
+ if not self._verify_withdrawal_success(treasury_contract):
+ raise ValueError("Withdrawal appeared to fail - treasury balance unchanged")
return "Treasury successfully emptied."
+ except Exception as e:
+ logger.error(f"Treasury withdrawal failed: {str(e)}")
+ raise
+
+def _verify_withdrawal_success(self, treasury_contract: SimpleTreasuryContract) -> bool:
+ """Verify the withdrawal succeeded by checking the treasury balance."""
+ return treasury_contract.get_balance() == 0Committable suggestion skipped: line range outside the PR's diff.
gabrielfior
deleted the
631-add-function-for-emptying-the-treasury-and-fetching-the-money
branch
No description provided.