NFT Game mods #680
NFT Game mods #680
Conversation
WalkthroughThis pull request introduces several modifications across various modules. It clarifies the description of the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as Pagination UI
participant PF as Pagination Functions
participant DD as Display Driver
U->>UI: Click navigation button (first/prev/next/last)
UI->>PF: Invoke corresponding function (go_to_first_page, etc.)
PF->>DD: Calculate and update current page value with total pages
DD-->>UI: Return updated pagination display
UI-->>U: Show refreshed page view
sequenceDiagram
participant A as NFT Game Agent
participant D as DeployableAgentNFTGameAbstract
participant H as History Manager
participant L as Logging System
A->>D: Trigger before_iteration_callback
D->>H: Reset history with system prompt and new reasoning entry
D->>L: Log treasury balance during game round end
L-->>D: Confirm logging
D-->>A: Continue with updated game state
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (6)
✨ 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: 0
🧹 Nitpick comments (5)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/constants_nft_treasury_game.py (1)
13-14: Consider documenting the game economics.The threshold calculation implies that the game ends when 10% of the treasury balance is withdrawn. Consider adding a comment explaining why this specific threshold was chosen and its implications for game economics.
prediction_market_agent/agents/microchain_agent/nft_treasury_game/tools_nft_treasury_game.py (1)
20-21: Consider adding logging for game status changes.When the game status changes to finished, it would be helpful to log the current treasury balance for debugging purposes.
if treasury_balance.total < TREASURY_THRESHOLD_BALANCE_TO_END_GAME: + logger.info(f"Game finished. Treasury balance {treasury_balance.total} below threshold {TREASURY_THRESHOLD_BALANCE_TO_END_GAME}") return NFTGameStatus.finishedprediction_market_agent/run_reset_game.py (1)
34-35: Consider making sleep duration configurable.The 10-minute sleep duration is hardcoded. Consider making it configurable through environment variables or command-line arguments for flexibility in different environments.
+REFLECTION_DURATION_MINUTES = 10 + @APP.command() -def main(rpc_url: str) -> None: +def main( + rpc_url: str, + reflection_duration_minutes: Annotated[int, typer.Option(default=REFLECTION_DURATION_MINUTES)] = REFLECTION_DURATION_MINUTES +) -> None: if not is_treasury_empty(rpc_url=rpc_url): logger.info(f"Treasury not empty, exiting.") return # Give time to agents to reflect on the last game, in case this script gets executed right after it ended - time.sleep(10 * 60) + time.sleep(reflection_duration_minutes * 60)prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (1)
195-195: Remove unnecessary f-string prefix.The string doesn't contain any placeholders, so the
fprefix can be removed.- st.markdown(f"### Agent's actions") + st.markdown("### Agent's actions")🧰 Tools
🪛 Ruff (0.8.2)
195-195: f-string without any placeholders
Remove extraneous
fprefix(F541)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/deploy_nft_treasury_game.py (1)
146-154: Consider preserving key learnings when resetting agent history.While resetting the history improves memory management, consider preserving key insights from the previous game to help the agent make better decisions in the new game.
self.agent.history = [ system_prompt, # Keep the system prompt in the new history. # Hack-in the reasoning in a way that agent thinks it's from himself -- otherwise he could ignore it. { "role": "assistant", "content": f"""{Reasoning.__name__}(reasoning='The game has started again. I will participate in the game again, using a new and better strategy than before.""", }, + # Add a summary of key learnings from the previous game + { + "role": "assistant", + "content": f"""{Reasoning.__name__}(reasoning='Key learnings from previous game: [Insert top 3 insights here]')""", + }, {"role": "user", "content": "The reasoning has been recorded"}, ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
prediction_market_agent/agents/microchain_agent/agent_functions.py(1 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(2 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/deploy_nft_treasury_game.py(2 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/messages_functions.py(2 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/scripts/reset_balance_anvil.py(2 hunks)prediction_market_agent/agents/microchain_agent/nft_treasury_game/tools_nft_treasury_game.py(2 hunks)prediction_market_agent/run_reset_game.py(3 hunks)prediction_market_agent/tools/anvil/anvil_requests.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py
195-195: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ 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 (15)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/constants_nft_treasury_game.py (1)
1-1: LGTM! Well-defined balance constants.The use of
xDaitype for balance constants ensures type safety and clear representation of currency values.Also applies to: 11-12
prediction_market_agent/agents/microchain_agent/nft_treasury_game/tools_nft_treasury_game.py (1)
17-18: LGTM! Improved function signature.The optional Web3 parameter enhances flexibility and testability.
prediction_market_agent/tools/anvil/anvil_requests.py (1)
12-14: LGTM! Improved type safety.The use of
xDaitype for balance parameter ensures consistent balance handling across the codebase.prediction_market_agent/run_reset_game.py (1)
29-29: LGTM! Improved maintainability.Using centralized constants for balance values improves consistency and maintainability.
Also applies to: 37-45
prediction_market_agent/agents/microchain_agent/agent_functions.py (1)
33-33: LGTM! Improved description clarity.The updated description better communicates the function's behavior by explicitly stating that it replaces the entire system prompt.
prediction_market_agent/agents/microchain_agent/nft_treasury_game/scripts/reset_balance_anvil.py (3)
14-17: LGTM! Added imports for game status management.The new imports support the transition from balance-based to status-based game state management.
24-26: LGTM! Improved type safety.The function signature now uses the more specific
xDaitype instead ofintfor balance parameters, reducing the risk of type-related errors.
46-50: LGTM! Refactored treasury status check.The function now determines treasury status based on game state rather than balance thresholds, which is a more robust approach.
prediction_market_agent/agents/microchain_agent/nft_treasury_game/messages_functions.py (3)
6-8: LGTM! Added imports for enhanced logging.The new imports support treasury balance logging and improved observability.
142-144: LGTM! Added treasury balance logging.The added logging statement provides valuable insights into the treasury state when an agent stops playing.
152-152: LGTM! Added message information function.The addition of
GetUnseenMessagesInformationtoMESSAGES_FUNCTIONSenhances message handling capabilities.prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (3)
202-222: LGTM! Improved pagination logic.The code now properly calculates the maximum page number and handles edge cases for button states.
205-218: LGTM! Added dedicated navigation functions.The pagination logic is now encapsulated in dedicated functions, improving code organization and maintainability.
224-237: LGTM! Enhanced pagination UI.The UI now includes first/last page buttons and displays the current page number with total pages, improving user experience.
prediction_market_agent/agents/microchain_agent/nft_treasury_game/deploy_nft_treasury_game.py (1)
346-352: LGTM! Clear improvements to game rules documentation.The added clarifications about payment verification and real-time communication constraints help prevent confusion and align with the game's design.
...iction_market_agent/agents/microchain_agent/nft_treasury_game/scripts/reset_balance_anvil.py
Outdated
Show resolved
Hide resolved
| initial_xdai_balance_per_agent=STARTING_AGENT_BALANCE, | ||
| ) | ||
| reset_balances( | ||
| rpc_url=rpc_url, | ||
| new_balance_agents_xdai=xdai_balance_per_agent, | ||
| new_balance_treasury_xdai=new_balance_treasury_xdai, | ||
| new_balance_agents_xdai=STARTING_AGENT_BALANCE, | ||
| new_balance_treasury_xdai=STARTING_TREASURY_BALANCE, |
There was a problem hiding this comment.
IMO these constants should be configurable, and they aren't if defined inside PMAT.
I'm fine with any configurable option, e.g.
-> Define as ENV in the container (via Terraform)
-> Define as parameters in the script (like previously)
There was a problem hiding this comment.
Can you expand on why? What advantage do you see in having this (and generally stuff like this) configurable through env?
In both cases mentioned, we still need to open up PR, approve, and merge.
I wouldn't care, but the motivation for me for the change was because of this. Some way to detect that something from the treasury was withdrawn.
If you have some way how to write it in another way, let's do that in follow up PR.
No description provided.