ENG-2072, ENG-2073: Update margin + update SL/TP#21
Merged
petar-omni merged 2 commits intomainfrom Apr 3, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR consolidates Take Profit / Stop Loss editing into a single unified action and introduces a new "Adjust Margin" feature for isolated margin positions. Core atoms are refactored to compute actions internally, UI components are made more type-safe, helper functions for margin calculations are added, and new routes and state management support the margin adjustment workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AdjustMarginForm
participant adjustMarginFormAtom
participant getAdjustMarginCalculations
participant updateMarginAtom
participant API as API Client
participant actionAtom
User->>AdjustMarginForm: Enter amount & select mode (add/remove)
AdjustMarginForm->>getAdjustMarginCalculations: Compute projections
getAdjustMarginCalculations->>getAdjustMarginCalculations: Calculate max limit, projected margin, liquidation price
getAdjustMarginCalculations-->>AdjustMarginForm: Return calculations
AdjustMarginForm->>User: Render estimated liquidation price
User->>AdjustMarginForm: Click Submit
AdjustMarginForm->>adjustMarginFormAtom: Read form state (amount, mode)
adjustMarginFormAtom->>adjustMarginFormAtom: Compute signed amount via getSignedUpdateMarginAmount
adjustMarginFormAtom->>API: Execute updateMargin(marketId, signedAmount)
API-->>adjustMarginFormAtom: Return action
adjustMarginFormAtom->>actionAtom: Store action result
actionAtom-->>AdjustMarginForm: Trigger navigation to sign
sequenceDiagram
participant User
participant TPOrSLDialog
participant editSLTPAtom
participant tpSlArgument
participant Match as Match Logic
participant API as API Client
User->>TPOrSLDialog: Submit TP/SL form
TPOrSLDialog->>editSLTPAtom: Call with settings + optional orderIds
editSLTPAtom->>tpSlArgument: Derive triggerPrice from settings
editSLTPAtom->>Match: Check which of newStopLossPrice / newTakeProfitPrice defined
Match-->>editSLTPAtom: Return actionArgs {action, args}
editSLTPAtom->>API: Execute ActionsControllerExecuteAction with computed action
API-->>editSLTPAtom: Return result
editSLTPAtom-->>TPOrSLDialog: Success/Failure
TPOrSLDialog->>User: Navigate or show error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/common/src/atoms/position-pending-actions-atom.ts (1)
33-42:⚠️ Potential issue | 🟡 MinorUse position.marginMode instead of hardcoding "isolated".
The
marginModeis hardcoded to"isolated", butposition.marginModeis available and the system supports both isolated and cross-margin modes. Either parameterize this usingposition.marginModeif both modes should be supported for updateLeverage, or add a comment explaining why only isolated mode is supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/atoms/position-pending-actions-atom.ts` around lines 33 - 42, The call to client.ActionsControllerExecuteAction hardcodes marginMode: "isolated"; change this to use the position's actual mode by passing marginMode: position.marginMode (or, if only isolated is intentionally supported, add a clear comment on why) so updateLeverage honors both isolated and cross modes; locate the call site in the function that invokes client.ActionsControllerExecuteAction where providerId: selectedProvider.id, address: wallet.currentAccount.address, action: "updateLeverage", args: { marketId: position.marketId, leverage: newLeverage, marginMode: ... } and replace the hardcoded string with position.marginMode (or document the reason for sticking to "isolated").
🧹 Nitpick comments (4)
packages/common/src/hooks/use-position-actions.ts (1)
36-43: AddMatch.orElsefor completeness.The
Match.value(...).pipe(...)chain doesn't have a terminal matcher. While this works because unmatched cases just don't modifyacc, addingMatch.orElse(() => {})would make the intent explicit and satisfy exhaustiveness if new action types are added.♻️ Add exhaustive match
Match.value(decoded).pipe( Match.when({ type: "updateLeverage" }, (v) => { acc.updateLeverage = v; }), Match.when({ type: "setTpAndSl" }, (v) => { acc.setTpAndSl = v; }), + Match.orElse(() => {}), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/hooks/use-position-actions.ts` around lines 36 - 43, The Match.value(...).pipe(...) call in use-position-actions.ts uses Match.when for "updateLeverage" and "setTpAndSl" but has no terminal case; add Match.orElse(() => {}) to the pipe to make the match exhaustive and explicit so future action types won't be silently ignored—place the orElse after the existing Match.when calls in the same chain (the Match.value(...).pipe(...) that assigns acc.updateLeverage and acc.setTpAndSl).packages/common/tests/adjust-margin.test.ts (1)
60-73: Consider adding more edge case tests for robustness.The invalid margin test only covers
projectedMargin: 0. Consider adding tests for:
- Negative
projectedMargin(should returnnull)- Invalid
amountinputs togetSignedUpdateMarginAmount(e.g.,"abc",NaN,0, negative values)🧪 Additional edge case tests
test("return null for invalid projected margin", () => { const position = { markPrice: 4_000, side: "long" as const, size: "1", }; expect( getEstimatedLiquidationPriceForProjectedMargin({ position, projectedMargin: 0, }), ).toBeNull(); + + expect( + getEstimatedLiquidationPriceForProjectedMargin({ + position, + projectedMargin: -100, + }), + ).toBeNull(); }); test("format update margin amounts for add and remove flows", () => { expect(getSignedUpdateMarginAmount({ amount: "125.5", mode: "add" })).toBe( "125.5", ); expect(getSignedUpdateMarginAmount({ amount: 125.5, mode: "remove" })).toBe( "-125.5", ); + // Invalid inputs should return null + expect(getSignedUpdateMarginAmount({ amount: "abc", mode: "add" })).toBeNull(); + expect(getSignedUpdateMarginAmount({ amount: 0, mode: "add" })).toBeNull(); + expect(getSignedUpdateMarginAmount({ amount: -50, mode: "add" })).toBe("50"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/tests/adjust-margin.test.ts` around lines 60 - 73, Add edge-case tests to cover negative projected margins and invalid amount inputs: extend the test suite for getEstimatedLiquidationPriceForProjectedMargin to assert it returns null for projectedMargin < 0 (e.g., -1) in addition to 0, and add tests for getSignedUpdateMarginAmount that pass invalid amount strings/values such as "abc", NaN, "0", and negative numbers to assert the expected failure behavior (null/throw/validation result used by your codebase); locate tests referencing getEstimatedLiquidationPriceForProjectedMargin and getSignedUpdateMarginAmount and add assertions matching the existing pattern to ensure these edge cases are covered.packages/common/src/atoms/edit-position-atoms.ts (1)
99-101: Consider usingEffect.failinstead ofEffect.dieMessagefor recoverable errors.
Effect.dieMessagecreates a defect (unrecoverable error). If a user submits the form without any TP/SL values set, this would crash rather than show a validation error. Consider whether this should be a typed error that callers can handle gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/atoms/edit-position-atoms.ts` around lines 99 - 101, Replace the unrecoverable defect thrown for missing actionArgs with a recoverable typed failure so callers can handle validation errors: in the actionArgs check (the block that currently does yield* Effect.dieMessage("No TP/SL settings provided")), return yield* Effect.fail(...) with a well-defined validation error (e.g., a ValidationError tag or specific error object/string) instead of dieMessage and propagate that error type through the surrounding effect so callers of the edit-position logic can catch and display a validation message.packages/common/src/lib/math.ts (1)
224-239: Consider usingparseFloatOrfor consistency.The file already has
parseFloatOr(line 27) andparseFloatOrZero(line 36) for safely parsing numeric strings. UsingNumber()directly here works but is inconsistent with the rest of the file's parsing approach.♻️ Optional: Use existing parseFloatOr for consistency
export const getSignedUpdateMarginAmount = ({ amount, mode, }: { amount: number | string; mode: "add" | "remove"; }) => { - const parsedAmount = typeof amount === "string" ? Number(amount) : amount; + const parsedAmount = typeof amount === "string" ? parseFloatOr(amount, NaN) : amount; const absoluteAmount = Math.abs(parsedAmount); if (!Number.isFinite(absoluteAmount) || absoluteAmount <= 0) { return null; } return (mode === "add" ? absoluteAmount : -absoluteAmount).toString(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/lib/math.ts` around lines 224 - 239, The parsing in getSignedUpdateMarginAmount uses Number(amount) directly; replace that with the existing parseFloatOr (or parseFloatOrZero if you prefer zero default) to match file-wide parsing utilities: call parseFloatOr(amount) to obtain parsedAmount, maintain Math.abs and Number.isFinite checks, preserve the early return of null for non-finite or non-positive values, and keep returning the signed value as a string; update imports/usages accordingly so getSignedUpdateMarginAmount relies on parseFloatOr instead of Number().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/widget/src/components/modules/PositionDetails/AdjustMargin/index.tsx`:
- Around line 267-293: The code incorrectly treats the value returned by
useAdjustMarginPosition as a Result; update AdjustMargin/index.tsx to stop
calling Result.isInitial/Result.isSuccess on the position variable and stop
using position.value. Keep the Result checks for market and providerBalance
only, and instead guard position by checking for null/undefined (or whatever
plain-shape sentinel the hook returns), then pass the plain position into
<AdjustMarginContent wallet={wallet} marketRef={market.value}
position={position} availableBalance={providerBalance.value.availableBalance} />
(i.e. remove .value usage for position and adjust the earlier conditional logic
accordingly).
In
`@packages/widget/src/components/modules/PositionDetails/AdjustMargin/state.tsx`:
- Around line 83-109: The code currently blocks the "remove" flow by always
requiring providerBalance; change the validation so providerBalance is only
required when values.Mode === "add": keep the position check
(registry.get(adjustMarginPositionAtom(key)) -> position) and its marginMode
check as-is, but move or guard the providerBalance check
(registry.get(selectedProviderBalancesAtom(key)) -> providerBalance) behind a
conditional that runs only when values.Mode === "add"; ensure maxAmount
computation still uses providerBalance.availableBalance for Mode === "add" and
position.margin for Mode === "remove" so remove-mode no longer fails when
providerBalance is missing.
- Around line 66-71: The Amount field currently only enforces >0; update the
validation on the Amount field (where Schema.NumberFromString and
Schema.greaterThan(...) are used) to enforce the API minimum of $10 by replacing
the greaterThan(0, ...) check with a minimum-10 check (e.g., use a
greaterThanOrEqual(10, { message: () => "Must be at least $10" }) or equivalent)
so values below $10 are rejected by the form before calling updateMargin.
In
`@packages/widget/src/components/modules/PositionDetails/Overview/Position/index.tsx`:
- Around line 266-268: The AdjustMarginDialog is rendered when
position.marginMode === "isolated" but lacks a provider capability check; update
the PendingActionSchema in use-position-actions.ts to include updateMargin, then
gate rendering by checking positionActions.updateMargin (e.g.,
positionActions.updateMargin && position.marginMode === "isolated") before
rendering <AdjustMarginDialog marketId={position.marketId} /> so it matches the
pattern used for setTpAndSl and updateLeverage.
---
Outside diff comments:
In `@packages/common/src/atoms/position-pending-actions-atom.ts`:
- Around line 33-42: The call to client.ActionsControllerExecuteAction hardcodes
marginMode: "isolated"; change this to use the position's actual mode by passing
marginMode: position.marginMode (or, if only isolated is intentionally
supported, add a clear comment on why) so updateLeverage honors both isolated
and cross modes; locate the call site in the function that invokes
client.ActionsControllerExecuteAction where providerId: selectedProvider.id,
address: wallet.currentAccount.address, action: "updateLeverage", args: {
marketId: position.marketId, leverage: newLeverage, marginMode: ... } and
replace the hardcoded string with position.marginMode (or document the reason
for sticking to "isolated").
---
Nitpick comments:
In `@packages/common/src/atoms/edit-position-atoms.ts`:
- Around line 99-101: Replace the unrecoverable defect thrown for missing
actionArgs with a recoverable typed failure so callers can handle validation
errors: in the actionArgs check (the block that currently does yield*
Effect.dieMessage("No TP/SL settings provided")), return yield* Effect.fail(...)
with a well-defined validation error (e.g., a ValidationError tag or specific
error object/string) instead of dieMessage and propagate that error type through
the surrounding effect so callers of the edit-position logic can catch and
display a validation message.
In `@packages/common/src/hooks/use-position-actions.ts`:
- Around line 36-43: The Match.value(...).pipe(...) call in
use-position-actions.ts uses Match.when for "updateLeverage" and "setTpAndSl"
but has no terminal case; add Match.orElse(() => {}) to the pipe to make the
match exhaustive and explicit so future action types won't be silently
ignored—place the orElse after the existing Match.when calls in the same chain
(the Match.value(...).pipe(...) that assigns acc.updateLeverage and
acc.setTpAndSl).
In `@packages/common/src/lib/math.ts`:
- Around line 224-239: The parsing in getSignedUpdateMarginAmount uses
Number(amount) directly; replace that with the existing parseFloatOr (or
parseFloatOrZero if you prefer zero default) to match file-wide parsing
utilities: call parseFloatOr(amount) to obtain parsedAmount, maintain Math.abs
and Number.isFinite checks, preserve the early return of null for non-finite or
non-positive values, and keep returning the signed value as a string; update
imports/usages accordingly so getSignedUpdateMarginAmount relies on parseFloatOr
instead of Number().
In `@packages/common/tests/adjust-margin.test.ts`:
- Around line 60-73: Add edge-case tests to cover negative projected margins and
invalid amount inputs: extend the test suite for
getEstimatedLiquidationPriceForProjectedMargin to assert it returns null for
projectedMargin < 0 (e.g., -1) in addition to 0, and add tests for
getSignedUpdateMarginAmount that pass invalid amount strings/values such as
"abc", NaN, "0", and negative numbers to assert the expected failure behavior
(null/throw/validation result used by your codebase); locate tests referencing
getEstimatedLiquidationPriceForProjectedMargin and getSignedUpdateMarginAmount
and add assertions matching the existing pattern to ensure these edge cases are
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72425096-77cd-4221-9ec3-166036002b23
📒 Files selected for processing (22)
packages/common/src/atoms/edit-position-atoms.tspackages/common/src/atoms/position-pending-actions-atom.tspackages/common/src/components/molecules/sign-transactions/index.tsxpackages/common/src/components/molecules/toggle-group.tsxpackages/common/src/hooks/use-edit-position.tspackages/common/src/hooks/use-position-actions.tspackages/common/src/lib/math.tspackages/common/src/services/index.tspackages/common/tests/adjust-margin.test.tspackages/dashboard/src/components/molecules/positions/index.tsxpackages/dashboard/src/components/molecules/positions/positions-tab.tsxpackages/widget/src/components/modules/Home/Positions/index.tsxpackages/widget/src/components/modules/PositionDetails/AdjustMargin/index.tsxpackages/widget/src/components/modules/PositionDetails/AdjustMargin/loading.tsxpackages/widget/src/components/modules/PositionDetails/AdjustMargin/sign.tsxpackages/widget/src/components/modules/PositionDetails/AdjustMargin/state.tsxpackages/widget/src/components/modules/PositionDetails/Overview/Position/index.tsxpackages/widget/src/components/modules/PositionDetails/Overview/adjust-margin-dialog.tsxpackages/widget/src/components/modules/PositionDetails/Overview/index.tsxpackages/widget/src/routeTree.gen.tspackages/widget/src/routes/position-details/$marketId/adjust-margin.tsxpackages/widget/src/routes/position-details/$marketId/adjust-margin_/sign.tsx
💤 Files with no reviewable changes (1)
- packages/common/src/services/index.ts
Comment on lines
+267
to
+293
| const position = useAdjustMarginPosition(formKey); | ||
| const providerBalance = useSelectedProviderBalanceResult( | ||
| wallet.currentAccount.address, | ||
| ); | ||
|
|
||
| if ( | ||
| Result.isInitial(market) || | ||
| Result.isInitial(position) || | ||
| Result.isInitial(providerBalance) | ||
| ) { | ||
| return <AdjustMarginLoading />; | ||
| } | ||
|
|
||
| if ( | ||
| !Result.isSuccess(market) || | ||
| !Result.isSuccess(position) || | ||
| !Result.isSuccess(providerBalance) | ||
| ) { | ||
| return <Navigate to="/position-details/$marketId" params={{ marketId }} />; | ||
| } | ||
|
|
||
| return ( | ||
| <AdjustMarginContent | ||
| wallet={wallet} | ||
| marketRef={market.value} | ||
| position={position.value} | ||
| availableBalance={providerBalance.value.availableBalance} |
There was a problem hiding this comment.
Stop treating position as a Result.
useAdjustMarginPosition is implemented as a plain useAtomValue(...) wrapper in packages/widget/src/components/modules/PositionDetails/AdjustMargin/state.tsx:222-226, but this code runs Result.isInitial/isSuccess against it and then reads position.value. As written, the non-success branch will win and the adjust-margin route will bounce back to /position-details/$marketId instead of rendering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/widget/src/components/modules/PositionDetails/AdjustMargin/index.tsx`
around lines 267 - 293, The code incorrectly treats the value returned by
useAdjustMarginPosition as a Result; update AdjustMargin/index.tsx to stop
calling Result.isInitial/Result.isSuccess on the position variable and stop
using position.value. Keep the Result checks for market and providerBalance
only, and instead guard position by checking for null/undefined (or whatever
plain-shape sentinel the hook returns), then pass the plain position into
<AdjustMarginContent wallet={wallet} marketRef={market.value}
position={position} availableBalance={providerBalance.value.availableBalance} />
(i.e. remove .value usage for position and adjust the earlier conditional logic
accordingly).
Collaborator
Author
There was a problem hiding this comment.
useAdjustMarginPosition() wraps useAtomValue() and returns Result<PositionDto, ...> so this code is legit. check again @coderabbitai
There was a problem hiding this comment.
🧩 Analysis chain
Let me inspect the actual implementation to verify.
🏁 Script executed:
#!/bin/bash
# Check the useAdjustMarginPosition and adjustMarginPositionAtom implementations
rg -n "useAdjustMarginPosition|adjustMarginPositionAtom" --type=ts --type=tsx -A 10
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for useAdjustMarginPosition implementation
rg -n "useAdjustMarginPosition|adjustMarginPositionAtom" -A 10 --glob="*.ts" --glob="*.tsx"
Length of output: 7518
@petar-omni, you're right — I apologize for the incorrect review comment. adjustMarginPositionAtom is defined as a runtimeAtom.atom(Effect.fn(...)) (Line 48 of state.tsx), so useAtomValue correctly surfaces it as a Result<PositionDto, ...>. The Result.isInitial, Result.isSuccess, and position.value usages in the component are all valid. My original concern was mistaken.
packages/widget/src/components/modules/PositionDetails/AdjustMargin/state.tsx
Show resolved
Hide resolved
packages/widget/src/components/modules/PositionDetails/AdjustMargin/state.tsx
Show resolved
Hide resolved
packages/widget/src/components/modules/PositionDetails/Overview/Position/index.tsx
Show resolved
Hide resolved
Philippoes
approved these changes
Apr 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI/UX Improvements
Refactor