Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@ pub fn swap_exact_tokens_for_tokens(
input_amount
};

// Apply trading fee, used to compute the output
// Apply trading fee, used to compute the output.
// u128 avoids overflow when input * fee approaches u64::MAX.
let amm = &ctx.accounts.amm;
let taxed_input = input - input * amm.fee as u64 / 10000;
let fee_amount = ((input as u128) * (amm.fee as u128) / 10000) as u64;

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.

should use checked_ math for better handling of overflow

let taxed_input = input - fee_amount;

let pool_a = &ctx.accounts.pool_account_a;
let pool_b = &ctx.accounts.pool_account_b;
Expand All @@ -51,8 +53,8 @@ pub fn swap_exact_tokens_for_tokens(
return err!(TutorialError::OutputTooSmall);
}

// Compute the invariant before the trade
let invariant = pool_a.amount * pool_b.amount;
// Compute the invariant before the trade (u128 to avoid overflow on large pools)
let invariant = (pool_a.amount as u128) * (pool_b.amount as u128);

// Transfer tokens to the pool
let authority_bump = ctx.bumps.pool_authority;
Expand Down Expand Up @@ -99,7 +101,7 @@ pub fn swap_exact_tokens_for_tokens(
},
signer_seeds,
),
input,
output,
)?;
token::transfer(
CpiContext::new(
Expand All @@ -110,7 +112,7 @@ pub fn swap_exact_tokens_for_tokens(
authority: ctx.accounts.trader.to_account_info(),
},
),
output,
input,
)?;
}

Expand All @@ -126,7 +128,8 @@ pub fn swap_exact_tokens_for_tokens(
// We tolerate if the new invariant is higher because it means a rounding error for LPs
ctx.accounts.pool_account_a.reload()?;
ctx.accounts.pool_account_b.reload()?;
if invariant > ctx.accounts.pool_account_a.amount * ctx.accounts.pool_account_a.amount {
let new_invariant = (ctx.accounts.pool_account_a.amount as u128) * (ctx.accounts.pool_account_b.amount as u128);
if invariant > new_invariant {
return err!(TutorialError::InvariantViolated);
}

Expand Down
32 changes: 32 additions & 0 deletions tokens/token-swap/anchor/tests/swap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,36 @@ describe('Swap', () => {
values.defaultSupply.sub(values.depositAmountB).add(input).toNumber(),
);
});

it('Swap from B to A', async () => {
const input = new anchor.BN(3 * 10 ** 6);
await program.methods
.swapExactTokensForTokens(false, input, new anchor.BN(100))
.accountsPartial({
amm: values.ammKey,
pool: values.poolKey,
poolAuthority: values.poolAuthority,
trader: values.admin.publicKey,
mintA: values.mintAKeypair.publicKey,
mintB: values.mintBKeypair.publicKey,
poolAccountA: values.poolAccountA,
poolAccountB: values.poolAccountB,
traderAccountA: values.holderAccountA,
traderAccountB: values.holderAccountB,
})
.signers([values.admin])
.rpc({ skipPreflight: true });

const traderTokenAccountA = await connection.getTokenAccountBalance(values.holderAccountA);
const traderTokenAccountB = await connection.getTokenAccountBalance(values.holderAccountB);
expect(traderTokenAccountB.value.amount).to.equal(
values.defaultSupply.sub(values.depositAmountB).sub(input).toString(),
);
expect(Number(traderTokenAccountA.value.amount)).to.be.greaterThan(
values.defaultSupply.sub(values.depositAmountA).toNumber(),
);
expect(Number(traderTokenAccountA.value.amount)).to.be.lessThan(
values.defaultSupply.sub(values.depositAmountA).add(input).toNumber(),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Output assertion accepts wrong amounts

The broad balance range accepts any positive A output below input, so regressions in the output formula, fee application, reserve selection, or rounding can pass without verifying the exact corrected result.

Suggested change
expect(Number(traderTokenAccountA.value.amount)).to.be.greaterThan(
values.defaultSupply.sub(values.depositAmountA).toNumber(),
);
expect(Number(traderTokenAccountA.value.amount)).to.be.lessThan(
values.defaultSupply.sub(values.depositAmountA).add(input).toNumber(),
);
expect(traderTokenAccountA.value.amount).to.equal(
values.defaultSupply
.sub(values.depositAmountA)
.add(new anchor.BN(2_961_038))
.toString(),
);

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

});
});
Loading