Skip to content

feat(amm): apply trading fees to LP accounting#46

Open
0x-r4bbit wants to merge 6 commits intomainfrom
feat/fee-system/apply
Open

feat(amm): apply trading fees to LP accounting#46
0x-r4bbit wants to merge 6 commits intomainfrom
feat/fee-system/apply

Conversation

@0x-r4bbit
Copy link
Copy Markdown
Collaborator

Implement Uniswap V2-style fees-in-reserves: the full swap_amount_in is deposited into the reserve (growing k = reserve_a * reserve_b), while only the fee-adjusted effective_amount_in is used to compute the output amount. This means LPs earn fees proportionally on every removal via k-growth rather than through a separate vault surplus.

  • swap_logic: add fee_bps parameter; compute effective_amount_in for output formula only; return full swap_amount_in as the reserve deposit
  • add_liquidity: remove vault balance checks; use pool reserves directly for ratio and LP calculations (vault == reserve invariant holds)
  • Fix all integration test fixture values to match fees-in-reserves math
  • Remove dead-code vault_a/b_init_zero helpers from unit tests

Comment thread amm/src/add.rs
Comment thread amm/src/add.rs
panic!(
"Add liquidity: AMM Program expects valid Fungible Token Holding Account for Vault A"
);
};
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.

LP calculation should be done on the reserves only (assuming they are up-to-date)
"Not up-to-date" means there's been a donation in the meantime. In which case, one can call syncReserves first.

We might also want to look into syncing reserves on any state changing operation to reduce likelihood of surplus.

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.

I think this is the main spec mismatch in the PR.

Issue #19 and the parent issue #7 explicitly describe a surplus-based model: the fee stays in the vault above tracked reserves, remove-liquidity pays from actual vault balances, and add-liquidity should use live vault balances after fee accrual so new LPs do not inherit previously earned fees. If we want reserve-only math instead, I think we should rewrite the issue/acceptance criteria first and review that design change explicitly.

Do you want me to do that?

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.

You can write the issue if you want. @fryorcraken has there been a verdict on whether or not we allow for recover surplus? My understanding was that we don't want/need it, so any surplus can be synced to the pool reserves (which benefits liquidity providers).

LPs are then always calculated based on reserves.
If a surplus has been created before and the LP doesn't call sync before removing, then it would be on them.

Comment thread amm/src/tests.rs
Comment thread amm/src/tests.rs
Copy link
Copy Markdown
Collaborator

@3esmit 3esmit left a comment

Choose a reason for hiding this comment

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

Revert removal of add_liquidity guard

@0x-r4bbit 0x-r4bbit force-pushed the feat/fee-system/apply branch from 34308eb to 8cccbc9 Compare April 14, 2026 11:47
Implement Uniswap V2-style fees-in-reserves: the full swap_amount_in is
deposited into the reserve (growing k = reserve_a * reserve_b), while
only the fee-adjusted effective_amount_in is used to compute the output
amount. This means LPs earn fees proportionally on every removal via
k-growth rather than through a separate vault surplus.

- swap_logic: add fee_bps parameter; compute effective_amount_in for
  output formula only; return full swap_amount_in as the reserve deposit
- add_liquidity: remove vault balance checks; use pool reserves directly
  for ratio and LP calculations (vault == reserve invariant holds)
- Fix all integration test fixture values to match fees-in-reserves math
- Remove dead-code vault_a/b_init_zero helpers from unit tests
@0x-r4bbit 0x-r4bbit force-pushed the feat/fee-system/apply branch from 8cccbc9 to 04c0490 Compare April 14, 2026 12:17
@3esmit 3esmit self-requested a review April 14, 2026 15:54
3esmit
3esmit previously requested changes Apr 14, 2026
Comment thread amm/src/swap.rs Outdated
// Compute deposit amount using ceiling division
// Formula: amount_in = ceil(reserve_in * exact_amount_out / (reserve_out - exact_amount_out))
let deposit_amount = reserve_deposit_vault_amount
// Compute the gross deposit amount the user must pay (fee inclusive).
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.

Change the exact-output input formula.

effective_in_min = ceil(reserve_in * amount_out / (reserve_out - amount_out))
deposit_amount = ceil(effective_in_min * FEE_DENOM / (FEE_DENOM - fee))

