fix(mock-amm): add withdraw_liquidity so seeded liquidity is recoverable

[Rob-in-son]

Fixes #8

Problem

deposit_liquidity moves token_out into the pool, but there was no counterpart, so any liquidity seeded for testnet swaps was permanently locked in the contract.

Change

Adds withdraw_liquidity(to, amount), gated to the pool admin:

• Non-positive amounts → new InvalidAmount error (= 4)
• Amount above the pool's balance → existing InsufficientLiquidity
• Balances are untouched on both rejection paths

No LP-share accounting — this is a testnet mock, so seeded liquidity is recoverable only by the pool admin. If the AMM ever becomes more than a mock, that's the point to revisit.

Note on the signature

The issue proposed withdraw_liquidity(admin, to, amount). This implementation drops the admin argument and authenticates from stored state instead.

That's deliberate: accepting an admin address as an argument is explicitly ruled out in this codebase. See the comment on require_admin in wpi-token/src/lib.rs — "Callers must never accept an admin address as an argument … doing so couples correctness to argument order and lets a caller-supplied address silently stand in for the real admin" — added in 7bfb3a6, which removed that pattern from wpi-token for exactly this reason. Writing new privileged code in the pattern the repo just finished eradicating seemed like the wrong trade.

Both acceptance criteria are still met. Happy to switch to the literal signature from the issue if maintainers prefer parity — it's a small change, and I'd rather settle it in review than assume.

upgrade() still takes an admin argument (it compares against stored admin, so it is safe, just older style). Left alone as out of scope.

Also included: a CI fix (eef6676)

The contracts job has been failing on main since #57, unrelated to this change. Since it would have shown up as a red X on this PR, it's fixed here, isolated in its own commit:

• cargo fmt --check diffs in wpi-token/src/lib.rs (remove_proposed_admin) and test.rs (trailing whitespace, an unwrapped mock_auths chain)
• test.rs:470 called client.volume_limit_config().unwrap(), but the generated client already unwraps, so VolumeLimitConfig has no unwrap — a hard compile error (E0599) that meant cargo test never ran at all
• 14 admin bindings left unused after fix(wpi-token): authenticate admin from stored state, not caller-supplied argument #55 removed the admin argument, each fatal under clippy's -D warnings

No contract behaviour or test assertions were changed. Happy to split this into its own PR if you'd rather review it separately — it cherry-picks cleanly.

Tests

Seven in mock-amm, all passing. The existing swap test is refactored onto a shared setup helper that the new tests reuse.

Test	Covers
deposit_and_withdraw_liquidity_round_trip	AC: deposit → withdraw round trip, including a partial withdraw
withdraw_liquidity_recovers_the_remainder_after_a_swap	Pool drained after swap activity
withdraw_liquidity_can_send_to_an_address_other_than_the_admin	Recipient need not be the admin
withdraw_liquidity_rejects_more_than_the_pool_holds	InsufficientLiquidity, balance unchanged
withdraw_liquidity_rejects_non_positive_amounts	InvalidAmount for 0 and -1, balance unchanged
non_admin_signer_cannot_authenticate_withdraw_liquidity	Attacker signer cannot satisfy require_auth
swaps_against_registered_stellar_asset_contract	Pre-existing, refactored only

Verified locally against every gate the contracts job runs:

Gate	Result
cargo fmt --all -- --check	pass
cargo clippy --locked --all-targets --all-features -- -D warnings	pass
cargo test --locked --all	30/30 (7 mock-amm, 22 wpi-token, 1 token-common)
cargo build --locked --target wasm32-unknown-unknown --release	pass
verify_dependency_provenance.sh	pass

No WASM baseline update needed — mock_amm is still 0 in wasm-size-baseline.json, so the size gate reports "no baseline" and skips.

Out of scope

token_in (wPi) accumulated in the pool from swaps is still permanently locked. Same class of bug as this issue, but not covered by #8 — worth a follow-up issue.

[coderabbitai]

📝 Walkthrough

Walkthrough

MockAmm adds an admin-authorized withdraw_liquidity entrypoint with amount and balance validation, token transfer handling, and expanded liquidity tests. wPi-token also receives formatting and unused-binding test cleanup.

Changes

Mock AMM liquidity management

Layer / File(s)	Summary
Withdrawal contract entrypoint Stellar-contracts-v1/mock-amm/src/lib.rs	Adds Error::InvalidAmount, storage readers, and withdraw_liquidity with authorization, amount, balance, and transfer checks.
Liquidity management test coverage Stellar-contracts-v1/mock-amm/src/lib.rs	Adds shared pool setup and tests for deposits, swaps, withdrawals, recipients, insufficient liquidity, invalid amounts, and authorization.

wPi-token maintenance

Layer / File(s)	Summary
Storage cleanup and test maintenance Stellar-contracts-v1/wpi-token/src/lib.rs, Stellar-contracts-v1/wpi-token/src/test.rs	Reformats proposed-admin storage removal and renames unused admin bindings while preserving test behavior and assertions.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Admin
  participant MockAmm
  participant TokenContract
  participant Recipient
  Admin->>MockAmm: withdraw_liquidity(to, amount)
  MockAmm->>TokenContract: check token_out balance
  MockAmm->>TokenContract: transfer token_out
  TokenContract-->>Recipient: receive withdrawn tokens

Loading

Possibly related PRs

• Pi-Defi-world/Wpi#43: Both changes add admin-gated MockAmm operations using stored admin authorization.
• Pi-Defi-world/Wpi#48: The MockAmm withdrawal uses the stored output-token configuration introduced by this earlier refactor.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR includes unrelated wpi-token formatting and test cleanup that do not support mock-amm liquidity withdrawal.	Remove the wpi-token cleanup from this PR or split it into a separate change, keeping only the mock-amm withdrawal work.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The admin-gated withdraw_liquidity and deposit-to-withdraw tests satisfy the linked issue's liquidity recovery requirements.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the main change: adding withdraw_liquidity to mock-amm to recover seeded liquidity.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}