Add Minimum Liquidity Lock For AMM Pools#34
Add Minimum Liquidity Lock For AMM Pools#343esmit wants to merge 8 commits intologos-blockchain:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a minimum-liquidity lock mechanism for the AMM so pools cannot be drained to a zero LP supply state, by permanently locking MINIMUM_LIQUIDITY LP tokens in a dedicated lock-holding PDA and preventing remove_liquidity from withdrawing below that floor.
Changes:
- Add
MINIMUM_LIQUIDITYand a new PDA derivation for the LP lock-holding account inamm_core. - Update pool initialization (
new_definition) to lockMINIMUM_LIQUIDITYand mint only the remaining LP to the creator. - Update
remove_liquidityand expand unit/integration tests to enforce and validate the locked-liquidity invariant.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
amm/core/src/lib.rs |
Adds MINIMUM_LIQUIDITY, documents the invariant, and introduces LP lock-holding PDA derivation. |
amm/src/new_definition.rs |
Updates LP initialization flow to mint locked minimum LP to a lock PDA and mint remaining LP to the user. |
amm/src/remove.rs |
Prevents removals that would bring LP supply below the minimum-liquidity floor and keeps pool active. |
amm/src/tests.rs |
Adds/updates unit tests for locked-liquidity initialization and removal behavior. |
integration_tests/tests/amm.rs |
Updates integration expectations for initial user LP balance (net of locked minimum). |
Comments suppressed due to low confidence (1)
amm/src/new_definition.rs:109
is_new_poolis computed once, but later the code repeatsif pool.account == Account::default()when deciding whether to return a claimed post-state. Usingis_new_poolconsistently would avoid duplicate comparisons and reduce the risk of future divergence if the "new pool" condition changes.
let is_new_pool = pool.account == Account::default();
let pool_account_data = if is_new_pool {
PoolDefinition::default()
} else {
PoolDefinition::try_from(&pool.account.data)
.expect("AMM program expects a valid Pool account")
};
assert!(
!pool_account_data.active,
"Cannot initialize an active Pool Definition"
);
// LP Token minting calculation
let initial_lp = (token_a_amount.get() * token_b_amount.get()).isqrt();
assert!(
initial_lp > MINIMUM_LIQUIDITY,
"Initial liquidity must exceed minimum liquidity lock"
);
let user_lp = initial_lp - MINIMUM_LIQUIDITY;
// Update pool account
let mut pool_post = pool.account.clone();
let pool_post_definition = PoolDefinition {
definition_token_a_id,
definition_token_b_id,
vault_a_id: vault_a.account_id,
vault_b_id: vault_b.account_id,
liquidity_pool_id: pool_definition_lp.account_id,
liquidity_pool_supply: initial_lp,
reserve_a: token_a_amount.into(),
reserve_b: token_b_amount.into(),
fees: 0u128, // TODO: we assume all fees are 0 for now.
active: true,
};
pool_post.data = Data::from(&pool_post_definition);
let pool_post: AccountPostState = if pool.account == Account::default() {
AccountPostState::new_claimed(pool_post.clone())
} else {
AccountPostState::new(pool_post.clone())
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3esmit
changed the title
…iquidity handling
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
amm/core/src/lib.rs:30
- The
Instruction::NewDefinitiondocs enumerate the required accounts, but the implementation now also requires the LP-lock holding PDA account to exist/be writable for the token-program chained call that locksMINIMUM_LIQUIDITY. Please update the required-accounts list to include this PDA (and how to derive it), otherwise clients will construct transactions that are missing a required account.
/// Initializes a new Pool (or re-initializes an inactive Pool).
///
/// On initialization, `MINIMUM_LIQUIDITY` LP tokens are permanently locked
/// in the LP-lock holding PDA; the caller receives `initial_lp - MINIMUM_LIQUIDITY`.
///
/// Required accounts:
/// - AMM Pool
/// - Vault Holding Account for Token A
/// - Vault Holding Account for Token B
/// - Pool Liquidity Token Definition
/// - User Holding Account for Token A (authorized)
/// - User Holding Account for Token B (authorized)
/// - User Holding Account for Pool Liquidity
NewDefinition {
amm/src/new_definition.rs:223
new_definitionreturns(post_states.clone(), chained_calls), which unnecessarily clones the fullVec<AccountPostState>(and any inner data) right before returning. Returnpost_statesdirectly to avoid extra allocation/copies and keep the function consistent with the other AMM instruction helpers.
let post_states = vec![
pool_post.clone(),
AccountPostState::new(vault_a.account.clone()),
AccountPostState::new(vault_b.account.clone()),
AccountPostState::new(pool_definition_lp.account.clone()),
AccountPostState::new(user_holding_a.account.clone()),
AccountPostState::new(user_holding_b.account.clone()),
AccountPostState::new(user_holding_lp.account.clone()),
];
(post_states.clone(), chained_calls)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
CI failed due GitHub API rate limit exceeded for 52.234.42.40 on an unauthenticated request. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Honest flows should never reach the permanent lock through a valid remove instruction, but | ||
| // we still reject legacy or corrupted states that are already at the locked floor. | ||
| assert!( | ||
| pool_def_data.liquidity_pool_supply > MINIMUM_LIQUIDITY, | ||
| "Pool only contains locked liquidity" | ||
| ); | ||
| let unlocked_liquidity = pool_def_data.liquidity_pool_supply - MINIMUM_LIQUIDITY; | ||
| // The remove instruction never sees the LP lock account directly, so we must still refuse any | ||
| // request that would burn through the permanent floor even if ownership is already corrupted. | ||
| assert!( | ||
| remove_liquidity_amount <= unlocked_liquidity, |
There was a problem hiding this comment.
remove_liquidity now unconditionally asserts liquidity_pool_supply > MINIMUM_LIQUIDITY, which rejects any active pool with LP supply <= MINIMUM_LIQUIDITY (including pools created before the lock was introduced that may legitimately have smaller initial LP supply). This can make such pools impossible to withdraw from after upgrading. Consider gating the minimum-liquidity floor enforcement to pools that were created under the new rules (or otherwise allowing the legacy path to drain below MINIMUM_LIQUIDITY / to zero), and only rejecting the exact locked-floor case for new pools.
| // Honest flows should never reach the permanent lock through a valid remove instruction, but | |
| // we still reject legacy or corrupted states that are already at the locked floor. | |
| assert!( | |
| pool_def_data.liquidity_pool_supply > MINIMUM_LIQUIDITY, | |
| "Pool only contains locked liquidity" | |
| ); | |
| let unlocked_liquidity = pool_def_data.liquidity_pool_supply - MINIMUM_LIQUIDITY; | |
| // The remove instruction never sees the LP lock account directly, so we must still refuse any | |
| // request that would burn through the permanent floor even if ownership is already corrupted. | |
| assert!( | |
| remove_liquidity_amount <= unlocked_liquidity, | |
| // Pools created with a permanent minimum-liquidity lock must retain that floor, but legacy | |
| // pools created before the lock was introduced may legitimately have total supply at or below | |
| // MINIMUM_LIQUIDITY and must still be removable. | |
| let max_removable_liquidity = if pool_def_data.liquidity_pool_supply > MINIMUM_LIQUIDITY { | |
| pool_def_data.liquidity_pool_supply - MINIMUM_LIQUIDITY | |
| } else { | |
| pool_def_data.liquidity_pool_supply | |
| }; | |
| // The remove instruction never sees the LP lock account directly, so for pools that are above | |
| // the locked floor we still refuse any request that would burn through the permanent minimum. | |
| assert!( | |
| remove_liquidity_amount <= max_removable_liquidity, |
| /// Initializes a new Pool (or re-initializes an inactive Pool). | ||
| /// | ||
| /// On initialization, `MINIMUM_LIQUIDITY` LP tokens are permanently locked | ||
| /// in the LP-lock holding PDA; the caller receives `initial_lp - MINIMUM_LIQUIDITY`. | ||
| /// | ||
| /// Required accounts: | ||
| /// - AMM Pool | ||
| /// - Vault Holding Account for Token A | ||
| /// - Vault Holding Account for Token B | ||
| /// - Pool Liquidity Token Definition | ||
| /// - LP Lock Holding Account, derived as `compute_lp_lock_holding_pda(amm_program_id, | ||
| /// pool.account_id)` | ||
| /// - User Holding Account for Token A (authorized) | ||
| /// - User Holding Account for Token B (authorized) | ||
| /// - User Holding Account for Pool Liquidity |
There was a problem hiding this comment.
PR description states the AMM public instruction interfaces are unchanged, but NewDefinition now requires an additional account (lp_lock_holding) as reflected in the IDL and the Instruction::NewDefinition required-accounts documentation. Please update the PR description to acknowledge this client-facing account-list/order change (even if the enum variant data is unchanged).
0x-r4bbit
left a comment
0x-r4bbit
left a comment
There was a problem hiding this comment.
Let's get the commit updated and merge
| vault_a: AccountWithMetadata, | ||
| vault_b: AccountWithMetadata, | ||
| pool_definition_lp: AccountWithMetadata, | ||
| lp_lock_holding: AccountWithMetadata, |
There was a problem hiding this comment.
This is a breaking change. Let's make sure we cover this via conventional commits
|
|
||
| let mut pool_lp_after_lock = pool_lp_auth.clone(); | ||
| if pool_definition_lp.account == Account::default() { | ||
| pool_lp_after_lock.account.program_owner = token_program_id; |
There was a problem hiding this comment.
I understand that this is necessary, but it feels off to me.
Not making this a blocker, but we should explore how to handle such cases nicer in the future.
/cc @schouhy
|
Landed via #39 |
Fixes #6
Summary
This PR adds a minimum liquidity lock to the AMM so pools cannot be fully drained to zero LP supply.
On initialization, the AMM now locks
MINIMUM_LIQUIDITYLP tokens in a dedicated lock holding PDA and gives the pool creator the remaining LP supply. Liquidity removal is updated to preserve that invariant.Changes
MINIMUM_LIQUIDITYand document the invariant in AMM core typesTesting
cargo test -p amm_programcargo test -p integration_tests ammNotes
initial_lp - MINIMUM_LIQUIDITY, and pools cannot be drained below the locked minimum.new_definition: mint the full LP supply into the lock holding account first and transfer the user share out, which would avoid the intermediate LP definition reconstruction step currently used to support the second LP chained call.