This matches the exact-input rounding and stops the 1-token undercharge case.

@3esmit
Copy link
Copy Markdown
Collaborator

3esmit commented Apr 14, 2026

The issue is that exact_output_swap_logic is supposed to invert the exact-input pricing rule, but it inverts the continuous formula instead of the discrete integer-rounded one.

In exact input

let effective_amount_in = swap_amount_in
.checked_mul(FEE_BPS_DENOMINATOR - fee_bps)
.expect("swap_amount_in * (FEE_BPS_DENOMINATOR - fee_bps) overflows u128")
/ FEE_BPS_DENOMINATOR;
assert!(
effective_amount_in != 0,
"Effective swap amount should be nonzero"
);
// Compute withdraw amount from fee-adjusted reserves while leaving the fee
// portion behind as vault surplus for LPs.
let withdraw_amount = reserve_withdraw_vault_amount
.checked_mul(effective_amount_in)
.expect("reserve * effective_amount_in overflows u128")
/ reserve_deposit_vault_amount
.checked_add(effective_amount_in)
.expect("reserve + effective_amount_in overflows u128");

the executed pricing function is:

effective_amount_in = floor(input * (10000 - fee) / 10000)
withdraw_amount = floor(reserve_out * effective_amount_in / (reserve_in + effective_amount_in))

In exact output

let numerator = reserve_deposit_vault_amount
.checked_mul(exact_amount_out)
.expect("reserve * amount_out overflows u128")
.checked_mul(FEE_BPS_DENOMINATOR)
.expect("reserve * amount_out * FEE_DENOM overflows u128");
let denominator = (reserve_withdraw_vault_amount - exact_amount_out)
.checked_mul(FEE_BPS_DENOMINATOR - fee_bps)
.expect("(reserve_out - amount_out) * (FEE_DENOM - fee) overflows u128");
let deposit_amount = numerator.div_ceil(denominator);

the code computes:

deposit_amount = ceil(reserve_in * amount_out * 10000 / ((reserve_out - amount_out) * (10000 - fee)))

That ceil is the inverse of the ideal rational formula, not the inverse of the actual integer-rounded function above. Because the exact-input path floors twice, deposit_amount can be 1 too small.

The proof test

lez-programs/amm/src/tests.rs

Lines 2442 to 2483 in 6d057ae

// This is a rounding-mismatch proof.
//
// For a 1000/1000 pool at 0.3%, asking for exact_amount_out = 1 should require
// max_amount_in >= 3. The current exact-output formula instead accepts 2:
// deposit = ceil(1000 * 1 * 10000 / (999 * 9970)) = 2
#[should_panic(expected = "Required input exceeds maximum amount in")]
#[test]
fn call_swap_exact_output_rejects_max_in_that_rounds_down_below_target_output() {
// Arrange
let pool = AccountWithMetadata {
account: Account {
program_owner: ProgramId::default(),
balance: 0,
data: Data::from(&PoolDefinition {
definition_token_a_id: IdForTests::token_a_definition_id(),
definition_token_b_id: IdForTests::token_b_definition_id(),
vault_a_id: IdForTests::vault_a_id(),
vault_b_id: IdForTests::vault_b_id(),
liquidity_pool_id: IdForTests::token_lp_definition_id(),
liquidity_pool_supply: MINIMUM_LIQUIDITY,
reserve_a: 1_000,
reserve_b: 1_000,
fees: FEE_TIER_BPS_30,
}),
nonce: Nonce(0),
},
is_authorized: true,
account_id: IdForTests::pool_definition_id(),
};
// Act
let _post_states = swap_exact_output(
pool,
AccountWithMetadataForTests::vault_a_init(),
AccountWithMetadataForTests::vault_b_init(),
AccountWithMetadataForTests::user_holding_a(),
AccountWithMetadataForTests::user_holding_b(),
1,
2,
IdForTests::token_a_definition_id(),
);
}

shows the concrete case:

  • pool 1000 / 1000, fee 30 bps, target output 1
  • exact-output accepts gross input 2
  • but exact-input math with gross input 2 gives effective_in = 1, then amount_out = 0
  • the true minimum gross input is 3

