Open
Conversation
📝 WalkthroughWalkthroughModule registration and perp creation APIs were consolidated: multiple module-specific events/functions unified into a generic ModuleRegistered/registerModule model; starting_price was removed from perp creation; Position and Perp events/returns were extended; create_perp call sites and related types/ABI updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/perpcity_sdk/context.py (1)
160-177: Consider exposingnotionalinLiveDetailsfor future completeness.The
_notionalvalue is now captured fromquoteClosePositionbut discarded. Since the contract now returns this as a sixth value, consider adding it to theLiveDetailsdataclass if SDK consumers may need it for displaying position sizing or calculations. This remains optional if notional is not currently required by users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/perpcity_sdk/context.py` around lines 160 - 177, The code discards the sixth return value (_notional) from quoteClosePosition in _fetch_position_live_details/_fetch; add a notional field to the LiveDetails dataclass and populate it from _notional (convert to the same units as pnl/funding/net_margin, e.g., int(_notional)/1e6) so SDK consumers can access position notional; update any constructors/usages of LiveDetails to accept the new notional attribute and include it in returned instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/perpcity_sdk/context.py`:
- Around line 160-177: The code discards the sixth return value (_notional) from
quoteClosePosition in _fetch_position_live_details/_fetch; add a notional field
to the LiveDetails dataclass and populate it from _notional (convert to the same
units as pnl/funding/net_margin, e.g., int(_notional)/1e6) so SDK consumers can
access position notional; update any constructors/usages of LiveDetails to
accept the new notional attribute and include it in returned instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8624a8ea-3420-4b9a-9ecb-40e2c6d56eb1
📒 Files selected for processing (9)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.mdCHANGELOG.mdCONTRIBUTING.mdexamples/create_perp.pysrc/perpcity_sdk/abis/perp_manager.jsonsrc/perpcity_sdk/context.pysrc/perpcity_sdk/functions/perp_manager.pysrc/perpcity_sdk/types.py
💤 Files with no reviewable changes (2)
- src/perpcity_sdk/types.py
- examples/create_perp.py
- Update quoteClosePosition: netMargin uint256->int256, add notional return value - Remove startingSqrtPriceX96 from createPerp (now auto-derived from beacon) - Replace per-module registration with generic registerModule/isModuleRegistered - Add quoteSwap function - Update PositionClosed event with new fields - Update adjustNotional field names (perpDelta->usdDelta, usdLimit->perpLimit)
koko1123
force-pushed
the
fix/align-contracts-v001
branch
from
883cdfa to
31f461e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/perpcity_sdk/abis/perp_manager.json (1)
1604-1652: NewquoteSwapfunction added - consider exposing in SDK.The new
quoteSwapfunction provides quote functionality for swap operations. If this is part of the public contract API intended for SDK users, consider adding a Python wrapper function (similar to the pattern used for other quote functions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 324e5c6e-ae9a-4a37-bd03-3324d1caf05d
📒 Files selected for processing (5)
examples/create_perp.pysrc/perpcity_sdk/abis/perp_manager.jsonsrc/perpcity_sdk/context.pysrc/perpcity_sdk/functions/perp_manager.pysrc/perpcity_sdk/types.py
💤 Files with no reviewable changes (2)
- src/perpcity_sdk/types.py
- examples/create_perp.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/perpcity_sdk/context.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
71-82: Consider checking both required secrets for robustness.The check only validates
BASE_SEPOLIA_RPC_URL, but line 102 also requiresTEST_PRIVATE_KEY. If only one secret is configured, the job will proceed and likely fail during test execution.♻️ Suggested improvement to check both secrets
- name: Check if secrets are available id: secrets-check env: RPC_SECRET: ${{ secrets.BASE_SEPOLIA_RPC_URL }} + KEY_SECRET: ${{ secrets.TEST_PRIVATE_KEY }} run: | - if [ -z "$RPC_SECRET" ]; then + if [ -z "$RPC_SECRET" ] || [ -z "$KEY_SECRET" ]; then echo "::warning::Integration test secrets not configured - skipping" echo "has_secrets=false" >> "$GITHUB_OUTPUT" else echo "Integration test configuration found - running tests" echo "has_secrets=true" >> "$GITHUB_OUTPUT" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 71 - 82, The current CI step only checks BASE_SEPOLIA_RPC_URL, so modify the "secrets-check" job to validate both required secrets: BASE_SEPOLIA_RPC_URL and TEST_PRIVATE_KEY (check both $RPC_SECRET and $PRIVATE_KEY or similar env names used in the workflow). Update the env block to expose both secrets, change the run script to test that neither variable is empty (e.g., if either is missing set has_secrets=false and emit a warning; only set has_secrets=true when both are present), and keep the existing informative echo messages so downstream steps only run when both secrets exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 71-82: The current CI step only checks BASE_SEPOLIA_RPC_URL, so
modify the "secrets-check" job to validate both required secrets:
BASE_SEPOLIA_RPC_URL and TEST_PRIVATE_KEY (check both $RPC_SECRET and
$PRIVATE_KEY or similar env names used in the workflow). Update the env block to
expose both secrets, change the run script to test that neither variable is
empty (e.g., if either is missing set has_secrets=false and emit a warning; only
set has_secrets=true when both are present), and keep the existing informative
echo messages so downstream steps only run when both secrets exist.
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/isModuleRegisteredquoteSwapfunction,ModuleRegisteredeventPositionClosedevent fields andadjustNotionalfield namesTest plan
ruff check)Summary by CodeRabbit
Refactor
Chores