Skip to content

Add RecoverSurplus Instruction For AMM Pools#38

Open
3esmit wants to merge 3 commits intologos-blockchain:mainfrom
3esmit:feat/amm-recover-surplus
Open

Add RecoverSurplus Instruction For AMM Pools#38
3esmit wants to merge 3 commits intologos-blockchain:mainfrom
3esmit:feat/amm-recover-surplus

Conversation

@3esmit
Copy link
Copy Markdown
Collaborator

@3esmit 3esmit commented Apr 8, 2026

Summary

This PR adds a RecoverSurplus instruction to the AMM so vault balances above the tracked reserves can be explicitly transferred out without changing pool reserve accounting.

The implementation is scoped to non-reserve-backed balances only and includes the validation needed to keep recovery bounded to the correct token accounts and pool states.

Changes

  • add RecoverSurplus and RecoverSurplusMode to AMM core types
  • add the guest entrypoint for recover_surplus
  • add AMM surplus-recovery logic that computes and transfers only vault surplus
  • validate vault and recipient token definitions before recovery
  • restrict the initial recovery mode to inactive or zero-supply pools
  • add focused AMM unit coverage for success, active-pool rejection, definition mismatch, and under-collateralized vault failures
  • update the checked-in AMM IDL to include the new instruction and mode enum

Testing

  • cargo +nightly fmt --all -- --check
  • taplo fmt --check .
  • RISC0_SKIP_BUILD=1 cargo +1.94.0 clippy --workspace --all-targets -- -D warnings
  • RISC0_DEV_MODE=1 cargo +1.94.0 test --workspace --exclude integration_tests
  • RISC0_DEV_MODE=1 cargo +1.94.0 test -p integration_tests

Notes

  • RecoverSurplus leaves the pool’s stored reserves unchanged because it transfers only balances above those reserves.
  • The initial mode intentionally keeps recovery unavailable for active pools with live LP supply.
  • The implementation includes the follow-up fix that validates vault token definitions before recovery proceeds.

Copilot AI review requested due to automatic review settings April 8, 2026 22:39
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a new AMM instruction (RecoverSurplus) that allows transferring vault balances above the pool’s tracked reserves out to specified recipient token holding accounts, without changing the pool’s reserve accounting. This fits into the AMM program as an administrative/cleanup mechanism for non-reserve-backed balances (e.g., donations) while preserving invariant accounting.

Changes:

  • Introduces RecoverSurplus + RecoverSurplusMode in amm_core and wires the new guest entrypoint.
  • Implements surplus computation + bounded transfer logic (vault/recipient token definition validation, inactive/zero-supply gating, under-collateralization checks).
  • Adds unit tests and updates the checked-in AMM IDL to include the new instruction.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
amm/src/vault_utils.rs Adds a small helper to consistently parse/validate fungible token holding accounts.
amm/src/recover.rs Implements surplus calculation, validation, and chained token transfers for vault surplus recovery.
amm/src/lib.rs Exposes the new recover module and adds the internal vault utils module.
amm/methods/guest/src/bin/amm.rs Adds the guest-program instruction entrypoint for recover_surplus.
amm/core/src/lib.rs Extends the core instruction enum with RecoverSurplus and introduces RecoverSurplusMode.
amm/src/tests.rs Adds unit tests for success and key rejection/failure paths of surplus recovery.
amm/amm-idl.json Updates the generated/checked-in IDL to include the new instruction and enum-typed arg.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@3esmit 3esmit self-assigned this Apr 9, 2026
@3esmit 3esmit linked an issue Apr 9, 2026 that may be closed by this pull request
@3esmit 3esmit force-pushed the feat/amm-recover-surplus branch from 6a90359 to ffee837 Compare April 9, 2026 03:51
@3esmit 3esmit requested a review from Copilot April 9, 2026 04:01
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +9 to +16
pub fn recover_surplus(
pool: AccountWithMetadata,
vault_a: AccountWithMetadata,
vault_b: AccountWithMetadata,
to_holding_a: AccountWithMetadata,
to_holding_b: AccountWithMetadata,
mode: RecoverSurplusMode,
) -> (Vec<AccountPostState>, Vec<ChainedCall>) {
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

recover_surplus authorizes the vault accounts (via PDA seeds) and allows the caller to choose arbitrary recipient holding accounts. Since there is no additional signer/authorization check, any caller can sweep all surplus to themselves whenever the pool is inactive or has zero LP supply. If that’s not intended, add an explicit recovery authority account (required to be authorized) and/or constrain recipients to a pool-configured destination before issuing the transfer chained calls.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, we need to align this with the team @0x-r4bbit, if this PR ever get merged. The caller-controlled recipient observation is accurate, but the intended model here is still for now permissionless recovery once the pool has been deactivated. The change Im making is to tighten the gate so RecoverSurplus is callable only when pool.active == false, which was an oversight I had while reimplementing it; Im removing the earlier active-with-zero-supply allowance rather than adding a separate recovery authority.

Comment on lines +274 to +276
"type": {
"defined": "RecoverSurplusMode"
}
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

The checked-in IDL introduces an arg type { "defined": "RecoverSurplusMode" }, but this IDL format doesn’t define custom types anywhere (and no RecoverSurplusMode definition exists in this file). This likely breaks IDL consumers/generators. Consider encoding mode as a supported primitive (e.g., u8) or extending the IDL schema to include and emit enum/type definitions consistently.

Suggested change
"type": {
"defined": "RecoverSurplusMode"
}
"type": "u8"

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for calling this out. What do you think @0x-r4bbit? I checked it against the current repo tooling before changing the wire shape: the repository already ships other IDLs that use defined references, and the current spel CLI loads amm/amm-idl.json successfully and exposes recover-surplus --mode <RecoverSurplusMode> without error. Based on that evidence, I’m keeping the current IDL shape in this PR.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did you test the IDL?

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +69 to +75
match mode {
RecoverSurplusMode::InactiveOrZeroSupplyOnly => {
assert!(
!pool_def_data.active,
"Recover surplus is only allowed for inactive pools"
);
}
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

PR description says recovery is restricted to "inactive or zero-supply" pools, but the implementation for RecoverSurplusMode::InactiveOrZeroSupplyOnly currently rejects any pool_def_data.active == true case (including active + zero LP supply). Either update the PR description to match the current behavior, or adjust the mode gating logic (and tests/docs) if zero-supply active pools should be allowed.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

@0x-r4bbit
Copy link
Copy Markdown
Collaborator

These changes are likely not desired.
Let's wait for @fryorcraken to settle the decision on this.

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.

Add RecoverSurplus Instruction For AMM Pools

3 participants