Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces comprehensive AMM (Automated Market Maker) configuration management features, including CLI scripts for creating and viewing AMM configurations, supporting utilities for artifact and ID resolution, transaction builders, and integration/unit tests validating end-to-end workflows across localnet and network deployments. Changes
Sequence DiagramsequenceDiagram
participant User as CLI User
participant Script as amm-create Script
participant Utils as AMM Utils
participant Artifacts as Artifacts/Tooling
participant SuiClient as Sui Client
participant Network as Sui Network
participant Results as Results Parser
User->>Script: Execute with parameters
Script->>Utils: resolveAmmPackageId()
Utils->>Artifacts: Load publish artifacts
Artifacts-->>Utils: Return package ID
Script->>Utils: resolveAmmAdminCapId()
Utils->>Artifacts: Query artifacts or Sui client
Artifacts-->>Utils: Return admin cap ID
Script->>Utils: resolvePythPriceFeedIdHex()
Utils->>Artifacts: Load mock artifacts (localnet)
Artifacts-->>Utils: Return feed ID (or fallback)
Script->>Script: buildCreateAmmConfigTransaction()
Script->>SuiClient: Execute/devInspect transaction
SuiClient->>Network: Submit transaction
Network-->>SuiClient: Transaction result
SuiClient-->>Script: Execution result
Script->>Results: Parse created artifacts
Results-->>Script: Extract AMM config object ID
Script->>SuiClient: getAmmConfigOverview(configId)
SuiClient->>Network: Fetch object state
Network-->>SuiClient: AMM config overview
SuiClient-->>Script: Config details
Script->>User: Output (JSON or human-readable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/dapp/src/scripts/owner/amm-create.ts (1)
6-11: ReuseAMM_CONFIG_TYPE_SUFFIXinstead of a second literal.This script hardcodes
::manager::AMMConfigeven though the domain layer already exports the same suffix. Pulling the shared constant in here avoids drift if the Move type path changes again.♻️ Small cleanup
import { + AMM_CONFIG_TYPE_SUFFIX, DEFAULT_BASE_SPREAD_BPS, DEFAULT_VOLATILITY_MULTIPLIER_BPS, getAmmConfigOverview, resolveAmmConfigInputs } from "@sui-amm/domain-core/models/amm" @@ const createdAmmConfig = findCreatedArtifactBySuffix( createdArtifacts, - "::manager::AMMConfig" + AMM_CONFIG_TYPE_SUFFIX )Also applies to: 101-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dapp/src/scripts/owner/amm-create.ts` around lines 6 - 11, Import the shared AMM_CONFIG_TYPE_SUFFIX from "@sui-amm/domain-core/models/amm" and replace the hardcoded literal "::manager::AMMConfig" wherever it is used in this script (e.g. the places that construct Move type paths around resolveAmmConfigInputs and getAmmConfigOverview) with AMM_CONFIG_TYPE_SUFFIX so the script reuses the domain constant and avoids drift; update all other occurrences (including the ones noted around the later block) to use the constant as well.
🤖 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/dapp/src/scripts/owner/test-integration/amm-create.test.ts`:
- Around line 80-82: Capture the result of context.publishPackage (e.g., const
publishRes = await context.publishPackage(...)) instead of discarding it,
extract the package digest from that result, then derive the expected admin
capability id from that digest and assert adminCapId === expectedAdminCapId;
replace the current non-empty check of adminCapId with this equality assertion
and use existing helpers (e.g., any compute/derive function for admin cap ids or
a context.getAdminCapIdForDigest/computeAdminCapId helper) to compute the
expected value from publishRes.digest.
In `@packages/dapp/src/scripts/user/test-integration/amm-view.test.ts`:
- Around line 73-153: The test currently creates only one AMM config so it can't
verify the "latest" fallback — modify the test ("renders the latest AMM config
snapshot when no id is provided") to create a second AMM config after the first
(reuse buildCreateAmmConfigTransaction, context.signAndExecuteTransaction and
ensureCreatedObject/AMM_CONFIG_TYPE_SUFFIX to obtain the second ammConfigId and
its initialSharedVersion via extractInitialSharedVersion), then run amm-view
(createSuiScriptRunner/resolveUserScriptPath("amm-view") and
parseJsonFromScriptOutput) and assert that parsed.ammConfig.configId equals the
second config's id and parsed.initialSharedVersion equals the second config's
initialSharedVersion (also keep the existing field assertions such as
baseSpreadBps/volatilityMultiplierBps/useLaser/pythPriceFeedIdHex using
normalizeHex).
In `@packages/dapp/src/utils/amm.ts`:
- Around line 90-123: Wrap the body of resolveLocalnetFeedIdFromArtifacts in a
try/catch so that any error from readArtifact (including malformed JSON parse
errors) is caught and the function returns undefined, ensuring
resolveLocalnetPythPriceFeedIdHex falls back to
logLocalnetPlaceholderFeedIdFallback and returns
DEFAULT_LOCALNET_PYTH_PRICE_FEED_ID; keep the existing call to
findPriceFeedIdFromMockArtifact(mockArtifact, desiredLabel) inside the try and
optionally log the caught error for debugging.
In `@packages/domain/core/src/ptb/amm.ts`:
- Around line 28-57: Add a guard in the exported builder functions to validate
that pythPriceFeedIdBytes is exactly 32 bytes long and throw a clear synchronous
error if not; specifically, in buildCreateAmmConfigTransaction (and likewise in
buildUpdateAmmConfigTransaction) check Array.isArray(pythPriceFeedIdBytes) and
pythPriceFeedIdBytes.length === 32 at the start of the function and throw a
TypeError (with context like "pythPriceFeedIdBytes must be a 32-byte array") if
the check fails so misuse fails locally and deterministically before
constructing the transaction.
In `@packages/domain/node/src/amm.ts`:
- Around line 23-29: The current resolvers fetch artifacts before honoring
explicit overrides causing valid user-supplied IDs (e.g. --amm-package-id) to
still trigger artifact lookups; update the logic in the AMM resolvers so they
short-circuit: first check if ammPackageId (or ammConfigId / adminCapId) is
provided and if so immediately return normalizeIdOrThrow(...) without calling
getLatestDeploymentFromArtifact(AMM_PACKAGE_NAME)(networkName); only call
getLatestDeploymentFromArtifact when the override is absent and then pass the
discovered packageId/configId/capId into normalizeIdOrThrow. Ensure the same
change is applied to resolveAmmConfigId and resolveAmmAdminCapId.
---
Nitpick comments:
In `@packages/dapp/src/scripts/owner/amm-create.ts`:
- Around line 6-11: Import the shared AMM_CONFIG_TYPE_SUFFIX from
"@sui-amm/domain-core/models/amm" and replace the hardcoded literal
"::manager::AMMConfig" wherever it is used in this script (e.g. the places that
construct Move type paths around resolveAmmConfigInputs and
getAmmConfigOverview) with AMM_CONFIG_TYPE_SUFFIX so the script reuses the
domain constant and avoids drift; update all other occurrences (including the
ones noted around the later block) to use the constant as well.
🪄 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: d79341fd-4af6-4ffd-b56d-523da0c63111
📒 Files selected for processing (10)
packages/dapp/src/scripts/owner/amm-create.tspackages/dapp/src/scripts/owner/test-integration/amm-create.test.tspackages/dapp/src/scripts/user/amm-view.tspackages/dapp/src/scripts/user/test-integration/amm-view.test.tspackages/dapp/src/utils/amm.tspackages/dapp/src/utils/test/amm-ptb.test.tspackages/dapp/src/utils/test/amm.test.tspackages/domain/core/src/models/amm.tspackages/domain/core/src/ptb/amm.tspackages/domain/node/src/amm.ts
| await context.publishPackage("prop-amm", publisher, { | ||
| withUnpublishedDependencies: true | ||
| }) |
There was a problem hiding this comment.
Assert adminCapId against the package published in this test.
Lines 80-82 discard the publish metadata, and Lines 115-117 only check that adminCapId is non-empty. A resolver or serialization bug can return a stale/unrelated cap ID and this test would still pass. Capture the publish digest and compare the output to the admin cap derived from that digest.
Also applies to: 115-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dapp/src/scripts/owner/test-integration/amm-create.test.ts` around
lines 80 - 82, Capture the result of context.publishPackage (e.g., const
publishRes = await context.publishPackage(...)) instead of discarding it,
extract the package digest from that result, then derive the expected admin
capability id from that digest and assert adminCapId === expectedAdminCapId;
replace the current non-empty check of adminCapId with this equality assertion
and use existing helpers (e.g., any compute/derive function for admin cap ids or
a context.getAdminCapIdForDigest/computeAdminCapId helper) to compute the
expected value from publishRes.digest.
| it("renders the latest AMM config snapshot when no id is provided", async () => { | ||
| await testEnv.withTestContext("user-amm-view", async (context) => { | ||
| const publisher = context.createAccount("publisher") | ||
| await context.fundAccount(publisher, { minimumCoinObjects: 2 }) | ||
|
|
||
| const publishArtifacts = await context.publishPackage( | ||
| "prop-amm", | ||
| publisher, | ||
| { withUnpublishedDependencies: true } | ||
| ) | ||
| const rootArtifact = pickRootPublishArtifact(publishArtifacts) | ||
| const adminCapId = await resolveAmmAdminCapIdFromPublishDigest({ | ||
| publishDigest: rootArtifact.digest, | ||
| suiClient: context.suiClient | ||
| }) | ||
|
|
||
| const baseSpreadBps = 37n | ||
| const volatilityMultiplierBps = 420n | ||
| const useLaser = true | ||
| const pythPriceFeedIdHex = DEFAULT_LOCALNET_PYTH_PRICE_FEED_ID | ||
| const createTransaction = buildCreateAmmConfigTransaction({ | ||
| packageId: rootArtifact.packageId, | ||
| adminCapId, | ||
| baseSpreadBps, | ||
| volatilityMultiplierBps, | ||
| useLaser, | ||
| pythPriceFeedIdBytes: parsePythPriceFeedIdBytes(pythPriceFeedIdHex) | ||
| }) | ||
|
|
||
| const createResult = await context.signAndExecuteTransaction( | ||
| createTransaction, | ||
| publisher | ||
| ) | ||
| await context.waitForFinality(createResult.digest) | ||
|
|
||
| const createdConfig = ensureCreatedObject( | ||
| AMM_CONFIG_TYPE_SUFFIX, | ||
| createResult | ||
| ) | ||
| const ammConfigId = createdConfig.objectId | ||
| const initialSharedVersion = extractInitialSharedVersion(createdConfig) | ||
| if (!initialSharedVersion) { | ||
| throw new Error( | ||
| "Expected AMM config to include shared version metadata." | ||
| ) | ||
| } | ||
|
|
||
| const scriptRunner = createSuiScriptRunner(context) | ||
| const result = await scriptRunner.runScript( | ||
| resolveUserScriptPath("amm-view"), | ||
| { | ||
| account: publisher, | ||
| args: { json: true } | ||
| } | ||
| ) | ||
|
|
||
| expect(result.exitCode).toBe(0) | ||
|
|
||
| const parsed = parseJsonFromScriptOutput<AmmViewOutput>( | ||
| result.stdout, | ||
| "amm-view output" | ||
| ) | ||
| if (!parsed.ammConfig) { | ||
| throw new Error("amm-view output did not include ammConfig.") | ||
| } | ||
| if (!parsed.initialSharedVersion) { | ||
| throw new Error("amm-view output did not include shared version.") | ||
| } | ||
|
|
||
| expect(parsed.ammConfig.configId).toBe(ammConfigId) | ||
| expect(parsed.ammConfig.baseSpreadBps).toBe(baseSpreadBps.toString()) | ||
| expect(parsed.ammConfig.volatilityMultiplierBps).toBe( | ||
| volatilityMultiplierBps.toString() | ||
| ) | ||
| expect(parsed.ammConfig.useLaser).toBe(useLaser) | ||
| expect(parsed.ammConfig.tradingPaused).toBe(false) | ||
| expect(normalizeHex(parsed.ammConfig.pythPriceFeedIdHex)).toBe( | ||
| normalizeHex(pythPriceFeedIdHex) | ||
| ) | ||
| expect(parsed.initialSharedVersion).toBe(initialSharedVersion) | ||
| }) |
There was a problem hiding this comment.
This doesn't actually verify the "latest" fallback.
Only one AMM config is created here, so the test still passes if amm-view returns the oldest or any arbitrary config when no id is supplied. Seed two configs and assert the second one is the snapshot that comes back.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dapp/src/scripts/user/test-integration/amm-view.test.ts` around
lines 73 - 153, The test currently creates only one AMM config so it can't
verify the "latest" fallback — modify the test ("renders the latest AMM config
snapshot when no id is provided") to create a second AMM config after the first
(reuse buildCreateAmmConfigTransaction, context.signAndExecuteTransaction and
ensureCreatedObject/AMM_CONFIG_TYPE_SUFFIX to obtain the second ammConfigId and
its initialSharedVersion via extractInitialSharedVersion), then run amm-view
(createSuiScriptRunner/resolveUserScriptPath("amm-view") and
parseJsonFromScriptOutput) and assert that parsed.ammConfig.configId equals the
second config's id and parsed.initialSharedVersion equals the second config's
initialSharedVersion (also keep the existing field assertions such as
baseSpreadBps/volatilityMultiplierBps/useLaser/pythPriceFeedIdHex using
normalizeHex).
| const resolveLocalnetFeedIdFromArtifacts = async ({ | ||
| desiredLabel | ||
| }: { | ||
| desiredLabel: string | ||
| }) => { | ||
| const mockArtifact = await readArtifact<MockArtifact>(mockArtifactPath, {}) | ||
|
|
||
| return findPriceFeedIdFromMockArtifact(mockArtifact, desiredLabel) | ||
| } | ||
|
|
||
| const logLocalnetPlaceholderFeedIdFallback = (desiredLabel: string) => { | ||
| logWarning( | ||
| `No localnet feed artifact found for ${desiredLabel}; using a deterministic placeholder feed id.` | ||
| ) | ||
| } | ||
|
|
||
| const resolveLocalnetPythPriceFeedIdHex = async ({ | ||
| pythPriceFeedLabel | ||
| }: { | ||
| pythPriceFeedLabel?: string | ||
| }): Promise<string> => { | ||
| const desiredLabel = pythPriceFeedLabel ?? DEFAULT_PYTH_PRICE_FEED_LABEL | ||
| const artifactFeedId = await resolveLocalnetFeedIdFromArtifacts({ | ||
| desiredLabel | ||
| }) | ||
|
|
||
| if (artifactFeedId) { | ||
| return artifactFeedId | ||
| } | ||
|
|
||
| logLocalnetPlaceholderFeedIdFallback(desiredLabel) | ||
|
|
||
| return DEFAULT_LOCALNET_PYTH_PRICE_FEED_ID | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect readArtifact implementation and any missing-file handling:"
rg -n -C4 'export (const|async function|function) readArtifact|ENOENT|readArtifact\(' packages
echo
echo "Inspect the localnet resolver and its tests:"
rg -n -C4 'resolveLocalnetFeedIdFromArtifacts|DEFAULT_LOCALNET_PYTH_PRICE_FEED_ID|mockArtifactPath' packages/dapp/src/utils/amm.ts packages/dapp/src/utils/test/amm.test.tsRepository: OpenZeppelin/openzeppelin-sui-amm
Length of output: 11652
Clarify malformed JSON handling in artifact resolution.
The missing file scenario already works as intended: readArtifact(mockArtifactPath, {}) is passed a default value, so ENOENT errors are caught, the file is created, and an empty object {} is returned. The fallback at line 120 will then execute because findPriceFeedIdFromMockArtifact({}, desiredLabel) returns undefined.
However, if mockArtifactPath exists but contains malformed JSON, readArtifact will throw a parse error (not caught), bypassing the placeholder fallback entirely. Wrap resolveLocalnetFeedIdFromArtifacts in a try–catch to return undefined on any error, ensuring the fallback always runs when the artifact cannot be read.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dapp/src/utils/amm.ts` around lines 90 - 123, Wrap the body of
resolveLocalnetFeedIdFromArtifacts in a try/catch so that any error from
readArtifact (including malformed JSON parse errors) is caught and the function
returns undefined, ensuring resolveLocalnetPythPriceFeedIdHex falls back to
logLocalnetPlaceholderFeedIdFallback and returns
DEFAULT_LOCALNET_PYTH_PRICE_FEED_ID; keep the existing call to
findPriceFeedIdFromMockArtifact(mockArtifact, desiredLabel) inside the try and
optionally log the caught error for debugging.
| export const buildCreateAmmConfigTransaction = ({ | ||
| packageId, | ||
| adminCapId, | ||
| baseSpreadBps, | ||
| volatilityMultiplierBps, | ||
| useLaser, | ||
| pythPriceFeedIdBytes | ||
| }: { | ||
| packageId: string | ||
| adminCapId: string | ||
| baseSpreadBps: bigint | number | ||
| volatilityMultiplierBps: bigint | number | ||
| useLaser: boolean | ||
| pythPriceFeedIdBytes: number[] | ||
| }) => { | ||
| const transaction = newTransaction() | ||
|
|
||
| transaction.moveCall({ | ||
| target: `${packageId}::manager::create_amm_config_and_share`, | ||
| arguments: [ | ||
| transaction.object(adminCapId), | ||
| transaction.pure.u64(baseSpreadBps), | ||
| transaction.pure.u64(volatilityMultiplierBps), | ||
| transaction.pure.bool(useLaser), | ||
| transaction.pure.vector("u8", pythPriceFeedIdBytes) | ||
| ] | ||
| }) | ||
|
|
||
| return transaction | ||
| } |
There was a problem hiding this comment.
Validate pythPriceFeedIdBytes inside the exported builders.
These helpers are public and already get called directly with raw arrays. A 31/33-byte input currently builds a transaction that only fails later at execution time. Re-assert the 32-byte invariant here so misuse fails locally and deterministically.
💡 One way to harden both builders
const PYTH_PRICE_FEED_ID_BYTES = 32
+
+const requirePythPriceFeedIdBytes = (value: number[]) =>
+ assertBytesLength(value, PYTH_PRICE_FEED_ID_BYTES)
export const buildCreateAmmConfigTransaction = ({
packageId,
adminCapId,
baseSpreadBps,
@@
pythPriceFeedIdBytes
}) => {
const transaction = newTransaction()
+ const feedIdBytes = requirePythPriceFeedIdBytes(pythPriceFeedIdBytes)
transaction.moveCall({
target: `${packageId}::manager::create_amm_config_and_share`,
arguments: [
transaction.object(adminCapId),
transaction.pure.u64(baseSpreadBps),
transaction.pure.u64(volatilityMultiplierBps),
transaction.pure.bool(useLaser),
- transaction.pure.vector("u8", pythPriceFeedIdBytes)
+ transaction.pure.vector("u8", feedIdBytes)
]
})Apply the same guard in buildUpdateAmmConfigTransaction.
Also applies to: 59-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/domain/core/src/ptb/amm.ts` around lines 28 - 57, Add a guard in the
exported builder functions to validate that pythPriceFeedIdBytes is exactly 32
bytes long and throw a clear synchronous error if not; specifically, in
buildCreateAmmConfigTransaction (and likewise in
buildUpdateAmmConfigTransaction) check Array.isArray(pythPriceFeedIdBytes) and
pythPriceFeedIdBytes.length === 32 at the start of the function and throw a
TypeError (with context like "pythPriceFeedIdBytes must be a 32-byte array") if
the check fails so misuse fails locally and deterministically before
constructing the transaction.
| const latestAmmPublishArtifact = | ||
| await getLatestDeploymentFromArtifact(AMM_PACKAGE_NAME)(networkName) | ||
|
|
||
| return normalizeIdOrThrow( | ||
| ammPackageId ?? latestAmmPublishArtifact?.packageId, | ||
| "An AMM package id is required; publish the package or provide --amm-package-id." | ||
| ) |
There was a problem hiding this comment.
Honor explicit IDs before reading artifacts.
All three resolvers hit the artifact store before checking the override. In a clean environment, --amm-package-id, --amm-config-id, or --admin-cap-id can still fail even though the caller supplied a valid ID.
💡 Short-circuit the override path first
export const resolveAmmPackageId = async ({
networkName,
ammPackageId
}: {
networkName: string
ammPackageId?: string
}): Promise<string> => {
+ if (ammPackageId) {
+ return normalizeIdOrThrow(
+ ammPackageId,
+ "An AMM package id is required; publish the package or provide --amm-package-id."
+ )
+ }
+
const latestAmmPublishArtifact =
await getLatestDeploymentFromArtifact(AMM_PACKAGE_NAME)(networkName)
return normalizeIdOrThrow(
- ammPackageId ?? latestAmmPublishArtifact?.packageId,
+ latestAmmPublishArtifact?.packageId,
"An AMM package id is required; publish the package or provide --amm-package-id."
)
}Do the same in resolveAmmConfigId and resolveAmmAdminCapId.
Also applies to: 39-46, 56-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/domain/node/src/amm.ts` around lines 23 - 29, The current resolvers
fetch artifacts before honoring explicit overrides causing valid user-supplied
IDs (e.g. --amm-package-id) to still trigger artifact lookups; update the logic
in the AMM resolvers so they short-circuit: first check if ammPackageId (or
ammConfigId / adminCapId) is provided and if so immediately return
normalizeIdOrThrow(...) without calling
getLatestDeploymentFromArtifact(AMM_PACKAGE_NAME)(networkName); only call
getLatestDeploymentFromArtifact when the override is absent and then pass the
discovered packageId/configId/capId into normalizeIdOrThrow. Ensure the same
change is applied to resolveAmmConfigId and resolveAmmAdminCapId.
qalisander
left a comment
There was a problem hiding this comment.
LGTM!
But we should double-check that coderabitai issues are not applicable
Part of #25
Summary
Adds the AMM config management module in TypeScript: domain model parsing, PTB builders, and create/view script flows.
Scope
amm-createand useramm-viewscriptsSummary by CodeRabbit
Release Notes
New Features
Tests