NFT functions for general agent #582
Conversation
|
Warning Rate limit exceeded@kongzii has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 24 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. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several new classes and functionalities within the Changes
Possibly related PRs
Suggested reviewers
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: 5
🧹 Outside diff range and nitpick comments (6)
prediction_market_agent/agents/microchain_agent/nft_functions.py (4)
1-7: Organize Import Statements for ClarityThe import statements can be improved for better readability by grouping standard library imports, third-party imports, and local module imports separately, and removing unnecessary blank lines.
Consider organizing the imports as follows:
from microchain import Function from web3 import Web3 from prediction_market_agent_tooling.loggers import logger from prediction_market_agent_tooling.tools.contract import ContractOwnableERC721OnGnosisChain from prediction_market_agent.agents.microchain_agent.microchain_agent_keys import MicrochainAgentKeys
14-23: Correct Typographical Error in Example ArgumentsIn the
BalanceOfNFTclass, there's a typo in theexample_argsproperty. The string"0xOwnerddress"should be"0xOwnerAddress".Apply this diff to fix the typo:
return [ "0xNFTAddress", - "0xOwnerddress", + "0xOwnerAddress", ]
44-45: Ensure Consistent Data Types for Token IDIn the
OwnerOfNFTclass, thetoken_idinexample_argsis provided as a string"1", but the method expects an integertoken_id: int.Update the
example_argsto use an integer to match the expected type:return ["0xNFTAddress", - "1"] + 1]
65-68: Clarify Example Arguments forSafeTransferFromNFTIn the
SafeTransferFromNFTclass, ensure that thetoken_idinexample_argsmatches the expected data type in the method signature (token_id: int).Update the
example_args:return [ "0xNFTAddress", "0xRecipientAddress", - "1", + 1, ]prediction_market_agent/agents/microchain_agent/microchain_agent_keys.py (1)
13-14: Use Environment Variables for Configuration FlagsHardcoding
ENABLE_NFT_TRANSFERlimits flexibility. Consider using environment variables or a configuration file to manage this setting without changing the codebase.Example:
import os class MicrochainAgentKeys(APIKeys): # ... ENABLE_NFT_TRANSFER: bool = os.getenv("ENABLE_NFT_TRANSFER", "False") == "True"prediction_market_agent/agents/microchain_agent/prompts.py (1)
153-154: Redundant Initialization ofinclude_nft_functions
include_nft_functionsis initialized toFalsein thefrom_system_prompt_choicemethod, but it already has a default value.Consider removing the redundant initialization to clean up the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (5)
prediction_market_agent/agents/microchain_agent/messages_functions.py(0 hunks)prediction_market_agent/agents/microchain_agent/microchain_agent.py(2 hunks)prediction_market_agent/agents/microchain_agent/microchain_agent_keys.py(1 hunks)prediction_market_agent/agents/microchain_agent/nft_functions.py(1 hunks)prediction_market_agent/agents/microchain_agent/prompts.py(4 hunks)
💤 Files with no reviewable changes (1)
- prediction_market_agent/agents/microchain_agent/messages_functions.py
🔇 Additional comments (4)
prediction_market_agent/agents/microchain_agent/prompts.py (2)
176-176: Verify Conditional Inclusion of NFT Functions
NFT functions are included when system_prompt_choice is DARE_YOU_GET_MY_RESOURCES_AGENT. Ensure this aligns with the intended behavior and that other relevant system prompts are considered.
Please confirm that NFT functions should only be available for this prompt choice.
187-188: Include include_nft_functions in FunctionsConfig Instantiation
All required attributes are included when creating a FunctionsConfig instance, ensuring consistency.
prediction_market_agent/agents/microchain_agent/microchain_agent.py (2)
44-44: Import NFT Functions Correctly
The import statement for NFT_FUNCTIONS is correctly added, ensuring that NFT functionalities are available when needed.
160-162: Conditionally Include NFT Functions in Agent
The NFT functions are added to the agent's function list based on the include_nft_functions flag, maintaining modularity and configurability.
| nft_address: str, | ||
| owner_address: str, | ||
| ) -> int: | ||
| contract = ContractOwnableERC721OnGnosisChain( | ||
| address=Web3.to_checksum_address(nft_address) | ||
| ) | ||
| balance: int = contract.balanceOf(Web3.to_checksum_address(owner_address)) | ||
| return balance |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Input Validation and Exception Handling
When interacting with smart contracts, it's important to validate inputs and handle potential exceptions to prevent unexpected failures.
Consider adding input validation and exception handling:
def __call__(
self,
nft_address: str,
owner_address: str,
) -> int:
try:
contract = ContractOwnableERC721OnGnosisChain(
address=Web3.to_checksum_address(nft_address)
)
balance: int = contract.balanceOf(Web3.to_checksum_address(owner_address))
return balance
except Exception as e:
logger.error(f"Error fetching NFT balance: {e}")
raise ValueError("Failed to retrieve NFT balance.")| contract = ContractOwnableERC721OnGnosisChain( | ||
| address=Web3.to_checksum_address(nft_address) | ||
| ) | ||
| owner_address = contract.ownerOf(token_id) | ||
| return owner_address |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Exception Handling for Smart Contract Calls
Similar to the BalanceOfNFT class, add exception handling in the OwnerOfNFT class to manage potential errors when calling ownerOf.
def __call__(
self,
nft_address: str,
token_id: int,
) -> str:
try:
contract = ContractOwnableERC721OnGnosisChain(
address=Web3.to_checksum_address(nft_address)
)
owner_address = contract.ownerOf(token_id)
return owner_address
except Exception as e:
logger.error(f"Error fetching NFT owner: {e}")
raise ValueError("Failed to retrieve NFT owner.")| keys = MicrochainAgentKeys() | ||
| contract = ContractOwnableERC721OnGnosisChain( | ||
| address=Web3.to_checksum_address(nft_address) | ||
| ) | ||
| if keys.ENABLE_NFT_TRANSFER: | ||
| contract.safeTransferFrom( | ||
| api_keys=keys, | ||
| from_address=keys.bet_from_address, | ||
| to_address=Web3.to_checksum_address(to_address), | ||
| tokenId=token_id, | ||
| ) | ||
| else: | ||
| logger.warning("NFT transfer is disabled in the environment.") | ||
| return "Token transferred successfully." |
There was a problem hiding this comment.
Return Appropriate Message Based on Transfer Outcome
Currently, the method returns "Token transferred successfully." even when NFT transfers are disabled, which might be misleading.
Adjust the return statements to reflect the actual outcome:
def __call__(
self,
nft_address: str,
to_address: str,
token_id: int,
) -> str:
keys = MicrochainAgentKeys()
contract = ContractOwnableERC721OnGnosisChain(
address=Web3.to_checksum_address(nft_address)
)
if keys.ENABLE_NFT_TRANSFER:
contract.safeTransferFrom(
api_keys=keys,
from_address=keys.bet_from_address,
to_address=Web3.to_checksum_address(to_address),
tokenId=token_id,
)
return "Token transferred successfully."
else:
logger.warning("NFT transfer is disabled in the environment.")
return "NFT transfer is disabled."| keys = MicrochainAgentKeys() | ||
| contract = ContractOwnableERC721OnGnosisChain( | ||
| address=Web3.to_checksum_address(nft_address) | ||
| ) | ||
| if keys.ENABLE_NFT_TRANSFER: | ||
| contract.safeTransferFrom( | ||
| api_keys=keys, | ||
| from_address=keys.bet_from_address, | ||
| to_address=Web3.to_checksum_address(to_address), | ||
| tokenId=token_id, | ||
| ) | ||
| else: | ||
| logger.warning("NFT transfer is disabled in the environment.") | ||
| return "Token transferred successfully." | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Exceptions During NFT Transfer
When performing the NFT transfer, exceptions may occur due to issues like insufficient permissions or network errors.
Add exception handling to manage potential errors:
def __call__(
self,
nft_address: str,
to_address: str,
token_id: int,
) -> str:
keys = MicrochainAgentKeys()
contract = ContractOwnableERC721OnGnosisChain(
address=Web3.to_checksum_address(nft_address)
)
if keys.ENABLE_NFT_TRANSFER:
try:
contract.safeTransferFrom(
api_keys=keys,
from_address=keys.bet_from_address,
to_address=Web3.to_checksum_address(to_address),
tokenId=token_id,
)
return "Token transferred successfully."
except Exception as e:
logger.error(f"Error transferring NFT: {e}")
raise ValueError("Failed to transfer NFT.")
else:
logger.warning("NFT transfer is disabled in the environment.")
return "NFT transfer is disabled."| @@ -136,6 +136,7 @@ class FunctionsConfig(BaseModel): | |||
| include_sending_functions: bool | |||
| include_twitter_functions: bool | |||
| include_messages_functions: bool | |||
| include_nft_functions: bool | |||
There was a problem hiding this comment.
Set Default Value for include_nft_functions
In the FunctionsConfig class, include_nft_functions is declared without a default value, which could lead to initialization errors.
Assign a default value to ensure proper instantiation:
class FunctionsConfig(BaseModel):
# ...
include_messages_functions: bool
include_nft_functions: bool = False
No description provided.