fix: align SDK with perpcity-contracts v0.0.1#41
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughVersion bumped to 0.5.0. PerpManager ABI consolidated module registration functions into a single registerModule function. Notional field added to quote results across codebase. StartingSqrtPriceX96 removed from CreatePerpParams. PositionClosed event enriched with additional position data. MockPerpManager contract expanded with setup and configuration methods. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Comment |
- Update quoteClosePosition to return 6 values (added notional, netMargin changed from uint256 to int256) - Remove startingSqrtPriceX96 from CreatePerpParams (now auto-derived from beacon index in contract) - Rename AdjustNotionalParams fields (perpDelta->usdDelta, usdLimit->perpLimit) - Replace individual module registration functions with generic registerModule/isModuleRegistered - Update PositionClosed event with new trailing fields and renames - Add quoteSwap function to ABI - Update mock contracts and integration tests - Bump version to 0.5.0
koko1123
force-pushed
the
koko/v0.0.1-contract-compat
branch
from
1a19ab5 to
0e026b1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/contracts/MockPerpManager.sol (1)
369-375:⚠️ Potential issue | 🟠 MajorEmit real close-side values in
PositionClosed.
_emitPositionClosedstill reportswasMaker = false, hardcodes tick bounds, and reuses the entry deltas asexitPerpDelta/exitUsdDelta. After the v0.0.1 event rename, that means the mock can emit close events the real contract would never produce, so event-based integration coverage can pass against impossible data.Suggested direction
_positions[posId].perpId = perpId; _positions[posId].margin = uint256(params.margin); _positions[posId].marginRatios = MarginRatios(100000, 1000000, 50000); + _positions[posId].makerDetails.tickLower = params.tickLower; + _positions[posId].makerDetails.tickUpper = params.tickUpper; + _positions[posId].makerDetails.liquidity = params.liquidity; _positionExists[posId] = true; @@ function _emitPositionClosed( uint256 posId, Position memory pos, QuoteResult memory quote ) internal { + bool wasMaker = pos.makerDetails.liquidity != 0; emit PositionClosed( pos.perpId, _sqrtPrices[pos.perpId], 1000, 1000, posId, - false, + wasMaker, quote.wasLiquidated, false, - pos.entryPerpDelta, - pos.entryUsdDelta, - -887220, 887220, + -pos.entryPerpDelta, + -pos.entryUsdDelta, + wasMaker ? pos.makerDetails.tickLower : int24(-887220), + wasMaker ? pos.makerDetails.tickUpper : int24(887220), 0, 0, 0, 0, 0, quote.netMargin ); }Also applies to: 405-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/contracts/MockPerpManager.sol` around lines 369 - 375, The mock emits impossible PositionClosed events: _emitPositionClosed is using hardcoded tick bounds, sets wasMaker = false, and reuses entry deltas (from _positions/_quoteResults) as exitPerpDelta/exitUsdDelta; update the mock to compute realistic close-side fields before emitting PositionClosed by deriving wasMaker from the stored _positions[posId].isMaker (or an equivalent flag), compute exitTickLower/exitTickUpper from the actual closing tick range instead of hardcoding, and set exitPerpDelta/exitUsdDelta using realistic exit deltas (e.g., calculate differences between stored entry values in _positions[posId] and actual close values or derive from _quoteResults[posId].exit values) so the emitted PositionClosed event matches what the real contract would produce; adjust the same logic in the other block (lines referenced around 405-423) to keep behavior consistent.
🧹 Nitpick comments (2)
src/__tests__/integration/context.test.ts (1)
33-40: Add one signednetMarginfixture.This setup only exercises a positive
netMargin, but v0.0.1 changed that field toint256. A second case withnetMargin < 0would lock in the new decoding/formatting path as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/integration/context.test.ts` around lines 33 - 40, Add a second test fixture that calls setupMockQuoteResult with the same parameters but a negative netMargin value to exercise the int256 path; specifically duplicate the existing call that uses TEST_POSITION_ID and change the netMargin argument (the third/fourth numeric args ordering as in the call to setupMockQuoteResult) to a negative BigInt (e.g., -545_000000n) and adjust any related comments to indicate "netMargin: -545 USDC" so the test covers decoding/formatting for negative netMargin values.src/abis/perp-manager.ts (1)
252-266: Consider sourcingPERP_MANAGER_ABIfrom generated contract artifacts.This file had to hand-edit events, tuple members, and return shapes in several places again. Importing a generated ABI from
perpcity-contracts(or a checked-in artifact derived from it) would remove a recurring drift point and keep viem’s named tuple types aligned on future contract bumps.Also applies to: 464-523, 709-737, 1090-1111, 1436-1669
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/abis/perp-manager.ts` around lines 252 - 266, Replace the hand-edited PERP_MANAGER_ABI by importing the generated contract artifact (e.g., from the perpcity-contracts package or a checked-in JSON artifact) and use that exported ABI where PERP_MANAGER_ABI is referenced (including places referencing events/tuples like ModuleRegistered); remove the inline/hand-written ABI constant and update any references to use the imported artifact name, ensuring viem named-tuple types are derived from that artifact; if necessary adjust the import/export (e.g., export const PERP_MANAGER_ABI = artifact.abi) so the same single source-of-truth ABI is used across the file and the other regions noted (around the other ranges) to prevent future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/contracts/MockPerpManager.sol`:
- Around line 369-375: The mock emits impossible PositionClosed events:
_emitPositionClosed is using hardcoded tick bounds, sets wasMaker = false, and
reuses entry deltas (from _positions/_quoteResults) as
exitPerpDelta/exitUsdDelta; update the mock to compute realistic close-side
fields before emitting PositionClosed by deriving wasMaker from the stored
_positions[posId].isMaker (or an equivalent flag), compute
exitTickLower/exitTickUpper from the actual closing tick range instead of
hardcoding, and set exitPerpDelta/exitUsdDelta using realistic exit deltas
(e.g., calculate differences between stored entry values in _positions[posId]
and actual close values or derive from _quoteResults[posId].exit values) so the
emitted PositionClosed event matches what the real contract would produce;
adjust the same logic in the other block (lines referenced around 405-423) to
keep behavior consistent.
---
Nitpick comments:
In `@src/__tests__/integration/context.test.ts`:
- Around line 33-40: Add a second test fixture that calls setupMockQuoteResult
with the same parameters but a negative netMargin value to exercise the int256
path; specifically duplicate the existing call that uses TEST_POSITION_ID and
change the netMargin argument (the third/fourth numeric args ordering as in the
call to setupMockQuoteResult) to a negative BigInt (e.g., -545_000000n) and
adjust any related comments to indicate "netMargin: -545 USDC" so the test
covers decoding/formatting for negative netMargin values.
In `@src/abis/perp-manager.ts`:
- Around line 252-266: Replace the hand-edited PERP_MANAGER_ABI by importing the
generated contract artifact (e.g., from the perpcity-contracts package or a
checked-in JSON artifact) and use that exported ABI where PERP_MANAGER_ABI is
referenced (including places referencing events/tuples like ModuleRegistered);
remove the inline/hand-written ABI constant and update any references to use the
imported artifact name, ensuring viem named-tuple types are derived from that
artifact; if necessary adjust the import/export (e.g., export const
PERP_MANAGER_ABI = artifact.abi) so the same single source-of-truth ABI is used
across the file and the other regions noted (around the other ranges) to prevent
future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e9c4c38-96aa-4711-be8c-39d95c1ad8c5
📒 Files selected for processing (17)
package.jsonsrc/__tests__/helpers/anvil-setup.tssrc/__tests__/integration/context.test.tssrc/abis/perp-manager.tssrc/context.tssrc/functions/open-position.tssrc/functions/perp-manager.tssrc/functions/position.tssrc/types/entity-data.tstests/contracts/MockPerpManager.soltests/contracts/cache/solidity-files-cache.jsontests/contracts/foundry.tomltests/contracts/out/MockFees.sol/MockFees.jsontests/contracts/out/MockMarginRatios.sol/MockMarginRatios.jsontests/contracts/out/MockPerpManager.sol/MockPerpManager.jsontests/contracts/out/MockUSDC.sol/MockUSDC.jsontests/contracts/out/build-info/c47e4dff3ecab415.json
💤 Files with no reviewable changes (1)
- src/types/entity-data.ts
Add tests/contracts/out/ and tests/contracts/cache/ to .gitignore. Add forge build step to CI before integration tests.
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
1 participant
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
quoteClosePositionreturn type (6 values,int256 netMargin, addednotional)startingSqrtPriceX96fromCreatePerpParams(now auto-derived from beacon index)registerModule/isModuleRegisteredPositionClosedevent to match v0.0.1 (new trailing fields, field renames)quoteSwapto ABITest plan
pnpm lintpasses (0 issues)pnpm testpasses (187/187 unit tests)pnpm test:integrationpasses (38/38 integration tests)Summary by CodeRabbit
Release Notes
New Features
Changes
Version