Skip to content

Add update scripts#81

Merged
bidzyyys merged 2 commits intomainfrom
fix/62-amm-update-port
Mar 11, 2026
Merged

Add update scripts#81
bidzyyys merged 2 commits intomainfrom
fix/62-amm-update-port

Conversation

@CoveMB
Copy link
Collaborator

@CoveMB CoveMB commented Mar 11, 2026

Parts of #26

Summary

Adds the AdminCap-managed AMM update script flow.

Scope

  • Owner amm-update script
  • Integration test covering update execution and updated snapshot checks

Notes

Admin capability resolution primitives are already available from the AMM domain layer in the prior stack PR.

Summary by CodeRabbit

  • New Features

    • Users can now update AMM (Automated Market Maker) configuration settings including base spreads, volatility multipliers, oracle price feeds, and trading pause status for active liquidity pools.
  • Tests

    • Comprehensive integration tests added to verify that AMM configuration updates execute successfully and persist correctly across different blockchain network deployments.

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

Walkthrough

This PR introduces a new AMM configuration update script and its integration test, enabling users to modify existing AMM settings on the target network through CLI arguments, resolving parameters from artifacts and current state, then executing the transaction and validating results.

Changes

Cohort / File(s) Summary
AMM Update Script
packages/dapp/src/scripts/owner/amm-update.ts
New script for updating AMM configurations with CLI argument handling, parameter resolution from artifacts and current state, transaction building, execution (supporting devInspect/dryRun), and JSON output formatting.
AMM Update Integration Test
packages/dapp/src/scripts/owner/test-integration/amm-update.test.ts
Comprehensive integration test validating the amm-update script end-to-end: environment setup, AMM creation, script execution, and verification of updated fields (baseSpreadBps, volatilityMultiplierBps, useLaser, tradingPaused, pythPriceFeedIdHex).

Sequence Diagram

sequenceDiagram
    participant CLI as User/CLI
    participant Script as AMM Update Script
    participant SUI as Sui Network
    participant Artifacts as Config Artifacts

    CLI->>Script: Invoke with update parameters
    Script->>Artifacts: Resolve package & config IDs
    Script->>Artifacts: Resolve admin cap ID
    Script->>SUI: Fetch current AMM config overview
    Script->>SUI: Fetch mutable shared object
    Script->>Script: Merge CLI args with current defaults
    Script->>Script: Compute new Pyth price feed ID
    Script->>SUI: Build & execute update transaction
    SUI-->>Script: Transaction result
    Script->>SUI: Fetch updated AMM config overview
    Script->>CLI: Output results (JSON or summary)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • qalisander
  • bidzyyys

Poem

🐰 The AMM spreads its wings anew, with updates swift and true,
A script that deftly turns the keys to change what users brew.
From Pyth feeds down to paused trades, each parameter takes flight,
The rabbit's work is now complete—the AMM shines so bright! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Add update scripts' is vague and generic; it does not clearly convey the specific change (AMM update script) and is insufficiently descriptive for scanning PR history. Replace with a more specific title like 'Add AMM update script and integration tests' to clearly describe the main changes.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the key scope and objective but lacks completion of the PR checklist items (Tests, Documentation, Changelog) which are required by the template.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/62-amm-update-port

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Member

@qalisander qalisander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/dapp/src/scripts/owner/test-integration/amm-update.test.ts (1)

51-139: LGTM!

Comprehensive integration test that exercises the full update flow: publish → create initial config → update config → verify output. Good use of waitForFinality after state-changing operations, and normalizeHex for robust hex comparison.

Consider adding additional test cases in the future for:

  • Partial updates (updating only some fields while others retain defaults)
  • Edge cases (e.g., updating with same values as current config)
  • Error scenarios (invalid admin cap, invalid config ID)

These can be deferred to a follow-up PR.

🤖 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-update.test.ts` around
lines 51 - 139, No code changes required—the integration test "owner amm-update
integration" correctly publishes, creates, updates, and verifies the AMM config
and uses waitForFinality and normalizeHex properly; keep as-is (no edits to
createSuiScriptRunner, parseJsonFromScriptOutput, AmmUpdateOutput handling, or
normalizeHex usage). If you choose to expand coverage later, add follow-up tests
that call the "amm-update" script runner to exercise partial updates, same-value
updates, and error scenarios (invalid adminCapId/ammConfigId) by asserting
expected transaction failures or unchanged fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/dapp/src/scripts/owner/test-integration/amm-update.test.ts`:
- Around line 51-139: No code changes required—the integration test "owner
amm-update integration" correctly publishes, creates, updates, and verifies the
AMM config and uses waitForFinality and normalizeHex properly; keep as-is (no
edits to createSuiScriptRunner, parseJsonFromScriptOutput, AmmUpdateOutput
handling, or normalizeHex usage). If you choose to expand coverage later, add
follow-up tests that call the "amm-update" script runner to exercise partial
updates, same-value updates, and error scenarios (invalid
adminCapId/ammConfigId) by asserting expected transaction failures or unchanged
fields.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0538423d-3487-4c46-924d-42ac2d376757

📥 Commits

Reviewing files that changed from the base of the PR and between aa97811 and 7c7e1b3.

📒 Files selected for processing (2)
  • packages/dapp/src/scripts/owner/amm-update.ts
  • packages/dapp/src/scripts/owner/test-integration/amm-update.test.ts

@bidzyyys bidzyyys merged commit cd649b5 into main Mar 11, 2026
10 checks passed
@bidzyyys bidzyyys deleted the fix/62-amm-update-port branch March 11, 2026 17:12
This was referenced Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants