FEDEV-57: Tradebox UX upgrade #2073
Conversation
Deploying gmx-interface-home with
|
| Latest commit: |
3a99e40
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7c3126ac.gmx-interface-home.pages.dev |
| Branch Preview URL: | https://tradebox-ux-update.gmx-interface-home.pages.dev |
Deploying gmx-interface with
|
| Latest commit: |
3a99e40
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d314b12f.gmx-interface.pages.dev |
| Branch Preview URL: | https://tradebox-ux-update.gmx-interface.pages.dev |
|
bugbot review |
|
Skipping Bugbot: Bugbot is disabled for this repository |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| </span> | ||
| ); | ||
| }, [order]); | ||
|
|
There was a problem hiding this comment.
Bug: TP/SL receive amount calculated from size
Receive is derived from order.sizeDeltaUsd and converted using targetCollateralToken.prices.minPrice, but sizeDeltaUsd represents position size reduction, not the collateral/token amount the user receives. This can massively overstate the receive amount (especially for leveraged positions) and mislead users about TP/SL outcomes.
Additional Locations (1)
| const pnlPercentage = position.collateralUsd > 0n ? bigMath.mulDiv(pnlUsd, 10000n, position.collateralUsd) : 0n; | ||
|
|
||
| return { pnlUsd, pnlPercentage }; | ||
| }, [order, position]); |
There was a problem hiding this comment.
Bug: Division by zero in TP/SL PnL
estimatedPnl divides by entryPrice via bigMath.mulDiv(..., entryPrice). If position.entryPrice is unset or 0n (e.g., not fully loaded or opening state), this can throw at runtime and break the TP/SL modal rendering.
Additional Locations (1)
| import { useLocalStorageSerializeKey } from "lib/localStorage"; | ||
| import { TradeType } from "sdk/types/trade"; | ||
|
|
||
| export const BEST_POOL_MARKER = "BEST_POOL"; |
There was a problem hiding this comment.
Can we avoid pseudo addresses and just store bestPool in some variable instead?
|
|
||
| const orderSizeInTokens = | ||
| increaseAmounts?.sizeDeltaInTokens ?? | ||
| (positionIndexToken && (sizeDeltaUsd ?? order.sizeDeltaUsd) |
There was a problem hiding this comment.
too much independent checks
let's rewrite it to more controlled If-s
like
if (sizeDeltaUsd && triggerPrice) {
....
} else {
... use order values
}
|
|
||
| const collateralDeltaAmountForTpSl = | ||
| increaseAmounts?.collateralDeltaAmount ?? | ||
| (isLimitOrStopIncrease && order ? order.initialCollateralDeltaAmount : undefined) ?? |
There was a problem hiding this comment.
shouldn't isLimitOrStopIncrease be the first check here?
| (isLimitOrStopIncrease && order ? order.initialCollateralDeltaAmount : undefined) ?? | ||
| 0n; | ||
|
|
||
| const totalCollateralAmountForTpSl = (existingPosition?.collateralAmount ?? 0n) + collateralDeltaAmountForTpSl; |
There was a problem hiding this comment.
would be better to move all calcs to pure function(s) and just use it in hooks for better testability
| updatedAtBlock: 0n, | ||
| }); | ||
|
|
||
| const nextEntryPrice = |
There was a problem hiding this comment.
same here, would be good to move it to pure functions
| userReferralInfo?.referralCodeForTxn, | ||
| ]); | ||
|
|
||
| const hasTpSlValues = typeof tpPriceField.value === "bigint" || typeof slPriceField.value === "bigint"; |
There was a problem hiding this comment.
hard to read these lines, can we simplify these checks somehow?
| } | ||
| } | ||
|
|
||
| let closestSl: PositionOrderInfo | undefined; |
There was a problem hiding this comment.
let's also move it all to pure functions
| const usdValue = (parsedInput * markPrice) / (10n ** BigInt(USD_DECIMALS) * visualMultiplier); | ||
| return formatUsd(usdValue, { fallbackToZero: true }) ?? "$0.00"; | ||
| } else { | ||
| const tokenValue = (parsedInput * visualMultiplier * 10n ** BigInt(indexToken?.decimals ?? 18)) / markPrice; |
There was a problem hiding this comment.
please avoid magic numbers
maybe wrapping this into a functions would help also
Main changes:
Updated Tradebox:
Other changes: