Skip to content

Conversation

@RedBoardDev
Copy link
Contributor

No description provided.

@CryptoGnome
Copy link
Owner

Security & Quality Review - Changes Requested

🚨 Critical Issues

1. Wrong Target Branch

This PR is targeting main but should target dev according to our Git Flow Lite strategy. Feature branches must merge to dev first.

Action Required:

gh pr edit 33 --base dev

2. Logic Inversion in Threshold Selection

The threshold selection logic appears inverted for SAME mode (lines 498-508 in hunter.ts):

} else {
    // Momentum: Use thresholds based on opposite of liquidation side
    thresholdToCheck = liquidation.side === 'BUY'
        ? (symbolConfig.longVolumeThresholdUSDT ?? symbolConfig.volumeThresholdUSDT ?? 0)
        : (symbolConfig.shortVolumeThresholdUSDT ?? symbolConfig.volumeThresholdUSDT ?? 0);
}

When trade_side is SAME (momentum strategy):

  • BUY liquidation → should check shortVolumeThresholdUSDT (we'll BUY)
  • SELL liquidation → should check longVolumeThresholdUSDT (we'll SELL)

But the code does the opposite. This could cause incorrect threshold applications.

3. Inconsistent Implementation

The threshold monitor (thresholdMonitor.ts line 159) still uses hardcoded contrarian logic:

const isLongOpportunity = liquidation.side === 'SELL'; // Always contrarian!

This doesn't respect the new trade_side parameter, creating inconsistency between instant and threshold trigger systems.

4. No Test Coverage

Zero tests exist for the new functionality. Required test scenarios:

  • OPPOSITE mode trade direction validation
  • SAME mode trade direction validation
  • Threshold selection logic for each mode
  • Integration with threshold monitor

✅ Security Review: PASSED

  • No credential leaks detected
  • No security vulnerabilities
  • Configuration changes are safe
  • No breaking changes to existing functionality

📝 Recommendations

Before merge:

  1. Change PR target to dev branch
  2. Fix threshold selection logic inversion
  3. Update threshold monitor to respect trade_side
  4. Add comprehensive test coverage

Code quality improvements:

  • Consider extracting trade direction logic into separate helper methods
  • Add JSDoc documentation for the new parameter
  • Add validation/warnings for high-risk configurations (SAME mode + high leverage)

🚦 Verdict

❌ CHANGES REQUESTED - DO NOT MERGE

The feature concept is valuable for supporting different trading strategies, but the implementation needs corrections to ensure it works as intended and doesn't cause unexpected trading behavior.

Please address these issues and request re-review when ready.


🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants