Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: increasing max swap fee to 99.9999% #158

Merged
merged 4 commits into from
Jul 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion .forge-snapshots/newBCoWFactory.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
4890648
4899289
2 changes: 1 addition & 1 deletion .forge-snapshots/newBCoWPool.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
4034907
4042925
2 changes: 1 addition & 1 deletion .forge-snapshots/newBFactory.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
4131877
4140477
2 changes: 1 addition & 1 deletion .forge-snapshots/newBPool.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3478592
3486610
2 changes: 1 addition & 1 deletion src/contracts/BConst.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ contract BConst {
/// @notice The minimum swap fee that can be set.
uint256 public constant MIN_FEE = BONE / 10 ** 6;
/// @notice The maximum swap fee that can be set.
uint256 public constant MAX_FEE = BONE / 10;
uint256 public constant MAX_FEE = BONE - MIN_FEE;

Choose a reason for hiding this comment

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

why set it to BONE - MIN_FEE instead of just BONE? It doesn't seem to trigger any errors, which tells me that either

  • it's perfectly lawful
  • we're not testing for high fee values because of other pre-existing vm.assume or bound

I'm currently trying to make sure it's not the latter

Copy link
Member Author

Choose a reason for hiding this comment

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

i think it was chosen in order to avoid edge cases, which now that i'm looking at the BMath tree it reverts in every swap operation (because of the calcSpotPrice division by zero), but it doesn't revert in joinswap or exitswap, causing an unconsistent behaviour

because of this, i believe is better to provide a highly taxed normal behaviour than fall into unchecked reverting cases, i think now is better 99.9999% instead of allowing 100%

/// @notice The immutable exit fee percentage
uint256 public constant EXIT_FEE = 0;

Expand Down
28 changes: 18 additions & 10 deletions test/unit/BMath.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -113,15 +113,6 @@ contract BMathTest is Test, BConst {
bMath.calcOutGivenIn(balanceIn, weightIn, balanceOut, weightOut, amountIn, _swapFee);
}

function test_CalcOutGivenInWhenSwapFeeEqualsBONE() external virtual {
uint256 _swapFee = BONE;

// it should return zero
uint256 _amountOut = bMath.calcOutGivenIn(balanceIn, weightIn, balanceOut, weightOut, amountIn, _swapFee);

assertEq(_amountOut, 0);
}

function test_CalcOutGivenInRevertWhen_TokenAmountInTooBig(uint256 _amountIn) external {
_amountIn = bound(_amountIn, type(uint256).max / (BONE - swapFee) + 1, type(uint256).max);

Expand Down Expand Up @@ -154,6 +145,15 @@ contract BMathTest is Test, BConst {
bMath.calcOutGivenIn(_balanceIn, weightIn, balanceOut, weightOut, amountIn, _swapFee);
}

function test_CalcOutGivenInWhenSwapFeeEqualsBONE() external virtual {
uint256 _swapFee = BONE;

// it should return zero
uint256 _amountOut = bMath.calcOutGivenIn(balanceIn, weightIn, balanceOut, weightOut, amountIn, _swapFee);

assertEq(_amountOut, 0);
}

function test_CalcOutGivenInWhenTokenWeightInIsZero() external virtual {
uint256 _weightIn = 0;

Expand Down Expand Up @@ -477,7 +477,7 @@ contract BMathTest is Test, BConst {
bMath.calcPoolInGivenSingleOut(_balanceOut, weightOut, poolSupply, totalWeight, amountOut, swapFee);
}

function test_CalcPoolInGivenSingleOutRevertWhen_SwapFeeIs1AndTokenWeightOutIsZero() external {
function test_CalcPoolInGivenSingleOutRevertWhen_SwapFeeEqualsBONEAndTokenWeightOutIsZero() external {
uint256 _swapFee = BONE;
uint256 _weightOut = 0;

Expand All @@ -488,6 +488,14 @@ contract BMathTest is Test, BConst {
bMath.calcPoolInGivenSingleOut(balanceOut, _weightOut, poolSupply, totalWeight, amountOut, _swapFee);
}

function test_CalcPoolInGivenSingleOutRevertWhen_SwapFeeGreaterThanBONE() external {
// it should revert
// subtraction underflow
vm.expectRevert(BNum.BNum_SubUnderflow.selector);

bMath.calcPoolInGivenSingleOut(balanceOut, weightOut, poolSupply, totalWeight, amountOut, BONE + 1);
}

function test_CalcPoolInGivenSingleOutWhenTokenAmountOutIsZero() external virtual {
uint256 _amountOut = 0;

Expand Down
20 changes: 11 additions & 9 deletions test/unit/BMath.tree
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ BMathTest::calcSpotPrice
│ └── it should revert // division by zero
├── when weighted token balance out is zero
│ └── it should revert // division by zero
├── when swapFee greater than BONE
├── when swap fee greater than BONE
│ └── it should revert // subtraction underflow
├── when swapFee equals BONE
├── when swap fee equals BONE
│ └── it should revert // division by zero
├── when swap fee is zero
│ └── it should return correct value
Expand All @@ -21,14 +21,14 @@ BMathTest::calcOutGivenIn
│ └── it should revert // division by zero
├── when swap fee greater than BONE
│ └── it should revert // subtraction underflow
├── when swap fee equals BONE
│ └── it should return zero
├── when token amount in too big
│ └── it should revert // ai * (1 - sf) > uint256 max
├── when token balance in and amount in are zero
│ └── it should revert // bi + (ai * (1 - swapFee)) = 0
│ └── it should revert // bi + (ai * (1 - sf)) = 0
├── when token balance in is zero and swap fee equals BONE
│ └── it should revert // bi + (ai * (1 - swapFee)) = 0
│ └── it should revert // bi + (ai * (1 - sf)) = 0
├── when swap fee equals BONE
│ └── it should return zero
├── when token weight in is zero
│ └── it should return zero
├── when token weights are equal
Expand All @@ -53,9 +53,9 @@ BMathTest::calcInGivenOut
│ └── it should revert // subtraction underflow
├── when token amount out equals token balance out
│ └── it should revert // division by zero
├── when swapFee greater than BONE
├── when swap fee greater than BONE
│ └── it should revert // subtraction underflow
├── when swapFee equals BONE
├── when swap fee equals BONE
│ └── it should revert // division by zero
├── when token weight out is zero
│ └── it should return zero
Expand Down Expand Up @@ -121,8 +121,10 @@ BMathTest::calcSingleOutGivenPoolIn
BMathTest::calcPoolInGivenSingleOut
├── when token balance out is zero
│ └── it should revert // subtraction underflow
├── when swap fee is 1 and token weight out is zero
wei3erHase marked this conversation as resolved.
Show resolved Hide resolved
├── when swap fee equals BONE and token weight out is zero
│ └── it should revert // division by zero
├── when swap fee greater than BONE
│ └── it should revert // subtraction underflow
├── when token amount out is zero
│ └── it should return zero
├── when pool supply is zero
Expand Down
2 changes: 2 additions & 0 deletions test/unit/BPool.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ abstract contract BasePoolTest is Test, BConst, Utils, BMath {
uint256 _zoo = bsub(BONE, _normalizedWeight);
uint256 _zar = bmul(_zoo, _swapFee);
uint256 _tokenAmountOutBeforeSwapFee = bdiv(_tokenAmountOut, bsub(BONE, _zar));
vm.assume(_tokenOutBalance >= _tokenAmountOutBeforeSwapFee);
uint256 _newTokenOutBalance = bsub(_tokenOutBalance, _tokenAmountOutBeforeSwapFee);
vm.assume(_newTokenOutBalance < type(uint256).max / _tokenOutBalance);

Expand Down Expand Up @@ -1379,6 +1380,7 @@ contract BPool_Unit_ExitswapPoolAmountIn is BasePoolTest {
_fuzz.poolAmountIn,
_fuzz.swapFee
);
vm.assume(_fuzz.tokenOutBalance < type(uint256).max / MAX_OUT_RATIO);
vm.assume(_tokenAmountOut > bmul(_fuzz.tokenOutBalance, MAX_OUT_RATIO));

_setValues(_fuzz);
Expand Down
Loading