-
Notifications
You must be signed in to change notification settings - Fork 5
fix: send tx to chain stg #445
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
Conversation
(cherry picked from commit e6ae1f4)
Lb/fix/max fee
…s-in-prod FIX: adds error handler and retry to connection state request
feat: adds logs to get info about send tx error
…ion does not interfere with the sending of another transaction
| ); | ||
|
|
||
| router.get('/:id/advanced-details', handleResponse(findAdvancedDetails)); | ||
| router.post('/send/:hash', handleResponse(send)); |
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.
🔴 CRITICAL: Security vulnerability - unauthenticated endpoint
Problem:
The /send/:hash route was moved outside the authMiddleware, making it accessible without authentication. This allows anyone to trigger transaction sending to the blockchain.
Suggestion:
Move the route back inside the auth middleware:
router.use(authMiddleware);
// ... other authenticated routes
router.post('/send/:hash', handleResponse(send));| //instance tx | ||
| //add witnesses | ||
| async sendToChain(hash: string, network: Network) { | ||
| logger.info({ hash, network }, '[SEND_TO_CHAIN] Sending transaction to chain'); |
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.
🟡 IMPORTANT: Missing input validation
Problem:
The hash parameter is used directly in database queries without validation. This could lead to injection attacks or unexpected behavior.
Suggestion:
Add hash validation at the beginning of the method:
if (!hash || typeof hash !== 'string' || hash.length < 10) {
throw new BadRequest({ type: ErrorTypes.InvalidInput, message: 'Invalid hash format' });
}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.
Great point about validation, but in this case:
- TypeORM uses prepared statements, so SQL injection is not a risk
- If it were invalid, the database would return null, and a NotFound error would be sent in the response
| ]), | ||
| ), | ||
| }, | ||
| order: { createdAt: 'DESC' }, |
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.
🔵 SUGGESTION: Consider database performance
Problem:
The query uses ORDER BY createdAt DESC which might impact performance if there's no index on createdAt column, especially with multiple transactions having the same hash.
Suggestion:
Ensure there's a database index on (hash, createdAt) or consider limiting the query:
.orderBy('createdAt', 'DESC')
.limit(1)
guimroque
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.
Code Review - Summary
What was done
- Fixed bug where canceled transactions with same hash interfered with new transactions
- Added CANCELED status to ignored statuses in transaction query
- Added order by createdAt DESC to get newest transaction with same hash
- Replaced console.* with logger.* for proper logging
- Added comprehensive test case for the bug scenario
- Updated fuels version from 0.99.0/0.101.1 to 0.101.3
- Moved /send/:hash route outside auth middleware
Positive Points
- Good bug fix addressing a real edge case scenario
- Comprehensive test coverage for the specific bug
- Proper logging implementation replacing console statements
- Clear commit message and PR description
- Follows conventional commit format
- Appropriate use of logger with structured data
Issues Found
- Route security issue: /send/:hash endpoint moved outside authentication
- Missing input validation on hash parameter
- Potential performance impact from ordering without index consideration
Total comments: 3 (1 critical, 1 important, 1 suggestion)
guimroque
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.
LGTM! ✅
Previous issues have been fixed. Code approved.
Description
Summary
console.*withlogger.*fuelsversionChecklist