Skip to content

fix: issue where certain profitable positions cannot be closed#5613

Open
therealemjy wants to merge 1 commit into
mainfrom
fix/trade-close-profit
Open

fix: issue where certain profitable positions cannot be closed#5613
therealemjy wants to merge 1 commit into
mainfrom
fix/trade-close-profit

Conversation

@therealemjy
Copy link
Copy Markdown
Member

Changes

  • fixed an issue where a profitable position that uses the same token as DSA and long could not be closed

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 3, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
dapp-preview Error Error Jun 4, 2026 8:42am
dapp-testnet Ready Ready Preview Jun 4, 2026 8:42am
venus.io Ready Ready Preview Jun 4, 2026 8:42am

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 3, 2026

🦋 Changeset detected

Latest commit: bd063e0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@venusprotocol/evm Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jun 3, 2026

Greptile Summary

This PR fixes a bug where profitable positions using the same token as both the DSA and the long (or short) token could not be closed, because the existing code tried to fetch a redundant swap quote that the router would reject.

  • Two new branches are inserted at the top of the PnL calculation block in getTradeReduceSwapQuotes: when longToken === dsaToken and there is a profit, pnlDsaTokens is set directly to longProfitAmountDeltaTokens (no swap needed); when shortToken === dsaToken and there is a loss, pnlDsaTokens is set to -shortLossAmountDeltaTokens (no swap needed). Both new branches correctly use .isGreaterThan(0) rather than a bare BigNumber truthiness check, and are mutually exclusive with the existing swap-quote-driven branches.
  • Two new unit tests cover the added paths, with mock values that are arithmetically consistent with the expected outputs.

Confidence Score: 5/5

The change is safe to merge; it adds two narrowly-scoped branches that are guarded by explicit .isGreaterThan(0) checks and are consistent with the swap-fetching guards already present in the function.

The new branches correctly bypass unnecessary swap quotes when the token identities overlap, use proper BigNumber comparison methods, and are covered by two new unit tests with arithmetically verified expected values. No existing branches are modified, and the swap-fetching guards at lines 129 and 143–147 remain consistent with the PnL branches they correspond to.

No files require special attention.

Important Files Changed

Filename Overview
apps/evm/src/clients/api/queries/getTradeReduceSwapQuotes/index.ts Adds two new branches to handle PnL calculation when longToken equals dsaToken (profit) or shortToken equals dsaToken (loss), skipping the unnecessary swap in each case. Logic is mutually exclusive with the existing swap-quote-based branches and consistent with the swap-fetching guards at lines 129 and 143–147.
apps/evm/src/clients/api/queries/getTradeReduceSwapQuotes/tests/index.spec.ts Adds two new test cases covering the long=DSA profit path (expects pnlDsaTokens=7, no profitSwapQuote, 2 swap calls) and the short=DSA loss path (expects pnlDsaTokens=-1, no lossSwapQuote, 2 swap calls). Both test expectations are arithmetically correct given the mock inputs.
apps/evm/src/libs/errors/handleError/index.ts Converts BaseError import from viem to a type-only import; no runtime behaviour change.
.changeset/angry-ideas-heal.md Adds a patch-level changeset entry for the fix.

Reviews (3): Last reviewed commit: "fix: issue where certain profitable posi..." | Re-trigger Greptile

Comment thread apps/evm/src/clients/api/queries/getTradeReduceSwapQuotes/index.ts
Comment thread apps/evm/src/clients/api/queries/getTradeReduceSwapQuotes/__tests__/index.spec.ts Outdated
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 3, 2026

Coverage Report for ./apps/evm

Status Category Percentage Covered / Total
🔵 Lines 81.32% 45830 / 56355
🔵 Statements 81.32% 45830 / 56355
🔵 Functions 62.21% 647 / 1040
🔵 Branches 72.34% 5175 / 7153
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
apps/evm/src/clients/api/queries/getTradeReduceSwapQuotes/index.ts 99.22% 80% 100% 99.22% 1
apps/evm/src/libs/errors/handleError/index.ts 92.1% 33.33% 100% 92.1% 3, 32-33
apps/evm/src/pages/Trade/ReduceForm/index.tsx 98.26% 96.1% 100% 98.26% 77, 314-317
Generated in workflow #13565 for commit bd063e0 by the Vitest Coverage Report Action

@therealemjy
Copy link
Copy Markdown
Member Author

@greptile

Comment thread apps/evm/src/clients/api/queries/getTradeReduceSwapQuotes/index.ts Outdated
Comment thread apps/evm/src/clients/api/queries/getTradeReduceSwapQuotes/index.ts Outdated
@therealemjy therealemjy force-pushed the fix/trade-close-profit branch from d1315e8 to c025648 Compare June 3, 2026 14:28
@therealemjy
Copy link
Copy Markdown
Member Author

@greptile

Copy link
Copy Markdown
Contributor

@cuzz-venus cuzz-venus left a comment

Choose a reason for hiding this comment

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

/lgtm

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants