fix: make /send endpoint minimal
#36
Conversation
WalkthroughThis change refactors the send and pending transaction APIs to use Telegram numeric user IDs instead of usernames for sender and recipient identification. It removes sender whitelisting, enforces a fixed sender ID from configuration, and updates related data structures, approval logic, and test scripts to align with these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant API Client
participant API Service
participant Config
participant UserStore
participant Admin
participant TelegramBot
API Client->>API Service: POST /api/send {to: userId, amount, memo}
API Service->>Config: GetAPIFromUserId()
API Service->>UserStore: Load fromUser by fixed ID
API Service->>UserStore: Load toUser by provided userId
API Service->>API Service: Create PendingTransaction (with user IDs)
API Service->>TelegramBot: CreateAPIApprovalRequest(fromUser, toUserId, ...)
TelegramBot->>Admin: Notify approval request
Admin->>TelegramBot: Approve/Reject via Telegram
TelegramBot->>API Service: approveAPITransactionHandler
API Service->>UserStore: Load users by IDs (if needed)
API Service->>API Service: Execute transaction
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
internal/api/send.go (1)
91-98: Update isTelegramID to properly validate the full Telegram ID rangeThe current regex (
^[0-9]{5,15}$) is too restrictive—Telegram IDs start at 1 and can be as large as 2⁵²−1 (≈4.5×10¹⁵), which is up to 16 digits. I recommend refactoring to:
- Strip the leading “@”
- Ensure the remaining string is all digits
- Parse it into a uint64
- Check it’s between 1 and (1<<52)-1
Example replacement in
internal/api/send.go(lines 91–98):func isTelegramID(identifier string) bool { - // Remove @ prefix if present - identifier = strings.TrimPrefix(identifier, "@") - // Check if it's all digits and has reasonable length for Telegram ID - match, _ := regexp.MatchString(`^[0-9]{5,15}$`, identifier) - return match + identifier = strings.TrimPrefix(identifier, "@") + // must be digits only + if !regexp.MustCompile(`^\d+$`).MatchString(identifier) { + return false + } + id, err := strconv.ParseUint(identifier, 10, 64) + if err != nil { + return false + } + const maxTelegramID = (1<<52) - 1 + return id >= 1 && id <= maxTelegramID }
- internal/api/send.go: adjust import to include
strconvand use a compiled digit-only regex.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/api/admin/pending_transactions.go(2 hunks)internal/api/pending_transaction.go(3 hunks)internal/api/send.go(6 hunks)internal/api/send_config.go(1 hunks)internal/config.go(3 hunks)internal/telegram/api_approval.go(6 hunks)test_pay_api.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
internal/api/send_config.go (1)
internal/config.go (1)
Configuration(13-21)
internal/telegram/api_approval.go (5)
internal/lnbits/types.go (1)
User(24-39)internal/telegram/database.go (1)
GetUserByTelegramID(164-175)internal/utils/number_format.go (1)
FormatSats(37-64)internal/telegram/bot.go (1)
TipBot(25-34)internal/telegram/users.go (1)
GetUserStr(29-38)
internal/api/pending_transaction.go (3)
internal/api/send.go (1)
SendRequest(19-23)internal/lnbits/types.go (1)
User(24-39)internal/telegram/database.go (1)
GetUserByTelegramID(164-175)
🔇 Additional comments (21)
internal/api/send_config.go (1)
37-40: LGTM! Clean implementation of the new fixed sender ID approach.The function correctly retrieves the configured sender Telegram ID, replacing the previous whitelist-based approach. This aligns perfectly with the PR objective of simplifying the
/sendendpoint.test_pay_api.sh (1)
8-13: LGTM! Test script correctly reflects the new API contract.The updated comments and payload format properly align with the refactored API that now uses numeric Telegram user IDs and a fixed sender ID from configuration.
internal/api/admin/pending_transactions.go (1)
130-152: LGTM! Improved user loading logic with proper fallback.The optimization to use cached user objects when available is excellent for performance, and the fallback to loading by Telegram ID correctly aligns with the new ID-based architecture. Error handling and messaging are appropriately updated.
internal/config.go (3)
57-57: Minor formatting improvement.Good addition of blank line for better code organization and readability.
82-92: LGTM! Configuration struct properly refactored for fixed sender approach.The removal of
WhitelistedSendersand addition ofFromUserIdcorrectly implements the new architecture that enforces a single, fixed sender ID instead of a whitelist-based approach.
190-199: Excellent enforcement of mandatory configuration.The validation logic correctly ensures that both
HMACSecretandFromUserIdare configured when the API Send module is enabled. Using panic for configuration validation at startup is appropriate and prevents runtime issues.internal/api/pending_transaction.go (4)
19-20: LGTM! Proper migration to ID-based user identification.The change from username strings to Telegram user ID integers is a significant improvement that aligns with Telegram's primary identifier system and provides more reliable user identification.
42-43: LGTM! Constructor properly updated for new architecture.The function signature correctly accepts the
fromUserIdparameter, and the transaction ID generation using numeric user IDs is more reliable than the previous username-based approach.
50-51: LGTM! Struct initialization correctly uses user IDs.The field assignments properly store the numeric Telegram user IDs, maintaining consistency with the new ID-based architecture.
125-140: LGTM! User loading logic properly migrated to ID-based approach.The function correctly loads users by their Telegram IDs instead of usernames, with appropriate error logging that reflects the new ID-based system. The graceful error handling allows the system to continue functioning even if user loading fails.
internal/telegram/api_approval.go (5)
29-31: LGTM! Struct fields properly support the transition to user IDs.The addition of
FromUserId,ToUser, andToUserIdfields aligns well with the migration from username-based to ID-based user identification.
66-79: LGTM! Efficient user retrieval with proper fallback.Good implementation of using cached user data when available and falling back to database lookup only when necessary. The error handling appropriately notifies the user of failures.
132-132: LGTM! Consistent with the new user object structure.The success message correctly uses the username from the user object.
177-192: LGTM! Robust user display name handling.Excellent implementation with multiple fallback options for displaying the recipient. The code gracefully handles cases where user lookup fails or users don't have usernames.
212-214: LGTM! Proper data storage for approval tracking.The approval data correctly stores both the user ID and the user object (when available) for efficient retrieval later.
internal/api/send.go (6)
20-20: LGTM! Simplified request structure aligns with PR objectives.The removal of the "from" field and restriction to numeric Telegram IDs simplifies the API and improves security by enforcing a fixed sender.
141-144: LGTM! Clear validation for recipient field.The validation correctly enforces that only numeric Telegram IDs are accepted as recipients.
169-183: LGTM! Robust recipient lookup with proper error handling.The code correctly parses the numeric ID and performs user lookup with appropriate error messages for both parsing and lookup failures.
197-197: LGTM! Correct parameter passing for pending transaction.The pending transaction creation correctly includes the sender's Telegram ID.
208-220: LGTM! Proper approval flow with ID-based responses.The approval request correctly uses the recipient's user ID, and the response fields appropriately use the string representations of the IDs.
280-281: LGTM! Consistent response format for successful transactions.The response correctly uses string representations of the user IDs, maintaining consistency with the approval flow.
| // Parse sender Telegram ID | ||
| fromUserIdStr := GetAPIFromUserId() | ||
| fromUserId, err := strconv.ParseInt(fromUserIdStr, 10, 64) | ||
| if err != nil { | ||
| log.Errorf("[api/send] Invalid sender Telegram ID %d: %v", fromUserId, err) | ||
| RespondError(w, "Invalid sender configuration") | ||
| return | ||
| } |
There was a problem hiding this comment.
Fix variable usage in error logging.
There's a bug in the error logging - fromUserId is used before it's assigned from the parsing result.
- log.Errorf("[api/send] Invalid sender Telegram ID %d: %v", fromUserId, err)
+ log.Errorf("[api/send] Invalid sender Telegram ID %s: %v", fromUserIdStr, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Parse sender Telegram ID | |
| fromUserIdStr := GetAPIFromUserId() | |
| fromUserId, err := strconv.ParseInt(fromUserIdStr, 10, 64) | |
| if err != nil { | |
| log.Errorf("[api/send] Invalid sender Telegram ID %d: %v", fromUserId, err) | |
| RespondError(w, "Invalid sender configuration") | |
| return | |
| } | |
| // Parse sender Telegram ID | |
| fromUserIdStr := GetAPIFromUserId() | |
| fromUserId, err := strconv.ParseInt(fromUserIdStr, 10, 64) | |
| if err != nil { | |
| log.Errorf("[api/send] Invalid sender Telegram ID %s: %v", fromUserIdStr, err) | |
| RespondError(w, "Invalid sender configuration") | |
| return | |
| } |
🤖 Prompt for AI Agents
In internal/api/send.go around lines 131 to 138, the error log uses fromUserId
before it is assigned, causing incorrect logging. Replace fromUserId in the log
message with fromUserIdStr, which holds the original string value that failed to
parse, to correctly reflect the invalid input in the error log.
User description
API_SEND_HMAC_SECRETandAPI_SEND_FROM_USER_IDmandatory configurations whenCONFIGOR_API_SEND_ENABLEDistruePR Type
Enhancement
Description
Remove whitelist configurations for sender accounts
Make sender ID configurable via environment variable
Switch from usernames to Telegram IDs for identification
Require mandatory HMAC secret and sender ID configuration
Changes diagram
Changes walkthrough 📝
4 files
Update pending transaction execution to use Telegram IDsReplace username fields with Telegram ID fieldsRemove whitelist and Lightning address support, use Telegram IDsUpdate approval system to use Telegram IDs2 files
Remove whitelist config, add sender ID configurationUpdate API configuration structure and validation1 files
Update test script to use Telegram ID formatSummary by CodeRabbit
New Features
Refactor
Chores