-
Notifications
You must be signed in to change notification settings - Fork 210
Feat/notification service #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
- 新增英文版插件化通知系统架构设计文档,详细说明系统架构和实现方案 - 包含核心组件说明:通知提供者接口、插件加载器、通知管理器、消息格式化器 - 提供完整的代码示例,包括配置管理扩展、交易执行器重构和Telegram服务适配 - 添加Slack插件包设计方案,包含包结构、配置选项和使用指南 - 详细说明环境变量配置和代码使用方法,便于开发者
|
Hello, I have integrated it into the application. If you encounter any unexpected errors, please contact me with the error details from wherever you are. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the notification system from a hardcoded Telegram-only implementation to a plugin-based architecture using the tracker-notification package. The changes remove direct Telegram dependencies and related CLI commands, replacing them with a unified notification service that supports multiple providers (Telegram and Slack).
Key Changes:
- Introduced
tracker-notificationpackage dependency for managing multiple notification providers - Removed
TelegramService,handleTelegramCommand, and related test files - Updated
TradingExecutorto use the newSendNotificationutility instead ofTelegramService - Updated environment variable configuration to use
NOTIFICATION_TELEGRAM_ENABLEDandNOTIFICATION_SLACK_ENABLED
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/notification-service.ts | New unified notification service that manages Telegram and Slack providers |
| src/services/trading-executor.ts | Refactored to use new notification service, removed ConfigManager and TelegramService dependencies |
| src/services/telegram-service.ts | Deleted - functionality moved to tracker-notification package |
| src/commands/telegram.ts | Deleted - telegram-test command removed |
| src/commands/index.ts | Removed handleTelegramCommand export |
| src/index.ts | Removed telegram-test CLI command |
| package.json | Added tracker-notification@^1.0.0 dependency |
| .env.example | Updated with new notification configuration variables |
| README_EN.md | Updated documentation to reflect notification service changes |
| docs/plugin-notification-system-*.md | New documentation files for plugin architecture |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const telegramProvider = new TelegramNotificationProvider({ | ||
| botToken: process.env.TELEGRAM_BOT_TOKEN || '123456:ABC-DEF', | ||
| chatId: process.env.TELEGRAM_CHAT_ID || '123456789', | ||
| parseMode: 'Markdown' | ||
| }); | ||
| notifications.addProvider(telegramProvider); | ||
| } | ||
|
|
||
| if (process.env.NOTIFICATION_SLACK_ENABLED === 'true') { | ||
| const slackProvider = new SlackNotificationProvider({ | ||
| botToken: process.env.SLACK_BOT_TOKEN || 'xoxb-test-token', | ||
| channelId: process.env.SLACK_CHANNEL_ID || 'C123456789', |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded fallback values for sensitive credentials (botToken and chatId) should not be used. If these environment variables are not set, the code should either throw an error or skip provider initialization rather than using placeholder values that could lead to unexpected behavior or security issues.
| const telegramProvider = new TelegramNotificationProvider({ | |
| botToken: process.env.TELEGRAM_BOT_TOKEN || '123456:ABC-DEF', | |
| chatId: process.env.TELEGRAM_CHAT_ID || '123456789', | |
| parseMode: 'Markdown' | |
| }); | |
| notifications.addProvider(telegramProvider); | |
| } | |
| if (process.env.NOTIFICATION_SLACK_ENABLED === 'true') { | |
| const slackProvider = new SlackNotificationProvider({ | |
| botToken: process.env.SLACK_BOT_TOKEN || 'xoxb-test-token', | |
| channelId: process.env.SLACK_CHANNEL_ID || 'C123456789', | |
| if (!process.env.TELEGRAM_BOT_TOKEN || !process.env.TELEGRAM_CHAT_ID) { | |
| throw new Error('TELEGRAM_BOT_TOKEN and TELEGRAM_CHAT_ID must be set in environment variables to enable Telegram notifications.'); | |
| } | |
| const telegramProvider = new TelegramNotificationProvider({ | |
| botToken: process.env.TELEGRAM_BOT_TOKEN, | |
| chatId: process.env.TELEGRAM_CHAT_ID, | |
| parseMode: 'Markdown' | |
| }); | |
| notifications.addProvider(telegramProvider); | |
| } | |
| if (process.env.NOTIFICATION_SLACK_ENABLED === 'true') { | |
| if (!process.env.SLACK_BOT_TOKEN || !process.env.SLACK_CHANNEL_ID) { | |
| throw new Error('SLACK_BOT_TOKEN and SLACK_CHANNEL_ID must be set in environment variables to enable Slack notifications.'); | |
| } | |
| const slackProvider = new SlackNotificationProvider({ | |
| botToken: process.env.SLACK_BOT_TOKEN, | |
| channelId: process.env.SLACK_CHANNEL_ID, |
| const slackProvider = new SlackNotificationProvider({ | ||
| botToken: process.env.SLACK_BOT_TOKEN || 'xoxb-test-token', | ||
| channelId: process.env.SLACK_CHANNEL_ID || 'C123456789', |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to Telegram, hardcoded fallback values for Slack credentials should not be used. These placeholder tokens could cause silent failures or unintended message delivery.
| const slackProvider = new SlackNotificationProvider({ | |
| botToken: process.env.SLACK_BOT_TOKEN || 'xoxb-test-token', | |
| channelId: process.env.SLACK_CHANNEL_ID || 'C123456789', | |
| if (!process.env.SLACK_CHANNEL_ID) { | |
| throw new Error('SLACK_CHANNEL_ID environment variable must be set when Slack notifications are enabled.'); | |
| } | |
| const slackProvider = new SlackNotificationProvider({ | |
| botToken: process.env.SLACK_BOT_TOKEN || 'xoxb-test-token', | |
| channelId: process.env.SLACK_CHANNEL_ID, |
| console.log('\n🎉 Stop order notification sent successfully!'); | ||
| } | ||
|
|
||
|
|
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Extra blank line before the catch block (line 36) should be removed for consistency with standard TypeScript formatting.
| private configManager: ConfigManager; | ||
|
|
||
|
|
||
| constructor(apiKey?: string, apiSecret?: string, testnet?: boolean, configManager?: ConfigManager) { |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor still accepts a 'configManager' parameter but it's no longer used or stored. This unused parameter should be removed from the constructor signature to avoid confusion.
| constructor(apiKey?: string, apiSecret?: string, testnet?: boolean, configManager?: ConfigManager) { | |
| constructor(apiKey?: string, apiSecret?: string, testnet?: boolean) { |
| @@ -36,6 +36,7 @@ | |||
| "fs-extra": "^11.3.2", | |||
| "node-telegram-bot-api": "^0.66.0", | |||
| "querystring": "^0.2.1", | |||
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The new 'tracker-notification' dependency should be documented. Consider adding a comment in package.json or ensuring the README includes information about what this package provides and where to find its source/documentation.
| "querystring": "^0.2.1", | |
| "querystring": "^0.2.1", | |
| "_comment-tracker-notification": "Provides notification utilities for tracker events. See https://github.com/your-org/tracker-notification for source and documentation.", |
| TELEGRAM_BOT_TOKEN=telegram_token | ||
| TELEGRAM_CHAT_ID=telegram_chat_id |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The example values 'telegram_token' and 'telegram_chat_id' are not realistic placeholders. Consider using more descriptive placeholder formats like '<your_telegram_bot_token>' or 'YOUR_BOT_TOKEN_HERE' to make it clearer that these need to be replaced with actual values.
| TELEGRAM_BOT_TOKEN=telegram_token | |
| TELEGRAM_CHAT_ID=telegram_chat_id | |
| TELEGRAM_BOT_TOKEN=<your_telegram_bot_token> | |
| TELEGRAM_CHAT_ID=<your_telegram_chat_id> |
| # Bot Token: Create Slack App at https://api.slack.com/apps | ||
| # Channel ID: Right-click channel → "Copy link" → extract ID from URL | ||
| SLACK_BOT_TOKEN=xoxb-1234 | ||
| SLACK_CHANNEL_ID="#new-channel" |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example shows a channel name format ('#new-channel') but the code expects a channel ID (like 'C123456789'). This mismatch could confuse users. The example should use a proper channel ID format or the documentation should clarify the difference.
| SLACK_CHANNEL_ID="#new-channel" | |
| SLACK_CHANNEL_ID="C1234567890" |
danimirpetrakov16-byte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BotНub
No description provided.