Open
Conversation
📝 WalkthroughWalkthroughUpdates eth.zig pin to v0.3.0 and adjusts perp manager ABI and implementation: removes starting price from create_perp, adds module registration and quoteSwap ABI entries, changes quote_close_position outputs and expands PositionClosed event, and updates context, types, tests, and compiled contract artifacts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
- Update quoteClosePosition: netMargin uint256->int256, add notional return value - Remove startingSqrtPriceX96 from createPerp (now auto-derived from beacon) - Add registerModule/isModuleRegistered (replaces per-module registration) - Add quoteSwap function - Update PositionClosed event with new fields - Add ModuleRegistered event
koko1123
force-pushed
the
fix/align-contracts-v0.0.1
branch
from
e8c2361 to
598a0a5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/context.zig (1)
228-245:⚠️ Potential issue | 🟠 MajorDon't drop the new
notionalreturn value from the quote close position contract call.The function decodes six values from
quote_close_position(line 234), but only usesresult[1]throughresult[4]. The sixth value—result[5](uint256), which should benotional—is never consumed or exposed. NeitherLiveDetailsnorOpenPositionDatacontain anotionalfield, so callers cannot access this value despite the contract now providing it. Add anotionalfield to the appropriate struct and thread it through the return value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.zig` around lines 228 - 245, The contract call to perp_manager_abi.quote_close_position returns six values but the code only consumes result[1]..result[4], dropping result[5] (the notional); add a notional field to types.LiveDetails and propagate it into the returned types.OpenPositionData, then populate it from result[5] when building LiveDetails in the function that calls contract.contractRead (use the same conversions.scaleFrom6Decimals helper and cast result[5] to the appropriate integer type like you do for other results before passing to scaleFrom6Decimals), and ensure any callers/constructors of OpenPositionData accept and forward the new notional field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/abi/perp_manager_abi.zig`:
- Around line 333-385: The ABI adds ModuleRegistered event and functions
registerModule, isModuleRegistered, and quoteSwap but no SDK wrappers exist; add
wrapper functions mirroring existing patterns (see createPerp in
perp_manager.zig and quoteClosePosition in context.zig) that call the
corresponding ABI selectors (register_module_selector,
is_module_registered_selector, quote_swap_selector), marshal inputs/outputs to
Zig types, emit/consume the ModuleRegistered event as needed, and expose these
wrappers from perp_manager.zig and/or context.zig so the SDK can call
registerModule(moduleType, module), isModuleRegistered(moduleType, module) ->
bool, and quoteSwap(perpId, zeroForOne, isExactIn, amount, sqrtPriceLimitX96) ->
(bytes, int256, int256).
---
Outside diff comments:
In `@src/context.zig`:
- Around line 228-245: The contract call to
perp_manager_abi.quote_close_position returns six values but the code only
consumes result[1]..result[4], dropping result[5] (the notional); add a notional
field to types.LiveDetails and propagate it into the returned
types.OpenPositionData, then populate it from result[5] when building
LiveDetails in the function that calls contract.contractRead (use the same
conversions.scaleFrom6Decimals helper and cast result[5] to the appropriate
integer type like you do for other results before passing to
scaleFrom6Decimals), and ensure any callers/constructors of OpenPositionData
accept and forward the new notional field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bfd151b-9ac4-4c1e-8024-d3f7948ee905
📒 Files selected for processing (5)
build.zig.zonsrc/abi/perp_manager_abi.zigsrc/context.zigsrc/perp_manager.zigsrc/types.zig
💤 Files with no reviewable changes (2)
- src/perp_manager.zig
- src/types.zig
Comment on lines
+333
to
+385
| pub const module_registered: Event = .{ | ||
| .name = "ModuleRegistered", | ||
| .inputs = &.{ | ||
| .{ .name = "moduleType", .abi_type = .uint8, .indexed = true }, | ||
| .{ .name = "module", .abi_type = .address, .indexed = true }, | ||
| }, | ||
| }; | ||
|
|
||
| // -- Module registration functions -- | ||
|
|
||
| pub const register_module: Function = .{ | ||
| .name = "registerModule", | ||
| .state_mutability = .nonpayable, | ||
| .inputs = &.{ | ||
| .{ .name = "moduleType", .abi_type = .uint8 }, | ||
| .{ .name = "module", .abi_type = .address }, | ||
| }, | ||
| .outputs = &.{}, | ||
| }; | ||
| pub const register_module_selector = keccak.comptimeSelector("registerModule(uint8,address)"); | ||
|
|
||
| pub const is_module_registered: Function = .{ | ||
| .name = "isModuleRegistered", | ||
| .state_mutability = .view, | ||
| .inputs = &.{ | ||
| .{ .name = "moduleType", .abi_type = .uint8 }, | ||
| .{ .name = "module", .abi_type = .address }, | ||
| }, | ||
| .outputs = &.{ | ||
| .{ .name = "", .abi_type = .bool }, | ||
| }, | ||
| }; | ||
| pub const is_module_registered_selector = keccak.comptimeSelector("isModuleRegistered(uint8,address)"); | ||
|
|
||
| // -- Quote swap -- | ||
|
|
||
| pub const quote_swap: Function = .{ | ||
| .name = "quoteSwap", | ||
| .state_mutability = .nonpayable, | ||
| .inputs = &.{ | ||
| .{ .name = "perpId", .abi_type = .bytes32 }, | ||
| .{ .name = "zeroForOne", .abi_type = .bool }, | ||
| .{ .name = "isExactIn", .abi_type = .bool }, | ||
| .{ .name = "amount", .abi_type = .uint256 }, | ||
| .{ .name = "sqrtPriceLimitX96", .abi_type = .uint160 }, | ||
| }, | ||
| .outputs = &.{ | ||
| .{ .name = "unexpectedReason", .abi_type = .bytes }, | ||
| .{ .name = "perpDelta", .abi_type = .int256 }, | ||
| .{ .name = "usdDelta", .abi_type = .int256 }, | ||
| }, | ||
| }; | ||
| pub const quote_swap_selector = keccak.comptimeSelector("quoteSwap(bytes32,bool,bool,uint256,uint160)"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for wrapper/context usage of the new ABI surface outside the ABI declaration file.
rg -n -C3 'quote_swap|quoteSwap|register_module|registerModule|is_module_registered|isModuleRegistered|module_registered|ModuleRegistered' src -g '!src/abi/perp_manager_abi.zig'Repository: StrobeLabs/perpcity-zig-sdk
Length of output: 53
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1. Verify the new ABI symbols exist in the target file
echo "=== Checking for new ABI symbols in perp_manager_abi.zig ==="
rg -n 'quote_swap|register_module|is_module_registered|module_registered' src/abi/perp_manager_abi.zig
# 2. Check how existing aligned functions (createPerp, quoteClosePosition) are used
echo -e "\n=== Checking usage of createPerp and quoteClosePosition outside ABI ==="
rg -n 'create_perp|createPerp|quote_close|quoteClose' src -g '!src/abi/perp_manager_abi.zig' | head -20
# 3. Search entire repo (not just src) for the new symbols
echo -e "\n=== Searching entire repo for new ABI symbols ==="
rg 'quote_swap|quoteSwap|register_module|registerModule|is_module_registered|isModuleRegistered' . -g '!.git' 2>/dev/null || trueRepository: StrobeLabs/perpcity-zig-sdk
Length of output: 1848
These new ABI entries lack wrapper functions and are not exposed at the SDK level.
The registerModule, isModuleRegistered, and quoteSwap symbols are declared in the ABI but have no corresponding wrapper functions in src/perp_manager.zig or src/context.zig. In contrast, existing functions like createPerp (line 44 of perp_manager.zig) and quoteClosePosition (line 232 of context.zig) are wrapped and exposed. These new symbols should follow the same pattern to be consumable from the SDK.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/abi/perp_manager_abi.zig` around lines 333 - 385, The ABI adds
ModuleRegistered event and functions registerModule, isModuleRegistered, and
quoteSwap but no SDK wrappers exist; add wrapper functions mirroring existing
patterns (see createPerp in perp_manager.zig and quoteClosePosition in
context.zig) that call the corresponding ABI selectors
(register_module_selector, is_module_registered_selector, quote_swap_selector),
marshal inputs/outputs to Zig types, emit/consume the ModuleRegistered event as
needed, and expose these wrappers from perp_manager.zig and/or context.zig so
the SDK can call registerModule(moduleType, module),
isModuleRegistered(moduleType, module) -> bool, and quoteSwap(perpId,
zeroForOne, isExactIn, amount, sqrtPriceLimitX96) -> (bytes, int256, int256).
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)
src/context.zig (1)
228-242:⚠️ Potential issue | 🟡 MinorAdd the
notionaloutput toLiveDetailsor explicitly document why it is intentionally dropped.The tuple is decoded with all 6 elements (
result[5].uint256is the notional value), but only the first 5 are stored inLiveDetails. Thenotionalfield should either be added to the struct or the omission should be documented. Additionally, sincenetMargin(noweffective_margin) is a signedint256, it can be negative; however,calculateLeverage()already guards against this withif (effective_margin <= 0.0) return std.math.inf(f64);, so that risk is mitigated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.zig` around lines 228 - 242, The tuple decoded from contract.contractRead includes a notional at result[5].uint256 but LiveDetails currently omits it; either add a notional field to the LiveDetails type and populate it from result[5] (using the same scaling/conversion logic as other numeric fields) where LiveDetails is constructed, or add a clear comment in the contractRead handling and LiveDetails definition explaining why notional is intentionally dropped; ensure callers such as calculateLeverage() (which already guards for effective_margin <= 0.0) continue to work with the updated LiveDetails shape or documented omission.
🤖 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 `@src/context.zig`:
- Around line 228-242: The tuple decoded from contract.contractRead includes a
notional at result[5].uint256 but LiveDetails currently omits it; either add a
notional field to the LiveDetails type and populate it from result[5] (using the
same scaling/conversion logic as other numeric fields) where LiveDetails is
constructed, or add a clear comment in the contractRead handling and LiveDetails
definition explaining why notional is intentionally dropped; ensure callers such
as calculateLeverage() (which already guards for effective_margin <= 0.0)
continue to work with the updated LiveDetails shape or documented omission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 883541f5-696d-4367-b343-3e8006914aed
📒 Files selected for processing (5)
build.zig.zonsrc/abi/perp_manager_abi.zigsrc/context.zigsrc/perp_manager.zigsrc/types.zig
💤 Files with no reviewable changes (2)
- src/perp_manager.zig
- src/types.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- build.zig.zon
- Sync MockPerpManager with TS SDK: remove startingSqrtPriceX96 from CreatePerpParams, update quoteClosePosition return type (int256 netMargin, add notional), update PositionClosed event fields - Sync MockFees with TS SDK (immutable fee constants) - Enable via_ir in foundry.toml (needed for PositionClosed stack depth) - Rebuild compiled artifacts
There was a problem hiding this comment.
Actionable comments posted: 2
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)
98-139:⚠️ Potential issue | 🟠 MajorAdd the new v0.0.1 module/quote-swap surface to the mock.
The PR objective includes
registerModule,isModuleRegistered,quoteSwap, andModuleRegistered, but this mock still exposes none of them. If SDK tests useMockPerpManager, they won't exercise the new ABI at all, so the alignment withperpcity-contracts v0.0.1is still incomplete.I can sketch minimal stubs/events for this mock if that helps.
Also applies to: 220-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/contracts/MockPerpManager.sol` around lines 98 - 139, MockPerpManager.sol is missing the new v0.0.1 module/quote-swap surface: add minimal ABI-compatible stubs for registerModule(address), isModuleRegistered(address) view returns (bool), and quoteSwap(address tokenIn, address tokenOut, uint256 amountIn) view returns (uint256 amountOut) plus emit the ModuleRegistered(address module) event when registerModule is called; ensure function and event signatures match the new perpcity-contracts v0.0.1 names so SDK tests using MockPerpManager exercise the new module paths and include these additions alongside the other existing events (e.g., PerpCreated, PositionOpened, PositionClosed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/contracts/MockPerpManager.sol`:
- Around line 405-423: The PositionClosed event emitted by _emitPositionClosed
is using hardcoded zeros for the new trailing fields and reuses
pos.entryPerpDelta/entryUsdDelta as the exit deltas, making the mock
inconsistent with quoteClosePosition; update _emitPositionClosed to populate the
exit deltas and trailing fields from the provided QuoteResult (e.g., use
quote.perpDelta/quote.usdDelta or the correct QuoteResult fields for exit
deltas, set the funding and netMargin from quote.funding and quote.netMargin,
and replace the zeroed trailing fields with the corresponding values from quote)
so the emitted event matches the configured close quote.
- Around line 313-319: The createPerp function currently only sets _perpExists
and _sqrtPrices so cfgs(perpId) reads empty/default values; update createPerp to
persist the supplied CreatePerpParams into the contract's storage by populating
_configs[perpId] (or the struct that cfgs reads) with params.beacon and all
provided fields (fees, marginRatios, etc.) and any intended defaults before
emitting PerpCreated so tests reading cfgs(perpId) return the expected values;
reference createPerp, _configs, cfgs(perpId), and params to locate and assign
the proper fields.
---
Outside diff comments:
In `@tests/contracts/MockPerpManager.sol`:
- Around line 98-139: MockPerpManager.sol is missing the new v0.0.1
module/quote-swap surface: add minimal ABI-compatible stubs for
registerModule(address), isModuleRegistered(address) view returns (bool), and
quoteSwap(address tokenIn, address tokenOut, uint256 amountIn) view returns
(uint256 amountOut) plus emit the ModuleRegistered(address module) event when
registerModule is called; ensure function and event signatures match the new
perpcity-contracts v0.0.1 names so SDK tests using MockPerpManager exercise the
new module paths and include these additions alongside the other existing events
(e.g., PerpCreated, PositionOpened, PositionClosed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bad38ad6-7095-46cb-b2b8-33cad2190d54
📒 Files selected for processing (8)
tests/contracts/MockFees.soltests/contracts/MockPerpManager.soltests/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/1436c63ed2ed889e.json
✅ Files skipped from review due to trivial changes (4)
- tests/contracts/out/MockFees.sol/MockFees.json
- tests/contracts/out/MockPerpManager.sol/MockPerpManager.json
- tests/contracts/out/MockUSDC.sol/MockUSDC.json
- tests/contracts/out/MockMarginRatios.sol/MockMarginRatios.json
Comment on lines
313
to
+319
| function createPerp(CreatePerpParams calldata params) external returns (bytes32 perpId) { | ||
| // Generate a deterministic perpId | ||
| perpId = keccak256(abi.encodePacked(block.timestamp, _nextPerpNonce++)); | ||
|
|
||
| _perpExists[perpId] = true; | ||
| _sqrtPrices[perpId] = params.startingSqrtPriceX96; | ||
|
|
||
| emit PerpCreated(perpId, params.beacon, params.startingSqrtPriceX96, params.startingSqrtPriceX96); | ||
| // Use a default sqrtPriceX96 (price = 1.0 = 2^96) | ||
| uint256 defaultSqrtPrice = 79228162514264337593543950336; | ||
| _sqrtPrices[perpId] = defaultSqrtPrice; | ||
| emit PerpCreated(perpId, params.beacon, defaultSqrtPrice, defaultSqrtPrice); |
There was a problem hiding this comment.
Persist the created perp config in createPerp.
cfgs(perpId) reads _configs, but this code path only marks the perp as existing and seeds _sqrtPrices. A market created via createPerp will therefore return zeroed config fields instead of the supplied beacon/fees/marginRatios/..., which diverges from the real contract and can hide SDK regressions in integration tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/contracts/MockPerpManager.sol` around lines 313 - 319, The createPerp
function currently only sets _perpExists and _sqrtPrices so cfgs(perpId) reads
empty/default values; update createPerp to persist the supplied CreatePerpParams
into the contract's storage by populating _configs[perpId] (or the struct that
cfgs reads) with params.beacon and all provided fields (fees, marginRatios,
etc.) and any intended defaults before emitting PerpCreated so tests reading
cfgs(perpId) return the expected values; reference createPerp, _configs,
cfgs(perpId), and params to locate and assign the proper fields.
Comment on lines
+405
to
423
| function _emitPositionClosed( | ||
| uint256 posId, | ||
| Position memory pos, | ||
| QuoteResult memory quote | ||
| ) internal { | ||
| emit PositionClosed( | ||
| pos.perpId, | ||
| _sqrtPrices[pos.perpId], | ||
| 1000, // longOI | ||
| 1000, // shortOI | ||
| params.posId, | ||
| false, // wasMaker | ||
| 1000, 1000, | ||
| posId, | ||
| false, | ||
| quote.wasLiquidated, | ||
| false, // wasPartialClose | ||
| false, | ||
| pos.entryPerpDelta, | ||
| pos.entryUsdDelta, | ||
| -887220, // tickLower | ||
| 887220 // tickUpper | ||
| -887220, 887220, | ||
| // new trailing fields | ||
| 0, 0, 0, 0, 0, quote.netMargin | ||
| ); |
There was a problem hiding this comment.
Emit PositionClosed from the configured close quote.
setupQuoteResult now lets tests configure funding and netMargin, but _emitPositionClosed hardcodes the new trailing fields to zero except netMargin, and it reuses the entry deltas as the exit deltas. That makes the close event inconsistent with quoteClosePosition, so event-driven SDK checks can pass against the mock while still being wrong against the real contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/contracts/MockPerpManager.sol` around lines 405 - 423, The
PositionClosed event emitted by _emitPositionClosed is using hardcoded zeros for
the new trailing fields and reuses pos.entryPerpDelta/entryUsdDelta as the exit
deltas, making the mock inconsistent with quoteClosePosition; update
_emitPositionClosed to populate the exit deltas and trailing fields from the
provided QuoteResult (e.g., use quote.perpDelta/quote.usdDelta or the correct
QuoteResult fields for exit deltas, set the funding and netMargin from
quote.funding and quote.netMargin, and replace the zeroed trailing fields with
the corresponding values from quote) so the emitted event matches the configured
close quote.
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:netMarginchanged fromuint256toint256, addednotional(uint256) as 6th return valuestartingSqrtPriceX96fromcreatePerpparams (now auto-derived from beacon index in contract)registerModule/isModuleRegistered(replaces per-module registration functions)quoteSwapfunction andModuleRegisteredeventPositionClosedevent with new trailing fieldsTest plan
zig build testwith Zig 0.15.2)Summary by CodeRabbit
New Features
Bug Fixes
Dependencies