-
Notifications
You must be signed in to change notification settings - Fork 1
Better logging #43
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: main
Are you sure you want to change the base?
Better logging #43
Conversation
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 introduces a notification level filtering system to control which notifications are sent to external services (Slack/Discord) or logged to the console. The filtering system has three levels: HIGH (critical alerts), MED (successful operations and moderate warnings), and LOW (informational messages).
Key changes include:
- Added NotificationLevel enum (HIGH, MED, LOW) with filtering logic in the sendNotification function
- Updated all sendNotification calls throughout the codebase to specify appropriate notification levels
- Added getNotificationLevelForAuction helper function to determine notification levels based on auction type and fill status
- Added configuration validation and documentation for the new notificationLevel config option
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/notifier.ts | Added NotificationLevel enum, implemented level-based filtering in sendNotification, and added getNotificationLevelForAuction helper function |
| src/utils/config.ts | Added notificationLevel field to AppConfig interface and validation logic |
| src/work_submitter.ts | Updated sendNotification calls to use appropriate notification levels; fixed spelling error "transfering" → "transferring" |
| src/work_handler.ts | Updated sendNotification call to use NotificationLevel.MED |
| src/pool_event_handler.ts | Updated multiple sendNotification calls to use getNotificationLevelForAuction for appropriate level assignment |
| src/liquidations.ts | Updated sendNotification call to use NotificationLevel.MED |
| src/bidder_submitter.ts | Updated sendNotification calls throughout to use getNotificationLevelForAuction and appropriate levels |
| src/bidder_handler.ts | Updated sendNotification call to use getNotificationLevelForAuction |
| src/utils/soroban_helper.ts | Adjusted transaction timeout from 6 seconds to 12 seconds; moved submitStartTime initialization |
| test/utils/notifier.test.ts | Added comprehensive test suite for notification filtering, webhook integration, and getNotificationLevelForAuction function |
| test/utils/config.test.ts | Added test for notification level validation |
| test/bidder_submitter.test.ts | Updated test expectations to include notification level parameter |
| example.config.json | Added example notificationLevel configuration |
| README.md | Added documentation for the new notificationLevel configuration option |
| start.sh | Removed extra blank lines |
| example.env | Removed example environment file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe('notification level filtering', () => { | ||
| beforeEach(() => { | ||
| (APP_CONFIG as any).slackWebhook = 'https://hooks.slack.com/test'; | ||
| mockFetch.mockResolvedValue({ | ||
| ok: true, | ||
| status: 200, | ||
| } as Response); | ||
| }); | ||
|
|
||
| it('should send HIGH level notifications when config is set to HIGH', async () => { | ||
| (APP_CONFIG as any).notificationLevel = NotificationLevel.HIGH; | ||
|
|
||
| await sendNotification('High priority message', NotificationLevel.HIGH); | ||
|
|
||
| expect(mockFetch).toHaveBeenCalledTimes(1); | ||
| expect(mockFetch).toHaveBeenCalledWith( | ||
| 'https://hooks.slack.com/test', | ||
| expect.objectContaining({ | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it('should NOT send MED level notifications when config is set to HIGH', async () => { | ||
| (APP_CONFIG as any).notificationLevel = NotificationLevel.HIGH; | ||
|
|
||
| await sendNotification('Medium priority message', NotificationLevel.MED); | ||
|
|
||
| expect(mockFetch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should NOT send LOW level notifications when config is set to HIGH', async () => { | ||
| (APP_CONFIG as any).notificationLevel = NotificationLevel.HIGH; | ||
|
|
||
| await sendNotification('Low priority message', NotificationLevel.LOW); | ||
|
|
||
| expect(mockFetch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should send HIGH and MED notifications when config is set to MED', async () => { | ||
| (APP_CONFIG as any).notificationLevel = NotificationLevel.MED; | ||
|
|
||
| await sendNotification('High priority message', NotificationLevel.HIGH); | ||
| expect(mockFetch).toHaveBeenCalledTimes(1); | ||
|
|
||
| mockFetch.mockClear(); | ||
|
|
||
| await sendNotification('Medium priority message', NotificationLevel.MED); | ||
| expect(mockFetch).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('should NOT send LOW notifications when config is set to MED', async () => { | ||
| (APP_CONFIG as any).notificationLevel = NotificationLevel.MED; | ||
|
|
||
| await sendNotification('Low priority message', NotificationLevel.LOW); | ||
|
|
||
| expect(mockFetch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should send all notifications when config level is undefined (defaults to MED)', async () => { | ||
| (APP_CONFIG as any).notificationLevel = undefined; | ||
|
|
||
| await sendNotification('High priority', NotificationLevel.HIGH); | ||
| expect(mockFetch).toHaveBeenCalledTimes(1); | ||
|
|
||
| mockFetch.mockClear(); | ||
| await sendNotification('Medium priority', NotificationLevel.MED); | ||
| expect(mockFetch).toHaveBeenCalledTimes(1); | ||
|
|
||
| mockFetch.mockClear(); | ||
| await sendNotification('Low priority', NotificationLevel.LOW); | ||
| expect(mockFetch).not.toHaveBeenCalled(); // MED level filters out LOW | ||
| }); | ||
| }); |
Copilot
AI
Dec 31, 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.
Missing test coverage for the notification level filtering behavior when the config is set to NotificationLevel.LOW. Tests should verify that when config is set to LOW, all notification levels (HIGH, MED, and LOW) are sent.
| if (isBotFill) { | ||
| switch (auctionType) { | ||
| case 0: // Liquidation | ||
| return NotificationLevel.MED; | ||
| case 1: // Bad Debt | ||
| return NotificationLevel.HIGH; | ||
| case 2: // Interest | ||
| return NotificationLevel.MED; | ||
| default: | ||
| return NotificationLevel.MED; | ||
| } | ||
| } else { | ||
| switch (auctionType) { | ||
| case 0: // Liquidation | ||
| return NotificationLevel.LOW; | ||
| case 1: // Bad Debt | ||
| return NotificationLevel.HIGH; | ||
| case 2: // Interest | ||
| return NotificationLevel.LOW; | ||
| default: | ||
| return NotificationLevel.LOW; | ||
| } |
Copilot
AI
Dec 31, 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.
Using hardcoded numeric values (0, 1, 2) instead of the AuctionType enum constants makes the code less maintainable and harder to understand. Consider using AuctionType.Liquidation, AuctionType.BadDebt, and AuctionType.Interest instead of the numeric values.
| @@ -1,7 +1,25 @@ | |||
| import { AuctionType } from '@blend-capital/blend-sdk'; | |||
Copilot
AI
Dec 31, 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.
There's an inconsistency in AuctionType imports. The getNotificationLevelForAuction function imports AuctionType from '@blend-capital/blend-sdk', but callers like work_submitter.ts import it from './utils/db.js'. While they may have compatible numeric values, this creates a type mismatch and potential maintenance issues if either definition changes. Consider using a single source of truth for AuctionType throughout the codebase.
| `Error transfering bad debt\n` + | ||
| `Error transferring bad debt\n` + | ||
| `Pool: ${badDebtTransfer.poolId}\n` + | ||
| `User: ${badDebtTransfer.user}` + |
Copilot
AI
Dec 31, 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.
Missing newline character in the error message. The string concatenation on line 152 should end with \n to properly format the error message, otherwise "User: " and "Error: " will appear on the same line.
| `User: ${badDebtTransfer.user}` + | |
| `User: ${badDebtTransfer.user}\n` + |
| let txResponse = await stellarRpc.sendTransaction(transaction); | ||
| if (txResponse.status === 'TRY_AGAIN_LATER') { | ||
| await new Promise((resolve) => setTimeout(resolve, 4000)); | ||
| txResponse = await stellarRpc.sendTransaction(transaction); | ||
| } | ||
| let submitStartTime = Date.now(); |
Copilot
AI
Dec 31, 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 submitStartTime is now initialized after the retry logic (lines 488-492), which means if a transaction retry occurs, the wait time won't include the 4-second delay. This could cause the timeout check on line 503 to wait longer than intended. The timer should be initialized before attempting to send the transaction to accurately measure the total wait time.
| let txResponse = await stellarRpc.sendTransaction(transaction); | |
| if (txResponse.status === 'TRY_AGAIN_LATER') { | |
| await new Promise((resolve) => setTimeout(resolve, 4000)); | |
| txResponse = await stellarRpc.sendTransaction(transaction); | |
| } | |
| let submitStartTime = Date.now(); | |
| let submitStartTime = Date.now(); | |
| let txResponse = await stellarRpc.sendTransaction(transaction); | |
| if (txResponse.status === 'TRY_AGAIN_LATER') { | |
| await new Promise((resolve) => setTimeout(resolve, 4000)); | |
| txResponse = await stellarRpc.sendTransaction(transaction); | |
| } |
| * filled it. | ||
| * @param auctionType - The auction type to determine the notification level for | ||
| * @param isBotFill - Whether the bot successfully filled the auction | ||
| * @returns |
Copilot
AI
Dec 31, 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.
Missing return type documentation in the JSDoc comment. The @returns tag is present but has no description of what the function returns.
| * @returns | |
| * @returns The notification level to use for the given auction type and fill status. |
| | `blndAddress` | The address of the BLND token contract. | | ||
| | `interestFillerAddress` | A contract address used to help fill interest auctions. Must implement the same interface as [this example](https://github.com/script3/interest-auction-filler). | | ||
| | `workerKeypair` | The secret key for the bot's auction creating account. This should be different from the filler as auction creation and auction bidding can happen simultaneously. **Keep this secret and secure!** | | ||
| | `fillerKeypair` | The securet key for the bot's auction filler account. This should be different from the worker as auction creation and auction bidding can happen simultaneously. **Keep this secret and secure!** | |
Copilot
AI
Dec 31, 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.
There's a spelling error in "securet" which should be "secret".
| | `fillerKeypair` | The securet key for the bot's auction filler account. This should be different from the worker as auction creation and auction bidding can happen simultaneously. **Keep this secret and secure!** | | |
| | `fillerKeypair` | The secret key for the bot's auction filler account. This should be different from the worker as auction creation and auction bidding can happen simultaneously. **Keep this secret and secure!** | |
| if (level === NotificationLevel.LOW) { | ||
| return; | ||
| } | ||
| break; |
Copilot
AI
Dec 31, 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 notification level filtering logic is missing a case for when the config is set to NotificationLevel.LOW. Currently, if the config is set to LOW, it falls through to the default case which treats it as MED, filtering out LOW notifications. There should be a case for NotificationLevel.LOW that allows all notifications to be sent.
| break; | |
| break; | |
| case NotificationLevel.LOW: | |
| // LOW threshold: allow all notifications (HIGH, MED, and LOW) | |
| break; |
No description provided.