Closed
Conversation
CoveMB
requested review from
bidzyyys,
ericnordelo and
immrsd
as code owners
CoveMB
changed the base branch from
split/16-executor-contracts
to
split/24-shared-mock-prerequisites
Contributor
|
Blocked by #70 |
WalkthroughThis pull request introduces AMM (Automated Market Maker) configuration management for prop-amm contracts, adds comprehensive TypeScript utilities for AMM creation and viewing, refactors directory structure from "move" to "contracts", updates Move environment naming from "localnet" to "test-publish", and adds integration tests spanning Move contracts, scripts, and utilities. Changes
Sequence DiagramssequenceDiagram
participant User as Owner Script
participant Builder as Transaction Builder
participant Network as Sui Network
participant Manager as AMM Manager Module
User->>User: Resolve parameters<br/>(spread, volatility, laser, pyth feed)
User->>Builder: buildCreateAmmConfigTransaction
Builder->>Builder: Create move call:<br/>manager::create_amm_config
Builder->>Builder: Create move call:<br/>manager::share_amm_config
Builder->>Network: Execute transaction
Network->>Manager: create_amm_config
Manager->>Manager: Validate inputs
Manager->>Manager: Create AMMConfig object
Manager->>Network: Emit AMMConfigCreatedEvent
Manager->>Network: Return config object
Network->>Manager: share_amm_config
Manager->>Network: Make config shared
Network->>User: Return digest & config ID
User->>Network: Fetch AMMConfig artifact
User->>User: Return JSON or formatted output
sequenceDiagram
participant User as View Script
participant Cache as Artifacts/Cache
participant Network as Sui Network
participant Core as Domain Core
User->>User: Resolve config ID<br/>(from artifacts or input)
User->>Core: collectAmmConfigSnapshot
Core->>Core: getAmmConfigOverview
Core->>Network: Fetch AMMConfig object
Network->>Core: Return object with fields
Core->>Core: Extract & validate fields
Core->>Cache: Get shared object version
Cache->>Network: Query immutable shared object
Network->>Cache: Return initialSharedVersion
Core->>User: Return AmmConfigSnapshot
User->>User: Output JSON or formatted view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tooling/node/test-unit/unit/publish.test.ts (1)
500-529:⚠️ Potential issue | 🟠 MajorWrap this success-path case in
withCwd(dir).This test now performs a real publish, but unlike the other successful publish cases it never switches into the temp workspace first. Since
publishPackageWithLoginpackages/tooling/node/src/publish.ts:131-175delegates topublishPackage, this can write deployment artifacts into the ambient working directory and make the case order-dependent.Proposed fix
- const artifacts = await publishPackageWithLog( - { - packagePath, - keypair: Ed25519Keypair.generate() - }, - { suiClient: client, suiConfig: config } - ) - - expect(artifacts[0]?.packageId).toBe("0x1") - - expect( - logMocks.logWarning.mock.calls.some(([message]) => - String(message).includes( - "Multiple Sui framework revisions detected in Move.lock" - ) - ) - ).toBe(true) + await withCwd(dir, async () => { + const artifacts = await publishPackageWithLog( + { + packagePath, + keypair: Ed25519Keypair.generate() + }, + { suiClient: client, suiConfig: config } + ) + + expect(artifacts[0]?.packageId).toBe("0x1") + expect( + logMocks.logWarning.mock.calls.some(([message]) => + String(message).includes( + "Multiple Sui framework revisions detected in Move.lock" + ) + ) + ).toBe(true) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tooling/node/test-unit/unit/publish.test.ts` around lines 500 - 529, In the test case inside publish.test.ts, wrap the success-path block that calls publishPackageWithLog (and the subsequent expects) inside withCwd(dir) so the temp workspace is used as the current working directory; locate the async callback passed to withTempDir that creates packagePath and config and replace the direct publishPackageWithLog invocation with a call inside withCwd(dir, async () => { ... }) (still using publishPackageWithLog which delegates to publishPackage) to avoid writing deployment artifacts to the ambient CWD.packages/tooling/node/src/testing/localnet.ts (1)
1258-1286:⚠️ Potential issue | 🟠 MajorUpdate stale environment entries instead of returning early.
If a copied
Move.tomlalready hastest-publish = "...", this helper keeps the old chain id and skips rewriting it. In isolated localnet runs that leaves packages pinned to stale environment metadata and can break later build/publish steps.🛠️ Suggested fix
const ensureMoveTomlEnvironmentEntry = async ({ moveTomlPath, environmentName, chainId }: { moveTomlPath: string environmentName: string chainId: string }) => { const contents = await readFile(moveTomlPath, "utf8") const entryRegex = buildEnvironmentEntryRegex(environmentName) - if (entryRegex.test(contents)) return - const entryLine = buildEnvironmentEntryLine(environmentName, chainId) + if (entryRegex.test(contents)) { + const updated = contents.replace(entryRegex, entryLine) + if (updated !== contents) { + await writeFile(moveTomlPath, updated, "utf8") + } + return + } - if (/^\s*\[environments\]\s*$/m.test(contents)) { + if (/^\s*\[environments\]\s*(#.*)?$/m.test(contents)) { const updated = contents.replace( - /^\s*\[environments\]\s*$/m, + /^\s*\[environments\]\s*(#.*)?$/m, `[environments]\n${entryLine}` ) if (updated !== contents) { await writeFile(moveTomlPath, updated, "utf8") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tooling/node/src/testing/localnet.ts` around lines 1258 - 1286, The helper ensureMoveTomlEnvironmentEntry currently returns early when entryRegex.test(contents) is true, which leaves stale test-publish values; change ensureMoveTomlEnvironmentEntry so that when entryRegex matches it replaces the existing environment entry with the fresh string from buildEnvironmentEntryLine(environmentName, chainId) (use the same entryRegex from buildEnvironmentEntryRegex to perform a replace), write the file if changed, and only fall back to the existing insertion logic (adding [environments] block or appending at EOF) when no match is found; keep function name ensureMoveTomlEnvironmentEntry and use buildEnvironmentEntryLine/buildEnvironmentEntryRegex/entryRegex to locate and update the entry and call writeFile only when contents change.
🧹 Nitpick comments (6)
scripts/strip-move-localnet.js (1)
11-11: Regex correctly updated for test-publish environment.The regex pattern now correctly targets
[env.test-publish]blocks.Consider whether the filename
strip-move-localnet.jsshould be renamed to match the new terminology (e.g.,strip-move-test-publish.js), as the current name may be confusing given it now strips[env.test-publish]blocks rather than[env.localnet].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/strip-move-localnet.js` at line 11, The script strip-move-localnet.js was updated to target [env.test-publish] (see the LOCALNET_SECTION regex) but the filename is misleading; rename the file to strip-move-test-publish.js and update all references to the old name (e.g., package.json scripts, CI config, imports or execution calls) so callers point to the new filename; keep the constant LOCALNET_SECTION and its regex unchanged but also search the repo for any occurrences of "strip-move-localnet" to replace with "strip-move-test-publish" to avoid confusion.packages/domain/core/src/index.ts (1)
1-2: Re-export domain-core models from the index entrypoint.The models directory contains
amm.tsandpyth.tswith exported types and utilities that are actively used across the codebase. Currently they're imported via long paths like@sui-amm/domain-core/models/amm. Consider updating the index to re-export these modules for a cleaner public API:export * from "./models/amm" export * from "./models/pyth"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/domain/core/src/index.ts` around lines 1 - 2, Update the package entrypoint to re-export the domain-core model modules so callers can import them from the package root: add exports that re-export everything from the amm and pyth model modules (the modules named amm.ts and pyth.ts under models) so symbols defined there are available via the index entrypoint instead of long internal paths.packages/domain/node/src/amm.ts (1)
22-38: Short-circuit explicit IDs before loading artifacts.All three resolvers hit artifact storage even when the caller already supplied an id. With the current artifact-loader behavior, that still means unnecessary I/O and can create empty artifact files on
ENOENT.Suggested pattern
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 deploymentArtifacts = await loadDeploymentArtifacts(networkName)Apply the same early-return shape to
resolveAmmConfigIdandresolveAmmAdminCapId.Also applies to: 41-56, 58-73
🤖 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 22 - 38, If an explicit ammPackageId is provided, short-circuit before calling loadDeploymentArtifacts to avoid unnecessary I/O: in resolveAmmPackageId check ammPackageId first and return normalizeIdOrThrow(ammPackageId, ...) immediately, otherwise continue to loadDeploymentArtifacts and findLatestArtifactThat(...) as now; apply the same early-return pattern to resolveAmmConfigId and resolveAmmAdminCapId so callers supplying explicit IDs never trigger artifact loading.packages/dapp/src/utils/amm.ts (1)
6-20: Avoid duplicating the default mock-feed label.
packages/domain/core/src/models/pyth.tsalready defines the default viaDEFAULT_MOCK_PRICE_FEED. Hardcoding"MOCK_SUI_FEED"again here makes the localnet fallback path drift-prone.Suggested change
-import { findMockPriceFeedConfig } from "@sui-amm/domain-core/models/pyth" +import { + DEFAULT_MOCK_PRICE_FEED, + findMockPriceFeedConfig +} from "@sui-amm/domain-core/models/pyth" @@ -export const DEFAULT_PYTH_PRICE_FEED_LABEL = "MOCK_SUI_FEED" +export const DEFAULT_PYTH_PRICE_FEED_LABEL = DEFAULT_MOCK_PRICE_FEED.label🤖 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 6 - 20, The constant DEFAULT_PYTH_PRICE_FEED_LABEL is hardcoded to "MOCK_SUI_FEED" which duplicates the canonical default defined in packages/domain/core/src/models/pyth.ts; instead import and re-export (or assign) the canonical DEFAULT_MOCK_PRICE_FEED from "@sui-amm/domain-core/models/pyth" and replace the literal value with that symbol so findMockPriceFeedConfig and any fallback logic use the single source of truth (update the import list to include DEFAULT_MOCK_PRICE_FEED and set DEFAULT_PYTH_PRICE_FEED_LABEL = DEFAULT_MOCK_PRICE_FEED, or remove the local constant and reference DEFAULT_MOCK_PRICE_FEED directly).packages/tooling/README.md (1)
17-23: Consider adding language specifier to code block.The directory structure code block lacks a language specifier. While this is valid markdown, adding
textor leaving it as a simple indented list would satisfy linting tools.-``` +```text packages/tooling/ core/ (Node + browser) node/ (Node only)<details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@packages/tooling/README.mdaround lines 17 - 23, The fenced code block under
"Package layout" currently has no language specifier; update the triple-backtick
block that wraps the directory listing (the block containing
"packages/tooling/", "core/ (Node + browser)", "node/ (Node only)") to include a
language tag such as text (i.e., ```text) or replace it with an
indented/bulleted list so markdown linters accept it.</details> </blockquote></details> <details> <summary>packages/dapp/src/scripts/owner/amm-create.ts (1)</summary><blockquote> `71-74`: **Use `AMM_CONFIG_TYPE_SUFFIX` for this lookup.** This literal already exists in the domain model. Reusing the shared constant keeps the artifact lookup aligned if the Move type name ever changes. <details> <summary>♻️ Suggested change</summary> ```diff 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 ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/dapp/src/scripts/owner/amm-create.ts` around lines 71 - 74, Replace the hard-coded suffix string "::manager::AMMConfig" used in the call to findCreatedArtifactBySuffix with the shared constant AMM_CONFIG_TYPE_SUFFIX to keep the artifact lookup consistent with the domain model; update the reference in the createdAmmConfig assignment and add the necessary import for AMM_CONFIG_TYPE_SUFFIX (from where the domain model exports it) so the code compiles and the lookup stays aligned if the Move type name changes. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@CONTRIBUTING.md:
- Around line 36-37: The second sentence is inconsistent: it refers to "localnet
block" while the earlier sentence names the block as "[env.test-publish]";
update the wording so both lines reference the same block (e.g., change
"localnet block" to "[env.test-publish] block" or "test-publish block") to make
the documentation consistent, keeping the rest of the sentence ("Your working
tree may still contain ...; only the commit content is cleaned.") unchanged.In
@packages/dapp/contracts/prop-amm/Move.toml:
- Around line 6-7: Replace the floating ref for the deepbook dependency in
Move.toml (the [dependencies] entry for deepbook) by pinning rev to the specific
immutable commit SHA or release tag currently used (the commit referenced in
Move.lock, e.g., 4c2f8e22d54e40da9b44e297656ee1d4528612b2) so the deepbook = {
git = "...", subdir = "packages/deepbook", rev = "" }
entry uses an explicit immutable ref rather than "main".In
@packages/dapp/contracts/prop-amm/sources/manager.move:
- Around line 4-6: The module is missing imports for UID, TxContext and the
object/transfer functions it references; add the appropriate Sui module uses so
name resolution succeeds—specifically import TxContext (e.g., from
sui::tx_context::TxContext), UID and object::new (from sui::object::{UID, new})
and the transfer functions transfer and share_object (from
sui::transfer::{transfer, share_object}) so references to UID, TxContext,
object::new, transfer::transfer and transfer::share_object resolve.In
@packages/dapp/src/scripts/move/test.ts:
- Around line 52-58: The log message hardcodes "test-publish" instead of using
the dynamic environment name; update the logging call in the block that uses
logKeyValueBlue("Move.toml") to interpolate the environmentName variable (e.g.,
updated ${updatedFiles.length} ${environmentName} environment entries) so the
message reflects the actual environment being updated—locate the code that
constructs the string near the logKeyValueBlue("Move.toml") invocation and
replace the literal "test-publish" with the environmentName identifier (preserve
pluralization/formatting as needed).In
@packages/domain/core/src/models/amm.ts:
- Around line 80-89: getAmmConfigOverview currently fetches the Sui object with
showType but never validates the object's type before passing it to
buildAmmConfigOverviewFromObject; add a deterministic type check after
getSuiObject (using the returned object's type/type string) to ensure it matches
the expected AMM config type and throw a clear "not an AMM config" or
InvalidArgument error if it doesn't, so callers get a precise failure instead of
later missing-field errors; update getAmmConfigOverview to validate the object's
type and only call buildAmmConfigOverviewFromObject when the type matches.In
@packages/domain/core/src/ptb/amm.ts:
- Around line 37-50: The AMM builder is calling manager::create_amm_config and
then manager::share_amm_config which bypasses the event-emitting wrappers;
replace those two calls with the single event-emitting call
manager::create_amm_config_and_share (and where configs are updated replace
manager::update_amm_config + share with manager::update_amm_config_and_emit) so
the AMMConfigCreatedEvent/AMMConfigUpdatedEvent are emitted; update the moveCall
target names (create_amm_config -> create_amm_config_and_share,
update_amm_config -> update_amm_config_and_emit) and pass the same argument
vector structure (the u64/u64/bool/vector bytes payload) in the same position;
apply the same replacement for the other occurrence mentioned (the block at
~77-88).In
@packages/tooling/node/src/artifacts.ts:
- Around line 268-269: The comparison normalizes artifact.packageName into
normalizedPackageName but only lowercases artifactName, so values with
trailing/leading whitespace (e.g., "oracle-market ") will fail; update the
lookup to normalize both sides the same way by trimming and lowercasing
artifactName before comparing (e.g., compute a normalizedLookup from
artifactName via artifactName.trim().toLowerCase() and compare it to
normalizedPackageName), keeping the existing null-safe access to
artifact.packageName and reusing the variables normalizedPackageName and
artifactName to locate where to change.In
@packages/tooling/README.md:
- Line 13: Replace the typo "integrating with by environment flows" in the
README line with a clear phrase such as "integrating with environment flows" or
"integrating with CI/environment flows" (or "integration flows") so the
description reads correctly; locate the exact string "integrating with by
environment flows" and update it accordingly.- Around line 36-39: The README import examples use the placeholder
_root_package__; update those import lines so they reference the real
workspace package names that export newTransaction and publishPackageWithLog
(replace_root_package__with the actual package scope/name used in your
monorepo), ensuring the import paths match the package entrypoints and exported
symbols (newTransaction from the tooling-core package and publishPackageWithLog
from the tooling-node package).In
@scripts/strip-move-localnet.js:
- Around line 159-161: The ESLint
no-undefonconsole(seen in the
main().catch(...) callback) means the linter doesn't know Node globals; fix by
either adding a top-of-file global directive like/* global console */or
enabling Node globals in your ESLint config (setenv: { node: true }), then
re-run linting to verify the warning for theconsole.errorin the main().catch
block is resolved.
Outside diff comments:
In@packages/tooling/node/src/testing/localnet.ts:
- Around line 1258-1286: The helper ensureMoveTomlEnvironmentEntry currently
returns early when entryRegex.test(contents) is true, which leaves stale
test-publish values; change ensureMoveTomlEnvironmentEntry so that when
entryRegex matches it replaces the existing environment entry with the fresh
string from buildEnvironmentEntryLine(environmentName, chainId) (use the same
entryRegex from buildEnvironmentEntryRegex to perform a replace), write the file
if changed, and only fall back to the existing insertion logic (adding
[environments] block or appending at EOF) when no match is found; keep function
name ensureMoveTomlEnvironmentEntry and use
buildEnvironmentEntryLine/buildEnvironmentEntryRegex/entryRegex to locate and
update the entry and call writeFile only when contents change.In
@packages/tooling/node/test-unit/unit/publish.test.ts:
- Around line 500-529: In the test case inside publish.test.ts, wrap the
success-path block that calls publishPackageWithLog (and the subsequent expects)
inside withCwd(dir) so the temp workspace is used as the current working
directory; locate the async callback passed to withTempDir that creates
packagePath and config and replace the direct publishPackageWithLog invocation
with a call inside withCwd(dir, async () => { ... }) (still using
publishPackageWithLog which delegates to publishPackage) to avoid writing
deployment artifacts to the ambient CWD.
Nitpick comments:
In@packages/dapp/src/scripts/owner/amm-create.ts:
- Around line 71-74: Replace the hard-coded suffix string "::manager::AMMConfig"
used in the call to findCreatedArtifactBySuffix with the shared constant
AMM_CONFIG_TYPE_SUFFIX to keep the artifact lookup consistent with the domain
model; update the reference in the createdAmmConfig assignment and add the
necessary import for AMM_CONFIG_TYPE_SUFFIX (from where the domain model exports
it) so the code compiles and the lookup stays aligned if the Move type name
changes.In
@packages/dapp/src/utils/amm.ts:
- Around line 6-20: The constant DEFAULT_PYTH_PRICE_FEED_LABEL is hardcoded to
"MOCK_SUI_FEED" which duplicates the canonical default defined in
packages/domain/core/src/models/pyth.ts; instead import and re-export (or
assign) the canonical DEFAULT_MOCK_PRICE_FEED from
"@sui-amm/domain-core/models/pyth" and replace the literal value with that
symbol so findMockPriceFeedConfig and any fallback logic use the single source
of truth (update the import list to include DEFAULT_MOCK_PRICE_FEED and set
DEFAULT_PYTH_PRICE_FEED_LABEL = DEFAULT_MOCK_PRICE_FEED, or remove the local
constant and reference DEFAULT_MOCK_PRICE_FEED directly).In
@packages/domain/core/src/index.ts:
- Around line 1-2: Update the package entrypoint to re-export the domain-core
model modules so callers can import them from the package root: add exports that
re-export everything from the amm and pyth model modules (the modules named
amm.ts and pyth.ts under models) so symbols defined there are available via the
index entrypoint instead of long internal paths.In
@packages/domain/node/src/amm.ts:
- Around line 22-38: If an explicit ammPackageId is provided, short-circuit
before calling loadDeploymentArtifacts to avoid unnecessary I/O: in
resolveAmmPackageId check ammPackageId first and return
normalizeIdOrThrow(ammPackageId, ...) immediately, otherwise continue to
loadDeploymentArtifacts and findLatestArtifactThat(...) as now; apply the same
early-return pattern to resolveAmmConfigId and resolveAmmAdminCapId so callers
supplying explicit IDs never trigger artifact loading.In
@packages/tooling/README.md:
- Around line 17-23: The fenced code block under "Package layout" currently has
no language specifier; update the triple-backtick block that wraps the directory
listing (the block containing "packages/tooling/", "core/ (Node + browser)",
"node/ (Node only)") to include a language tag such as text (i.e., ```text) or
replace it with an indented/bulleted list so markdown linters accept it.In
@scripts/strip-move-localnet.js:
- Line 11: The script strip-move-localnet.js was updated to target
[env.test-publish] (see the LOCALNET_SECTION regex) but the filename is
misleading; rename the file to strip-move-test-publish.js and update all
references to the old name (e.g., package.json scripts, CI config, imports or
execution calls) so callers point to the new filename; keep the constant
LOCALNET_SECTION and its regex unchanged but also search the repo for any
occurrences of "strip-move-localnet" to replace with "strip-move-test-publish"
to avoid confusion.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `f8a77cd5-6197-433e-aa7d-dc193a0b24ed` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 99a935e87e5e7dfd24dfc0f1be53e28ad3abb665 and cb2b3acec82e4a2fc52a41b2498cb7453e922264. </details> <details> <summary>⛔ Files ignored due to path filters (3)</summary> * `packages/dapp/contracts/prop-amm/Move.lock` is excluded by `!**/*.lock` * `packages/tooling/tests-integration/fixtures/localnet-move/simple-contract/Move.lock` is excluded by `!**/*.lock` * `packages/tooling/tests-integration/fixtures/move/Move.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (55)</summary> * `.githooks/pre-commit` * `.gitignore` * `CONTRIBUTING.md` * `package.json` * `packages/dapp/contracts/.prettierrc` * `packages/dapp/contracts/prop-amm/Move.toml` * `packages/dapp/contracts/prop-amm/README.md` * `packages/dapp/contracts/prop-amm/sources/executor.move` * `packages/dapp/contracts/prop-amm/sources/manager.move` * `packages/dapp/contracts/prop-amm/tests/manager_tests.move` * `packages/dapp/move/amm/executor.move` * `packages/dapp/move/amm/manager.move` * `packages/dapp/package.json` * `packages/dapp/src/scripts/move/publish.ts` * `packages/dapp/src/scripts/move/test.ts` * `packages/dapp/src/scripts/owner/amm-create.ts` * `packages/dapp/src/scripts/owner/test-integration/amm-create.test.ts` * `packages/dapp/src/scripts/user/amm-view.ts` * `packages/dapp/src/scripts/user/test-integration/amm-view.test.ts` * `packages/dapp/src/utils/amm.ts` * `packages/dapp/src/utils/mocks.ts` * `packages/dapp/src/utils/test/amm.test.ts` * `packages/dapp/sui.config.ts` * `packages/domain/core/src/index.ts` * `packages/domain/core/src/models/amm.ts` * `packages/domain/core/src/models/pyth.ts` * `packages/domain/core/src/ptb/amm.ts` * `packages/domain/node/src/amm.ts` * `packages/domain/node/src/index.ts` * `packages/tooling/README.md` * `packages/tooling/core/src/constants.ts` * `packages/tooling/node/package.json` * `packages/tooling/node/src/artifacts.ts` * `packages/tooling/node/src/config.ts` * `packages/tooling/node/src/move-toml.ts` * `packages/tooling/node/src/move.ts` * `packages/tooling/node/src/process.ts` * `packages/tooling/node/src/publish.ts` * `packages/tooling/node/src/testing/localnet.ts` * `packages/tooling/node/src/testing/paths.ts` * `packages/tooling/node/test-unit/unit/artifacts.test.ts` * `packages/tooling/node/test-unit/unit/config.test.ts` * `packages/tooling/node/test-unit/unit/move-build.test.ts` * `packages/tooling/node/test-unit/unit/move.test.ts` * `packages/tooling/node/test-unit/unit/publish.test.ts` * `packages/tooling/node/test-unit/unit/testing-scripts.test.ts` * `packages/tooling/tests-integration/fixtures/localnet-move/simple-contract/Move.toml` * `packages/tooling/tests-integration/fixtures/localnet-move/simple-contract/build/simple_contract/BuildInfo.yaml` * `packages/tooling/tests-integration/fixtures/localnet-move/simple-contract/sources/counter.move` * `packages/tooling/tests-integration/fixtures/localnet-move/simple-contract/sources/pool.move` * `packages/tooling/tests-integration/fixtures/move/Move.lock.pinned` * `packages/tooling/tests-integration/fixtures/move/Move.toml` * `packages/tooling/tests-integration/fixtures/move/sources/fixture.move` * `packages/tooling/tests-integration/integration/transactions-events.test.ts` * `scripts/strip-move-localnet.js` </details> <details> <summary>💤 Files with no reviewable changes (3)</summary> * package.json * packages/tooling/tests-integration/fixtures/localnet-move/simple-contract/sources/pool.move * packages/tooling/tests-integration/fixtures/localnet-move/simple-contract/build/simple_contract/BuildInfo.yaml </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+6
to
+7
| [dependencies] | ||
| deepbook = { git = "https://github.com/MystenLabs/deepbookv3.git", subdir = "packages/deepbook", rev = "main" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether this package still relies on a floating git ref and whether a lock file is the only immutable pin.
rg -n --glob 'Move.toml' 'deepbook.*rev\s*=\s*"main"'
fd '^Move\.lock(\.pinned)?$' packages/dapp/contracts -x sed -n '1,120p' {}Repository: OpenZeppelin/openzeppelin-sui-amm
Length of output: 3712
Pin deepbook to an immutable ref in Move.toml.
Line 7 uses rev = "main", a floating reference. While Move.lock currently pins this to commit 4c2f8e22d54e40da9b44e297656ee1d4528612b2, the manifest should declare an explicit immutable ref (commit SHA or release tag) for clarity and to avoid relying on lock file regeneration. Pin it directly in Move.toml.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dapp/contracts/prop-amm/Move.toml` around lines 6 - 7, Replace the
floating ref for the deepbook dependency in Move.toml (the [dependencies] entry
for deepbook) by pinning rev to the specific immutable commit SHA or release tag
currently used (the commit referenced in Move.lock, e.g.,
4c2f8e22d54e40da9b44e297656ee1d4528612b2) so the deepbook = { git = "...",
subdir = "packages/deepbook", rev = "<immutable-sha-or-tag>" } entry uses an
explicit immutable ref rather than "main".
Contributor
There was a problem hiding this comment.
It's better to pin specific commit/version instead of main @CoveMB @qalisander
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+52
to
+58
| "Unable to resolve localnet chain id; Move.toml test-publish environments were not updated." | ||
| ) | ||
| } | ||
|
|
||
| if (updatedFiles.length) { | ||
| logKeyValueBlue("Move.toml")( | ||
| `updated ${updatedFiles.length} localnet environment entries` | ||
| `updated ${updatedFiles.length} test-publish environment entries` |
There was a problem hiding this comment.
Keep the log output tied to the actual environment name.
This helper passes a dynamic environmentName, but the messages now always say test-publish. That will misreport what was actually updated and make sync failures harder to debug on other networks.
Suggested fix
const syncMoveEnvironmentForTests = async (
tooling: Pick<Tooling, "syncLocalnetMoveEnvironmentChainId" | "suiConfig">
) => {
+ const environmentName = tooling.suiConfig.network.networkName
const { chainId, updatedFiles, didAttempt } =
await tooling.syncLocalnetMoveEnvironmentChainId({
moveRootPath: path.resolve(tooling.suiConfig.paths.move),
- environmentName: tooling.suiConfig.network.networkName
+ environmentName
})
if (didAttempt && !chainId) {
logWarning(
- "Unable to resolve localnet chain id; Move.toml test-publish environments were not updated."
+ `Unable to resolve localnet chain id; Move.toml ${environmentName} environments were not updated.`
)
}
if (updatedFiles.length) {
logKeyValueBlue("Move.toml")(
- `updated ${updatedFiles.length} test-publish environment entries`
+ `updated ${updatedFiles.length} ${environmentName} environment entries`
)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dapp/src/scripts/move/test.ts` around lines 52 - 58, The log message
hardcodes "test-publish" instead of using the dynamic environment name; update
the logging call in the block that uses logKeyValueBlue("Move.toml") to
interpolate the environmentName variable (e.g., `updated ${updatedFiles.length}
${environmentName} environment entries`) so the message reflects the actual
environment being updated—locate the code that constructs the string near the
logKeyValueBlue("Move.toml") invocation and replace the literal "test-publish"
with the environmentName identifier (preserve pluralization/formatting as
needed).
Comment on lines
+80
to
+89
| export const getAmmConfigOverview = async ( | ||
| configId: string, | ||
| suiClient: SuiClient | ||
| ): Promise<AmmConfigOverview> => { | ||
| const { object } = await getSuiObject( | ||
| { objectId: configId, options: { showContent: true, showType: true } }, | ||
| { suiClient } | ||
| ) | ||
|
|
||
| return buildAmmConfigOverviewFromObject({ configId, object }) |
There was a problem hiding this comment.
Reject non-AMM objects before extracting fields.
getAmmConfigOverview already requests showType: true, but never checks it. Passing the wrong object id will currently fail later with generic field-missing errors instead of a deterministic "not an AMM config" error.
Suggested change
export const getAmmConfigOverview = async (
configId: string,
suiClient: SuiClient
): Promise<AmmConfigOverview> => {
const { object } = await getSuiObject(
{ objectId: configId, options: { showContent: true, showType: true } },
{ suiClient }
)
+ if (!object.type?.endsWith(AMM_CONFIG_TYPE_SUFFIX)) {
+ throw new Error(`Object ${configId} is not an AMM config.`)
+ }
+
return buildAmmConfigOverviewFromObject({ configId, object })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/domain/core/src/models/amm.ts` around lines 80 - 89,
getAmmConfigOverview currently fetches the Sui object with showType but never
validates the object's type before passing it to
buildAmmConfigOverviewFromObject; add a deterministic type check after
getSuiObject (using the returned object's type/type string) to ensure it matches
the expected AMM config type and throw a clear "not an AMM config" or
InvalidArgument error if it doesn't, so callers get a precise failure instead of
later missing-field errors; update getAmmConfigOverview to validate the object's
type and only call buildAmmConfigOverviewFromObject when the type matches.
Comment on lines
+37
to
+50
| const config = transaction.moveCall({ | ||
| target: `${packageId}::manager::create_amm_config`, | ||
| arguments: [ | ||
| transaction.pure.u64(baseSpreadBps), | ||
| transaction.pure.u64(volatilityMultiplierBps), | ||
| transaction.pure.bool(useLaser), | ||
| transaction.pure.vector("u8", pythPriceFeedIdBytes) | ||
| ] | ||
| }) | ||
|
|
||
| transaction.moveCall({ | ||
| target: `${packageId}::manager::share_amm_config`, | ||
| arguments: [config] | ||
| }) |
There was a problem hiding this comment.
Use the event-emitting manager calls here.
These builders currently bypass create_amm_config_and_share / update_amm_config_and_emit, so AMM configs created or updated through the official PTB path never emit AMMConfigCreatedEvent / AMMConfigUpdatedEvent.
Suggested change
- const config = transaction.moveCall({
- target: `${packageId}::manager::create_amm_config`,
- arguments: [
- transaction.pure.u64(baseSpreadBps),
- transaction.pure.u64(volatilityMultiplierBps),
- transaction.pure.bool(useLaser),
- transaction.pure.vector("u8", pythPriceFeedIdBytes)
- ]
- })
-
transaction.moveCall({
- target: `${packageId}::manager::share_amm_config`,
+ target: `${packageId}::manager::create_amm_config_and_share`,
arguments: [
- config
+ transaction.pure.u64(baseSpreadBps),
+ transaction.pure.u64(volatilityMultiplierBps),
+ transaction.pure.bool(useLaser),
+ transaction.pure.vector("u8", pythPriceFeedIdBytes)
]
})
@@
transaction.moveCall({
- target: `${packageId}::manager::update_amm_config`,
+ target: `${packageId}::manager::update_amm_config_and_emit`,
arguments: [
configArgument,
transaction.object(adminCapId),Also applies to: 77-88
🤖 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 37 - 50, The AMM builder is
calling manager::create_amm_config and then manager::share_amm_config which
bypasses the event-emitting wrappers; replace those two calls with the single
event-emitting call manager::create_amm_config_and_share (and where configs are
updated replace manager::update_amm_config + share with
manager::update_amm_config_and_emit) so the
AMMConfigCreatedEvent/AMMConfigUpdatedEvent are emitted; update the moveCall
target names (create_amm_config -> create_amm_config_and_share,
update_amm_config -> update_amm_config_and_emit) and pass the same argument
vector structure (the u64/u64/bool/vector bytes payload) in the same position;
apply the same replacement for the other occurrence mentioned (the block at
~77-88).
Comment on lines
268
to
+269
| const normalizedPackageName = artifact.packageName?.trim().toLowerCase() | ||
| if (normalizedPackageName === artifactName) return true | ||
| if (normalizedPackageName === artifactName.toLowerCase()) return true |
There was a problem hiding this comment.
Normalize the lookup key the same way on both sides.
packageName is trimmed before comparison, but artifactName is only lowercased. An input like "oracle-market " will still miss even though the stored artifact name is normalized.
Suggested fix
export const isPublishArtifactNamed =
(artifactName: string) =>
(artifact: PublishArtifact): boolean => {
+ const normalizedArtifactName = artifactName.trim().toLowerCase()
const normalizedPackageName = artifact.packageName?.trim().toLowerCase()
- if (normalizedPackageName === artifactName.toLowerCase()) return true
+ if (normalizedPackageName === normalizedArtifactName) return true
return false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tooling/node/src/artifacts.ts` around lines 268 - 269, The
comparison normalizes artifact.packageName into normalizedPackageName but only
lowercases artifactName, so values with trailing/leading whitespace (e.g.,
"oracle-market ") will fail; update the lookup to normalize both sides the same
way by trimming and lowercasing artifactName before comparing (e.g., compute a
normalizedLookup from artifactName via artifactName.trim().toLowerCase() and
compare it to normalizedPackageName), keeping the existing null-safe access to
artifact.packageName and reusing the variables normalizedPackageName and
artifactName to locate where to change.
| - publishing Move packages with artifact capture across networks | ||
| - managing deployment/object changes with artifacts across networks | ||
| - executing scripts with a consistent CLI environment | ||
| - integrating with by environment flows in CI or integration tests |
There was a problem hiding this comment.
Fix typo in description.
The phrase "integrating with by environment flows" appears to be a typo. Consider:
-- integrating with by environment flows in CI or integration tests
+- integrating with environment flows in CI or integration tests🧰 Tools
🪛 LanguageTool
[grammar] ~13-~13: Use a hyphen to join words.
Context: ...nt CLI environment - integrating with by environment flows in CI or integration t...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tooling/README.md` at line 13, Replace the typo "integrating with by
environment flows" in the README line with a clear phrase such as "integrating
with environment flows" or "integrating with CI/environment flows" (or
"integration flows") so the description reads correctly; locate the exact string
"integrating with by environment flows" and update it accordingly.
Comment on lines
+36
to
+39
| ```ts | ||
| import { newTransaction } from "_root_package__/tooling-core/transactions" | ||
| import { publishPackageWithLog } from "_root_package__/tooling-node/publish" | ||
| ``` |
There was a problem hiding this comment.
Replace placeholder package names.
The import examples use _root_package__ which appears to be a placeholder. Update with the actual workspace package names for clarity.
-import { newTransaction } from "_root_package__/tooling-core/transactions"
-import { publishPackageWithLog } from "_root_package__/tooling-node/publish"
+import { newTransaction } from "@sui-amm/tooling-core/transactions"
+import { publishPackageWithLog } from "@sui-amm/tooling-node/publish"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tooling/README.md` around lines 36 - 39, The README import examples
use the placeholder `_root_package__`; update those import lines so they
reference the real workspace package names that export newTransaction and
publishPackageWithLog (replace `_root_package__` with the actual package
scope/name used in your monorepo), ensuring the import paths match the package
entrypoints and exported symbols (newTransaction from the tooling-core package
and publishPackageWithLog from the tooling-node package).
Comment on lines
159
to
161
| main().catch((error) => { | ||
| console.error("Failed to strip [env.localnet] from Move.lock:", error) | ||
| console.error("Failed to strip [env.test-publish] from Move.lock:", error) | ||
| process.exit(1) |
There was a problem hiding this comment.
Address ESLint no-undef warning for console.
The static analysis flagged console as undefined. While console is globally available in Node.js, the ESLint configuration may be missing the node environment. You can either:
- Add
/* global console */at the top of the file, or - Update ESLint config to include
env: { node: true }
🛠️ Quick fix option
#!/usr/bin/env node
+/* global console */
import { execFileSync } from "node:child_process"🧰 Tools
🪛 ESLint
[error] 160-160: 'console' is not defined.
(no-undef)
[error] 161-161: 'process' is not defined.
(no-undef)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/strip-move-localnet.js` around lines 159 - 161, The ESLint `no-undef`
on `console` (seen in the main().catch(...) callback) means the linter doesn't
know Node globals; fix by either adding a top-of-file global directive like `/*
global console */` or enabling Node globals in your ESLint config (set `env: {
node: true }`), then re-run linting to verify the warning for the
`console.error` in the main().catch block is resolved.
bidzyyys
reviewed
Mar 11, 2026
| /// Account owner. | ||
| owner: address, | ||
| /// Active order IDs keyed by pool ID (table entries are stored on-chain). | ||
| active_orders: Table<ID, vector<ID>>, |
Contributor
There was a problem hiding this comment.
Is it the best possible way for organising storage for order/pool ids?
Member
There was a problem hiding this comment.
It is fine if we don't have that many ID's in vector. But would like to see how is it used
Member
There was a problem hiding this comment.
Should be fine if vector<ID> doesn't contain that many elements
| const EBaseSpreadBpsExceedsMaxBasisPoints: vector<u8> = b"base spread bps must be at most 10000"; | ||
| #[error] | ||
| const EInvalidPythPriceFeedIdLength: vector<u8> = b"pyth price feed id must be 32 bytes"; | ||
| const EInvalidSpread: u64 = 1; |
Collaborator
There was a problem hiding this comment.
@bidzyyys while asserting and (potentialyl) throwing errors, Sui Move throws them as u64 numbers. That's the reason for using them like this.
Member
There was a problem hiding this comment.
@d-carmo it is, but we had in the previous feedback request to use clever errors
Member
There was a problem hiding this comment.
But this comment is outdated, since we already applied clever errors ref inside main. We need just resolve merge conflicts
Contributor
There was a problem hiding this comment.
Thanks @qalisander!
@d-carmo @CoveMB my main point is that we cannot afford repeating mistakes from M1 dApp.
Collaborator
Author
There was a problem hiding this comment.
because of divergent history changes made in previous MR are reverted here I'll review
| public struct AMMConfigCreatedEvent has copy, drop { | ||
| /// ID of the configuration object. | ||
| config_id: ID, | ||
| config_id: address, |
Contributor
There was a problem hiding this comment.
It confuses me -> config_id that is address type.
Collaborator
Author
There was a problem hiding this comment.
because of divergent history changes made in previous MR are reverted here I'll review
| AMMConfigUpdatedEvent { | ||
| config_id, | ||
| } | ||
| config_id: address, |
| /// Shared configs are readable by anyone; only the admin cap can update. | ||
| /// This function does not emit events. | ||
| public fun share_amm_config(config: AMMConfig) { | ||
| transfer::share_object(config); |
Contributor
There was a problem hiding this comment.
Why wrapper function for a single line function?
| transfer::share_object(config); | ||
| } | ||
|
|
||
| // === Public Functions === |
Contributor
There was a problem hiding this comment.
Function above are also public fun, so what's the difference?
| /// Verifies the admin capability is valid. | ||
| macro fun assert_admin_cap($admin_cap: &AMMAdminCap) { | ||
| let admin_cap = $admin_cap; | ||
| let _ = admin_cap.id.to_address(); |
Contributor
There was a problem hiding this comment.
No assert here, is it intended?
Collaborator
There was a problem hiding this comment.
My take: just checking it exists.
Contributor
There was a problem hiding this comment.
AMMAdminCp is implemented this way -> it must have existing id with UID type.
/// Capability required to update configuration.
public struct AMMAdminCap has key, store {
/// Unique ID for the admin capability object.
id: UID,
}Additionally, to_address() IMHO cannot fail as it is just a read of bytes inside UID.
Contributor
There was a problem hiding this comment.
IMHO file should be deleted.
bidzyyys
reviewed
Mar 11, 2026
| @@ -1,10 +1,13 @@ | |||
| [package] | |||
| name = "openzeppelin_market_maker" | |||
| name = "amm" | |||
Contributor
There was a problem hiding this comment.
Name & version to be properly set.
Comment on lines
+6
to
+7
| [dependencies] | ||
| deepbook = { git = "https://github.com/MystenLabs/deepbookv3.git", subdir = "packages/deepbook", rev = "main" } |
Contributor
There was a problem hiding this comment.
It's better to pin specific commit/version instead of main @CoveMB @qalisander
| initialSharedVersion?: string | ||
| } | ||
|
|
||
| const resolveKeepTemp = () => process.env.SUI_IT_KEEP_TEMP === "1" |
Contributor
There was a problem hiding this comment.
Looks like duplication from amm-create.test.ts.
Contributor
There was a problem hiding this comment.
Branch should be updated with main.
Member
|
@bidzyyys many issues are aftermath of outdated changes |
Minor text correction and being specific and clear about imports. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Daniel Carmo <97439344+d-carmo@users.noreply.github.com>
qalisander
reviewed
Mar 11, 2026
Comment on lines
+7
to
+8
| use sui::transfer; | ||
| use sui::tx_context::TxContext; |
Member
There was a problem hiding this comment.
We don't use some of those import
| /// Account owner. | ||
| owner: address, | ||
| /// Active order IDs keyed by pool ID (table entries are stored on-chain). | ||
| active_orders: Table<ID, vector<ID>>, |
Member
There was a problem hiding this comment.
It is fine if we don't have that many ID's in vector. But would like to see how is it used
| /// Account owner. | ||
| owner: address, | ||
| /// Active order IDs keyed by pool ID (table entries are stored on-chain). | ||
| active_orders: Table<ID, vector<ID>>, |
Member
There was a problem hiding this comment.
Should be fine if vector<ID> doesn't contain that many elements
Contributor
|
Replaced with #80. |
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
4 participants
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.
Fixes #25
Summary
Adds the AMM config management module in TypeScript: domain model parsing, PTB builders, and create/view script flows.
Scope
amm-createand useramm-viewscriptsNotes
This branch builds on contract-level manager support from earlier stack PRs.
Summary by CodeRabbit
New Features
Documentation
Configuration & Setup
Refactoring
Tests
Chores