So the bug is: exact-output computes ceil(ideal inverse), but exact-input executes a floored integer function, and those are not equivalent.

@3esmit
Copy link
Copy Markdown
Collaborator

3esmit commented Apr 14, 2026

@3esmit
Copy link
Copy Markdown
Collaborator

3esmit commented Apr 14, 2026

fix(swap): revert changes in exact_output_swap_logic from original PR actually fixes the rounding error, the reason the tests failed are because of call_swap_exact_output_chained_call_successful which was expecting different format.
https://github.com/logos-blockchain/lez-programs/actions/runs/24416362575/job/71326484229#step:6:313
test tests::call_swap_exact_output_rejects_max_in_that_rounds_down_below_target_output - should panic ... ok

While fixing the test I realized that the format introduced by @0x-r4bbit, where the rounding happened was more clear and maintainable, so instead of changing the test, I adapted the division to apply that same format where numerator and denominator are separated. I will now request a review from AI and fix any findings they encounter.

@3esmit 3esmit self-assigned this Apr 14, 2026
@3esmit 3esmit requested a review from Copilot April 14, 2026 19:24
@3esmit 3esmit dismissed their stale review April 14, 2026 19:25

I already submitted the fix.

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 updates the AMM to apply Uniswap V2-style “fees-in-reserves” accounting: the pool receives the full (gross) input amount into reserves while the output amount is priced using a fee-adjusted (effective) input, causing LP value to accrue via k growth.

Changes:

  • Updated swap math to use fee-adjusted effective input for pricing while depositing the full swap input into reserves; added exact-output fee rounding logic.
  • Adjusted add-liquidity logic to rely on pool reserves for ratio/LP computations while still enforcing the vault balance ≥ reserve invariant.
  • Updated and expanded unit + integration tests (including new rounding-boundary and fee-accrual scenarios) to match the new accounting.

Reviewed changes

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

File Description
amm/src/swap.rs Implements fee-adjusted pricing for swaps while keeping gross deposits in reserves; updates exact-output input calculation to account for fee rounding.
amm/src/add.rs Uses pool reserves for add-liquidity math and centralizes vault-balance reads via a helper.
amm/src/tests.rs Updates swap/add-liquidity tests for fees-in-reserves and adds new rounding/fee-enforcement test coverage.
integration_tests/tests/amm.rs Updates fixture values and adds integration coverage for fee accrual across swaps and payout via remove/add liquidity.

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

Comment thread amm/src/swap.rs Outdated
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 updates the AMM to use Uniswap V2-style “fees-in-reserves” accounting: swaps deposit the full (gross) input into reserves while pricing uses a fee-adjusted effective input, so LPs earn fees via reserve (k) growth and receive them proportionally on liquidity removal.

Changes:

  • Update swap math to price with effective_amount_in (fee-adjusted) while crediting reserves with the full swap_amount_in; update exact-output swaps to enforce fee rounding correctly.
  • Refactor vault balance parsing via a shared helper and add new unit tests covering fee rounding boundaries and tiny-swap rejection cases.
  • Update integration test fixtures/expectations and add new integration scenarios validating fee accrual across swaps and payout on remove/add liquidity.

Reviewed changes

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

File Description
amm/src/swap.rs Implements fee-adjusted pricing with gross reserve crediting; updates exact-output math to correctly “lift” effective-in back to required gross-in under floor fee rounding.
amm/src/add.rs Refactors vault balance reads via helper and preserves vault≥reserve invariant checks before LP math.
amm/src/tests.rs Updates expected values for fee-in-reserves behavior and adds rounding/fee enforcement tests for both exact-input and exact-output paths.
integration_tests/tests/amm.rs Updates fixture balances/reserves for new accounting, introduces nonce helpers and reusable execution helpers, and adds integration tests for fee accrual effects.

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

Comment thread amm/src/add.rs
Comment on lines 50 to 60
@@ -80,7 +59,10 @@ pub fn add_liquidity(
"Vaults' balances must be at least the reserve amounts"
);
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.

@0x-r4bbit Can you answer this one?

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.

Ah, I need to update the commit message. This is about the invariant checks re: token.balance >= vault.reserves that I've originally removed from add_liquidity (which we then decided to put back in)

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.

4 participants