Refactor mcp-atp#11
Conversation
🦋 Changeset detectedLatest commit: 4d45995 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Hello @GPSxtreme, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team,
Gemini here, providing a summary of this pull request authored by GPSxtreme. The primary goal of this PR is to refactor the mcp-atp package to improve its structure, configuration management, and simplify certain parts of the codebase, particularly the swap service.
The key changes involve introducing a centralized configuration module using dotenv and zod for environment variable validation, removing the distinction between development and production ABIs and API endpoints in favor of configurable values, and streamlining the SwapService logic. The README and tool parameter descriptions have also been updated to reflect these changes.
Highlights
- Centralized Configuration: A new
src/config.tsfile is introduced usingdotenvto load environment variables andzodfor schema validation. This centralizes configuration logic and provides type safety and default values for critical settings like API URLs, router addresses, and token addresses. - Simplified ABIs and Environment Handling: The distinction between development and production ABIs and API endpoints (
ATP_USE_DEVenvironment variable) has been removed. Instead, the necessary addresses and API URL are now configurable via environment variables (ATP_AGENT_ROUTER_ADDRESS,ATP_BASE_TOKEN_ADDRESS,ATP_API_URL) with defaults provided in the new config file. A minimal router ABI (ROUTER_MINIMAL_ABI) is introduced, containing only thebuyandsellfunctions needed by the swap service. - Refactored Swap Service: The
SwapServicehas been updated to use the new configuration module and the minimal ABI. Thebuyandsellmethods are simplified, leveraging a new privateapproveTokenAllowancehelper method to handle token approvals before executing the swap. The methods now consistently use the 3-argumentbuy/sellfunctions on the router contract. - Tool Parameter Updates: The
buyandselltools'amountparameter descriptions and validation regex have been updated to explicitly state that the amount should be provided as a string representing the value in wei units, rather than a standard decimal number. - Documentation and Dependency Updates: The README is updated to remove references to
ATP_USE_DEVand correct the wallet private key environment variable name toATP_WALLET_PRIVATE_KEY. Thedotenvpackage is added as a dependency.
Changelog
Click here to see the changelog
- .changeset/wise-trains-fix.md
- Added a changeset entry documenting the updated router ABI, removal of the dev router ABI, and the new environment variable configuration options.
- README.md
- Removed documentation for the
ATP_USE_DEVenvironment variable. - Corrected the required environment variable for wallet private key from
WALLET_PRIVATE_KEYtoATP_WALLET_PRIVATE_KEYin the main description and example configurations.
- Removed documentation for the
- package.json
- Added
dotenvdependency (^16.5.0).
- Added
- pnpm-lock.yaml
- Updated lock file to include
dotenvdependency.
- Updated lock file to include
- src/config.ts
- New File: Created a configuration module using
dotenvandzod. - Defines and validates environment variables:
ATP_WALLET_PRIVATE_KEY,ATP_API_URL(with default),ATP_API_KEY,ATP_AGENT_ROUTER_ADDRESS(with default), andATP_BASE_TOKEN_ADDRESS(with default). - Exports the parsed environment configuration (
env) and default pagination constants (DEFAULT_PAGE,DEFAULT_LIMIT,MAX_LIMIT).
- New File: Created a configuration module using
- src/lib/router.abi.minimal.ts
- New File: Created a minimal ABI (
ROUTER_MINIMAL_ABI) containing only thebuyandsellfunction definitions.
- New File: Created a minimal ABI (
- src/services/agent-logs.ts
- Updated imports to use constants from
../config.jsinstead of../constants.js. - Modified API endpoint construction to use
env.ATP_API_URLinstead of conditional logic based onprocess.env.ATP_USE_DEV.
- Updated imports to use constants from
- src/services/agent-positions.ts
- Updated imports to use configuration from
../config.jsinstead of../constants.js. - Modified API endpoint construction to use
env.ATP_API_URLinstead of conditional logic based onprocess.env.ATP_USE_DEV.
- Updated imports to use configuration from
- src/services/agent-stats.ts
- Updated imports to use configuration from
../config.jsinstead of../constants.js. - Modified API endpoint construction to use
env.ATP_API_URLinstead of conditional logic based onprocess.env.ATP_USE_DEV.
- Updated imports to use configuration from
- src/services/get-agents.ts
- Updated imports to use configuration from
../config.jsinstead of../constants.js. - Modified API endpoint construction to use
env.ATP_API_URLinstead of conditional logic based onprocess.env.ATP_USE_DEV. - Updated the API endpoint path from
/agentsto/agents/top.
- Updated imports to use configuration from
- src/services/swap.ts
- Removed imports for old constants and dev/prod ABIs.
- Imported
envfrom../config.jsandROUTER_MINIMAL_ABI. - Updated constructor to initialize
routerAddress,baseTokenAddress, androuterAbiusing values from theenvconfiguration. - Introduced a private
approveTokenAllowancemethod to handle ERC20 approvals. - Refactored
buyandsellmethods to use the newapproveTokenAllowancemethod and consistently call the router contract with the 3-argumentbuy/sellfunctions (including0nforminAmountOut). - Removed conditional logic based on
process.env.ATP_USE_DEVwithin thebuyandsellmethods.
- src/tools/add-agent-log.ts
- Imported
envfrom../config.js. - Updated to use
env.ATP_API_KEYinstead ofprocess.env.ATP_API_KEY. - Formatted the success return message with line breaks for better readability.
- Imported
- src/tools/buy.ts
- Imported
envfrom../config.js. - Updated the
amountparameter schema regex (/^d+$/) and description to require a valid integer string representing the amount in wei. - Updated to use
env.ATP_WALLET_PRIVATE_KEYinstead ofprocess.env.WALLET_PRIVATE_KEY. - Formatted the success return message with line breaks for better readability.
- Imported
- src/tools/get-agent-logs.ts
- Updated imports to use constants from
../config.jsinstead of../constants.js.
- Updated imports to use constants from
- src/tools/get-agent-positions.ts
- Removed import for
zod. - Imported
envfrom../config.js. - Updated to use
env.ATP_WALLET_PRIVATE_KEYinstead ofprocess.env.ATP_WALLET_PRIVATE_KEY.
- Removed import for
- src/tools/sell.ts
- Imported
envfrom../config.js. - Updated the
amountparameter schema regex (/^d+$/) and description to require a valid integer string representing the amount in wei. - Updated to use
env.ATP_WALLET_PRIVATE_KEYinstead ofprocess.env.WALLET_PRIVATE_KEY. - Formatted the success return message with line breaks for better readability.
- Imported
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Config files now clean,
Zod validates the env,
Swap code is streamlined.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the mcp-atp package by centralizing configuration, simplifying contract ABIs, and streamlining the swap service. These are excellent improvements for maintainability and clarity.
The introduction of src/config.ts with Zod validation for environment variables is a standout improvement, making configuration robust. The move to a minimal ABI for router interactions is also a good step.
The primary concern identified is related to slippage protection in the SwapService, which should be addressed before merging. Other minor points are noted in the summary.
Summary of Findings
- Changeset Clarity on
amountUnit: In.changeset/wise-trains-fix.md, while ABI and env var changes are noted, it might be beneficial to explicitly mention that theamountparameter for buy/sell tools now expects units in wei, not ether. This is a significant change for users of the package. (Severity: Low, not commented due to settings) - Redundant Wallet Client Check: In
src/services/swap.ts, thebuy(lines 49-53) andsell(lines 79-83) methods include checks forwalletClientandwalletClient.account. SinceapproveTokenAllowance(which is called first) already performs this check, these subsequent checks might be considered redundant. However, they provide defense-in-depth. (Severity: Low, not commented due to settings) - Formatting of Return Strings in Tools: In
src/tools/add-agent-log.ts(lines 52-55),src/tools/buy.ts(lines 51-54), andsrc/tools/sell.ts(lines 51-54), the success message strings are now multi-line template literals with leading/trailing newlines and indentation. This might affect the display if printed directly to a console or log. Consider using.trim()or adjusting the template literal for cleaner output if necessary. (Severity: Low, not commented due to settings)
Merge Readiness
The pull request introduces valuable refactorings and improvements. However, due to the high-severity concern regarding the lack of slippage protection in the SwapService (buy/sell operations), I recommend that these changes be addressed before merging. Addressing this will enhance the safety and reliability of token trading functionalities. I am unable to approve the pull request myself; please ensure further review and approval after addressing the feedback. Other minor points are listed in the findings summary for consideration.
Closes https://github.com/IQAIcom/issues/issues/300 when this PR merged
Changes