Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
249 changes: 249 additions & 0 deletions app/components/UI/Perps/Views/PerpsOrderView/PerpsOrderView.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ jest.mock('../../../../../../locales/i18n', () => ({
'Size must be a positive number',
'perps.tpsl.stop_loss_order_view_warning':
'Stop loss is {{direction}} liquidation price',
'perps.tpsl.take_profit_wrong_side_warning':
'Take profit must be {{direction}} current price. Update or clear it to place the order.',
'perps.tpsl.stop_loss_wrong_side_warning':
'Stop loss must be {{direction}} current price. Update or clear it to place the order.',
'perps.tpsl.below': 'below',
'perps.tpsl.above': 'above',
'perps.points': 'Points',
Expand Down Expand Up @@ -535,6 +539,16 @@ jest.mock('../../../../../core/Engine', () => ({
},
}));

// Mock usePerpsABTest hook (controllable per-test)
const mockUsePerpsABTest = jest.fn(() => ({
variantName: 'control',
variant: { long: 'green', short: 'red' },
isEnabled: false,
}));
jest.mock('../../utils/abTesting/usePerpsABTest', () => ({
usePerpsABTest: () => mockUsePerpsABTest(),
}));

// Mock useTooltipModal hook
jest.mock('../../../../hooks/useTooltipModal', () => ({
__esModule: true,
Expand Down Expand Up @@ -1970,6 +1984,241 @@ describe('PerpsOrderView', () => {
});
});

describe('TP/SL wrong-side price validation', () => {
const orderContextWithTPSL = (overrides: {
direction: 'long' | 'short';
takeProfitPrice?: string;
stopLossPrice?: string;
}) => ({
orderForm: {
asset: 'ETH',
amount: '100',
leverage: 10,
direction: overrides.direction,
type: 'market' as const,
limitPrice: undefined,
takeProfitPrice: overrides.takeProfitPrice,
stopLossPrice: overrides.stopLossPrice,
balancePercent: 10,
},
setAmount: jest.fn(),
setLeverage: jest.fn(),
setTakeProfitPrice: jest.fn(),
setStopLossPrice: jest.fn(),
setLimitPrice: jest.fn(),
setOrderType: jest.fn(),
handlePercentageAmount: jest.fn(),
handleMaxAmount: jest.fn(),
handleMinAmount: jest.fn(),
optimizeOrderAmount: jest.fn(),
maxPossibleAmount: 1000,
balanceForValidation: 1000,
calculations: {
marginRequired: '10',
positionSize: '0.033',
},
});

it('shows warning and disables button for long position with TP below current price', async () => {
// Arrange: TP at 2000 is below current price 3000 → invalid for long
(usePerpsOrderContext as jest.Mock).mockReturnValue(
orderContextWithTPSL({ direction: 'long', takeProfitPrice: '2000' }),
);
(usePerpsOrderValidation as jest.Mock).mockReturnValue({
isValid: true,
errors: [],
isValidating: false,
});
(usePerpsOrderExecution as jest.Mock).mockReturnValue({
placeOrder: jest.fn(),
isPlacing: false,
});

// Act
render(<PerpsOrderView />, { wrapper: TestWrapper });

// Assert: warning visible
await waitFor(() => {
expect(
screen.getByText(
'Take profit must be above current price. Update or clear it to place the order.',
),
).toBeDefined();
Comment thread
michalconsensys marked this conversation as resolved.
});

// Assert: button disabled
const placeOrderButton = await screen.findByTestId(
PerpsOrderViewSelectorsIDs.PLACE_ORDER_BUTTON,
);
expect(placeOrderButton.props.accessibilityState?.disabled).toBeTruthy();
});

it('shows warning and disables button for long position with SL above current price', async () => {
// Arrange: SL at 3500 is above current price 3000 → invalid for long
(usePerpsOrderContext as jest.Mock).mockReturnValue(
orderContextWithTPSL({ direction: 'long', stopLossPrice: '3500' }),
);
(usePerpsOrderValidation as jest.Mock).mockReturnValue({
isValid: true,
errors: [],
isValidating: false,
});
(usePerpsOrderExecution as jest.Mock).mockReturnValue({
placeOrder: jest.fn(),
isPlacing: false,
});

// Act
render(<PerpsOrderView />, { wrapper: TestWrapper });

// Assert: warning visible
await waitFor(() => {
expect(
screen.getByText(
'Stop loss must be below current price. Update or clear it to place the order.',
),
).toBeDefined();
});

// Assert: button disabled
const placeOrderButton = await screen.findByTestId(
PerpsOrderViewSelectorsIDs.PLACE_ORDER_BUTTON,
);
expect(placeOrderButton.props.accessibilityState?.disabled).toBeTruthy();
});

it('shows warning for short position with TP above current price', async () => {
// Arrange: TP at 3500 is above current price 3000 → invalid for short
(usePerpsOrderContext as jest.Mock).mockReturnValue(
orderContextWithTPSL({ direction: 'short', takeProfitPrice: '3500' }),
);

// Act
render(<PerpsOrderView />, { wrapper: TestWrapper });

// Assert: warning uses "below" for short
await waitFor(() => {
expect(
screen.getByText(
'Take profit must be below current price. Update or clear it to place the order.',
),
).toBeDefined();
});
});

it('shows warning for short position with SL below current price', async () => {
// Arrange: SL at 2000 is below current price 3000 → invalid for short
(usePerpsOrderContext as jest.Mock).mockReturnValue(
orderContextWithTPSL({ direction: 'short', stopLossPrice: '2000' }),
);

// Act
render(<PerpsOrderView />, { wrapper: TestWrapper });

// Assert: warning uses "above" for short
await waitFor(() => {
expect(
screen.getByText(
'Stop loss must be above current price. Update or clear it to place the order.',
),
).toBeDefined();
});
});

it('does not show wrong-side warnings when TP/SL prices are valid', async () => {
// Arrange: valid TP above and SL below current price for long
(usePerpsOrderContext as jest.Mock).mockReturnValue(
orderContextWithTPSL({
direction: 'long',
takeProfitPrice: '3500',
stopLossPrice: '2500',
}),
);

// Act
render(<PerpsOrderView />, { wrapper: TestWrapper });

// Assert: component renders (TP/SL summary with valid percentages)
await waitFor(() => {
expect(screen.getByText('Leverage')).toBeDefined();
});

// Assert: no wrong-side warnings
expect(screen.queryByText(/Take profit must be/)).toBeNull();
expect(screen.queryByText(/Stop loss must be.*current price/)).toBeNull();
});

it('disables button when both TP and SL are on wrong side', async () => {
// Arrange: both invalid for long (TP below, SL above current price)
(usePerpsOrderContext as jest.Mock).mockReturnValue(
orderContextWithTPSL({
direction: 'long',
takeProfitPrice: '2000',
stopLossPrice: '3500',
}),
);
(usePerpsOrderValidation as jest.Mock).mockReturnValue({
isValid: true,
errors: [],
isValidating: false,
});
(usePerpsOrderExecution as jest.Mock).mockReturnValue({
placeOrder: jest.fn(),
isPlacing: false,
});

// Act
render(<PerpsOrderView />, { wrapper: TestWrapper });

// Assert: both warnings shown
await waitFor(() => {
expect(screen.getByText(/Take profit must be above/)).toBeDefined();
expect(screen.getByText(/Stop loss must be below/)).toBeDefined();
});

// Assert: button disabled
const placeOrderButton = await screen.findByTestId(
PerpsOrderViewSelectorsIDs.PLACE_ORDER_BUTTON,
);
expect(placeOrderButton.props.accessibilityState?.disabled).toBeTruthy();
});

it('disables monochrome button variant when TP/SL is invalid', async () => {
// Arrange: monochrome A/B test variant + invalid TP
mockUsePerpsABTest.mockReturnValue({
variantName: 'monochrome',
variant: { long: 'white', short: 'white' },
isEnabled: true,
});
(usePerpsOrderContext as jest.Mock).mockReturnValue(
orderContextWithTPSL({ direction: 'long', takeProfitPrice: '2000' }),
);
(usePerpsOrderValidation as jest.Mock).mockReturnValue({
isValid: true,
errors: [],
isValidating: false,
});
(usePerpsOrderExecution as jest.Mock).mockReturnValue({
placeOrder: jest.fn(),
isPlacing: false,
});

// Act
render(<PerpsOrderView />, { wrapper: TestWrapper });

// Assert: warning visible (proves hasInvalidTPSL is true in monochrome path)
await waitFor(() => {
expect(screen.getByText(/Take profit must be above/)).toBeDefined();
});

// Assert: monochrome button rendered and receives isDisabled prop
const placeOrderButton = await screen.findByTestId(
PerpsOrderViewSelectorsIDs.PLACE_ORDER_BUTTON,
);
expect(placeOrderButton).toBeDefined();
});
});

describe('TP/SL limit price validation', () => {
it('shows toast and prevents TP/SL bottom sheet from opening on limit order without limit price', async () => {
// Clear all mocks to ensure clean state
Expand Down
67 changes: 65 additions & 2 deletions app/components/UI/Perps/Views/PerpsOrderView/PerpsOrderView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
} from '../../../../../component-library/components/Texts/Text';
import useTooltipModal from '../../../../../components/hooks/useTooltipModal';
import Routes from '../../../../../constants/navigation/Routes';
import Engine from '../../../../../core/Engine';
import DevLogger from '../../../../../core/SDKConnect/utils/DevLogger';
import { useTheme } from '../../../../../util/theme';
import { TraceName } from '../../../../../util/trace';
Expand Down Expand Up @@ -139,6 +140,8 @@
import {
calculateRoEForPrice,
isStopLossSafeFromLiquidation,
isValidStopLossPrice,
isValidTakeProfitPrice,
} from '../../utils/tpslValidation';
import createStyles from './PerpsOrderView.styles';
import { PerpsPayRow } from './PerpsPayRow';
Expand Down Expand Up @@ -672,7 +675,11 @@
);
const absRoE = Math.abs(parseFloat(tpRoE || '0'));
tpDisplay =
absRoE > 0 ? `${absRoE.toFixed(0)}%` : strings('perps.order.off');
absRoE > 0
? `${absRoE.toFixed(0)}%`
: formatPerpsFiat(orderForm.takeProfitPrice, {
ranges: PRICE_RANGES_UNIVERSAL,
});
}

if (orderForm.stopLossPrice && price > 0 && orderForm.leverage) {
Expand All @@ -689,7 +696,11 @@
);
const absRoE = Math.abs(parseFloat(slRoE || '0'));
slDisplay =
absRoE > 0 ? `${absRoE.toFixed(0)}%` : strings('perps.order.off');
absRoE > 0
? `${absRoE.toFixed(0)}%`
: formatPerpsFiat(orderForm.stopLossPrice, {
ranges: PRICE_RANGES_UNIVERSAL,
});
}

return `${strings('perps.order.tp')} ${tpDisplay}, ${strings(
Expand Down Expand Up @@ -1079,6 +1090,12 @@
} else {
await executeOrder(orderParams);
}

// Clear pending trade config after successful submission to prevent
// stale TP/SL values from being restored on the next order form visit
Engine.context.PerpsController?.clearPendingTradeConfiguration(
orderForm.asset,
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending config cleared even when order execution fails

Medium Severity

clearPendingTradeConfiguration runs unconditionally after executeOrder, but executeOrder (from usePerpsOrderExecution) never throws — it catches all errors internally and resolves regardless of success or failure. This means the pending trade config (including TP/SL) is cleared even when the order fails on the exchange, contradicting the comment's stated intent of clearing only "after successful submission." If a user's order fails, their TP/SL settings are lost when they return to the order form.

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended

} finally {
// Always reset submission flag
isSubmittingRef.current = false;
Expand Down Expand Up @@ -1204,6 +1221,26 @@
),
);

const isTakeProfitPriceInvalid = Boolean(
orderForm.takeProfitPrice?.trim() &&
assetData.price > 0 &&
!isValidTakeProfitPrice(orderForm.takeProfitPrice, {
currentPrice: assetData.price,
Comment thread
michalconsensys marked this conversation as resolved.
Outdated
direction: orderForm.direction,
}),
);

const isStopLossPriceInvalid = Boolean(
orderForm.stopLossPrice?.trim() &&
assetData.price > 0 &&
!isValidStopLossPrice(orderForm.stopLossPrice, {
currentPrice: assetData.price,
direction: orderForm.direction,
}),
);

const hasInvalidTPSL = isTakeProfitPriceInvalid || isStopLossPriceInvalid;
Comment thread
cursor[bot] marked this conversation as resolved.

let rewardAnimationState = RewardAnimationState.Idle;
if (rewardsState.isLoading) {
rewardAnimationState = RewardAnimationState.Loading;
Expand Down Expand Up @@ -1418,9 +1455,33 @@
? strings('perps.tpsl.below')
: strings('perps.tpsl.above'),
})}
</Text>

Check warning on line 1458 in app/components/UI/Perps/Views/PerpsOrderView/PerpsOrderView.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

'Text' is deprecated.

See more on https://sonarcloud.io/project/issues?id=metamask-mobile&issues=AZ0aa8wSuBFMN0bhaOQq&open=AZ0aa8wSuBFMN0bhaOQq&pullRequest=27791
</View>
)}
{!hideTPSL && isTakeProfitPriceInvalid && (
<View style={styles.stopLossLiquidationWarning}>
<Text variant={TextVariant.BodySM} color={TextColor.Error}>

Check warning on line 1463 in app/components/UI/Perps/Views/PerpsOrderView/PerpsOrderView.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

'Text' is deprecated.

See more on https://sonarcloud.io/project/issues?id=metamask-mobile&issues=AZ0aa8wSuBFMN0bhaOQr&open=AZ0aa8wSuBFMN0bhaOQr&pullRequest=27791
{strings('perps.tpsl.take_profit_wrong_side_warning', {
direction:
orderForm.direction === 'long'
? strings('perps.tpsl.above')
: strings('perps.tpsl.below'),
})}
</Text>

Check warning on line 1470 in app/components/UI/Perps/Views/PerpsOrderView/PerpsOrderView.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

'Text' is deprecated.

See more on https://sonarcloud.io/project/issues?id=metamask-mobile&issues=AZ0aa8wSuBFMN0bhaOQs&open=AZ0aa8wSuBFMN0bhaOQs&pullRequest=27791
</View>
)}
{!hideTPSL && isStopLossPriceInvalid && (
<View style={styles.stopLossLiquidationWarning}>
<Text variant={TextVariant.BodySM} color={TextColor.Error}>

Check warning on line 1475 in app/components/UI/Perps/Views/PerpsOrderView/PerpsOrderView.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

'Text' is deprecated.

See more on https://sonarcloud.io/project/issues?id=metamask-mobile&issues=AZ0aa8wSuBFMN0bhaOQt&open=AZ0aa8wSuBFMN0bhaOQt&pullRequest=27791
{strings('perps.tpsl.stop_loss_wrong_side_warning', {
direction:
orderForm.direction === 'long'
? strings('perps.tpsl.below')
: strings('perps.tpsl.above'),
})}
</Text>
</View>
)}
Comment thread
cursor[bot] marked this conversation as resolved.
</View>
)}

Expand Down Expand Up @@ -1652,6 +1713,7 @@
!orderValidation.isValid ||
isPlacingOrder ||
doesStopLossRiskLiquidation ||
hasInvalidTPSL ||
isAtOICap ||
shouldBlockBecauseOfFeesLoading
}
Expand All @@ -1672,6 +1734,7 @@
!orderValidation.isValid ||
isPlacingOrder ||
doesStopLossRiskLiquidation ||
hasInvalidTPSL ||
isAtOICap ||
shouldBlockBecauseOfFeesLoading
}
Expand Down
Loading
Loading