Remove Redundant active Flag From AMM PoolDefinition#44
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the redundant active field from amm_core::PoolDefinition (and its serialized layout) and updates the AMM to gate “pool readiness” using explicit liquidity_pool_supply checks (primarily against MINIMUM_LIQUIDITY), with corresponding fixture/test updates.
Changes:
- Removes
activefromPoolDefinitionand updates docs/comments to describe LP-supply-based readiness semantics. - Updates
new_definitionto reject initialization when the existing pool has nonzero LP supply. - Replaces
activechecks with explicitliquidity_pool_supplyminimum-liquidity checks in swap/sync/remove flows; updates unit and integration tests/fixtures accordingly.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
amm/core/src/lib.rs |
Removes active from the serialized PoolDefinition schema and updates related docs. |
amm/src/new_definition.rs |
Replaces active gating with liquidity_pool_supply == 0 precondition for initialization/reinit. |
amm/src/swap.rs |
Updates swap validation to require liquidity_pool_supply >= MINIMUM_LIQUIDITY. |
amm/src/sync.rs |
Updates sync gating to require liquidity_pool_supply >= MINIMUM_LIQUIDITY. |
amm/src/remove.rs |
Updates remove-liquidity gating to require liquidity_pool_supply >= MINIMUM_LIQUIDITY and removes active field updates. |
amm/src/tests.rs |
Refactors unit test fixtures and expected panic messages to use LP-supply semantics. |
integration_tests/tests/amm.rs |
Refactors integration fixtures and test names to remove active and model zero-supply/below-min states explicitly. |
amm/methods/guest/src/bin/amm.rs |
Updates instruction docs to reference “existing zero-supply” reinitialization semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3eeb2e9 to
d2edb75
Compare
0x-r4bbit
left a comment
There was a problem hiding this comment.
Change LGTM!
Only thing I'd ask is to mark this change as a breaking change in the commit message (we're removing a field in the pool definition)
d2edb75 to
1adbd43
Compare
Added a |
|
Please check how this is done here: https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with-scope-and--to-draw-attention-to-breaking-change |
1adbd43 to
6bd7900
Compare
…plicit flag Remove the `active: bool` field from `PoolDefinition` and replace it with an implicit invariant: a pool is considered active when `liquidity_pool_supply >= MINIMUM_LIQUIDITY`. BREAKING CHANGE: `PoolDefinition` Borsh serialization format has changed. Existing on-chain pool accounts encoded with the `active` field are incompatible with this version. Closes #25
6bd7900 to
8259c18
Compare
|
@3esmit updated the commit message for you and rebased. Will merge once green. |
Fixes #25
Summary
This PR removes the redundant
activeflag fromamm_core::PoolDefinitionand replaces its callers with explicitliquidity_pool_supplychecks.After the minimum-liquidity lock,
activeno longer tracked anything meaningfully distinct from pool initialization, so the AMM now makes those readiness checks directly from LP supply at each call site.Changes
activefrom the serializedPoolDefinitionschemanew_definitionto reject any existing pool definition with nonzero LP supplyswap_exact_input,swap_exact_output,sync_reserves, andremove_liquidityto gate on explicit minimum-liquidity checks instead ofactiveTesting
cargo +1.94.0 test -p amm_programRISC0_DEV_MODE=1 cargo +1.94.0 test -p integration_tests --test ammcargo +nightly fmt --all -- --checktaplo fmt --check .RISC0_SKIP_BUILD=1 cargo +1.94.0 clippy --workspace --all-targets -- -D warningsRISC0_DEV_MODE=1 cargo +1.94.0 test --workspace --exclude integration_testsRISC0_DEV_MODE=1 cargo +1.94.0 test -p integration_testsNotes
PoolDefinitionlayout by removingactive, so older serialized pool bytes are not schema-compatible with this version.new_definition#26. The current zero-supply reinitialization branch innew_definitionremains as a follow-up cleanup risk.