Skip to content

Commit

Permalink
fix: max redeem rounding (#83)
Browse files Browse the repository at this point in the history
* fix: use convert to shares

* fix: tests

* test: add check
  • Loading branch information
Schlagonia authored Jan 31, 2024
1 parent d23f6d8 commit 0821426
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 22 deletions.
4 changes: 2 additions & 2 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
[submodule "lib/openzeppelin-contracts"]
path = lib/openzeppelin-contracts
url = https://github.com/OpenZeppelin/openzeppelin-contracts
release = v4.8.2
branch = v4.8.2
release = v4.9.5
branch = v4.9.5
[submodule "lib/erc4626-tests"]
path = lib/erc4626-tests
url = https://github.com/a16z/erc4626-tests
Expand Down
2 changes: 1 addition & 1 deletion lib/openzeppelin-contracts
4 changes: 2 additions & 2 deletions src/TokenizedStrategy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -935,8 +935,8 @@ contract TokenizedStrategy {
maxRedeem_ = _balanceOf(S, owner);
} else {
maxRedeem_ = Math.min(
// Use preview withdraw to round up
_previewWithdraw(S, maxRedeem_),
// Can't redeem more than the balance.
_convertToShares(S, maxRedeem_),
_balanceOf(S, owner)
);
}
Expand Down
28 changes: 15 additions & 13 deletions src/test/CustomImplementation.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ contract CustomImplementationsTest is Setup {

// Make sure max withdraw and redeem return the correct amounts
assertEq(strategy.maxWithdraw(_address), idle);
assertEq(strategy.maxRedeem(_address), strategy.previewWithdraw(idle));
assertEq(strategy.maxRedeem(_address), strategy.convertToShares(idle));
assertLe(
strategy.maxRedeem(_address),
strategy.convertToAssets(strategy.maxRedeem(_address)),
strategy.availableWithdrawLimit(_address)
);

Expand All @@ -67,9 +67,9 @@ contract CustomImplementationsTest is Setup {

// Make sure max withdraw and redeem return the correct amounts
assertEq(strategy.maxWithdraw(_address), idle);
assertEq(strategy.maxRedeem(_address), strategy.previewWithdraw(idle));
assertEq(strategy.maxRedeem(_address), strategy.convertToShares(idle));
assertLe(
strategy.maxRedeem(_address),
strategy.convertToAssets(strategy.maxRedeem(_address)),
strategy.availableWithdrawLimit(_address)
);

Expand All @@ -82,18 +82,20 @@ contract CustomImplementationsTest is Setup {
strategy.withdraw(_amount, _address, _address);

uint256 before = asset.balanceOf(_address);
uint256 redeem = strategy.previewWithdraw(idle);
uint256 redeem = strategy.maxRedeem(_address);
uint256 conversion = strategy.convertToAssets(_amount);

vm.prank(_address);
strategy.redeem(redeem, _address, _address);

// We need to give a i wei rounding buffer
assertApproxEq(asset.balanceOf(_address) - before, idle, 1);
assertApproxEq(strategy.availableWithdrawLimit(_address), 0, 1);
assertApproxEq(strategy.maxWithdraw(_address), 0, 1);
assertApproxEq(strategy.maxRedeem(_address), 0, 1);
strategy.redeem(redeem, _address, _address, 0);

// We need to give 2 wei rounding buffer
assertApproxEq(strategy.convertToAssets(_amount), conversion, 2);
assertApproxEq(asset.balanceOf(_address) - before, idle, 2);
assertApproxEq(strategy.availableWithdrawLimit(_address), 0, 2);
assertApproxEq(strategy.maxWithdraw(_address), 0, 2);
assertApproxEq(strategy.maxRedeem(_address), 0, 2);
assertLe(
strategy.maxRedeem(_address),
strategy.convertToAssets(strategy.maxRedeem(_address)),
strategy.availableWithdrawLimit(_address)
);
}
Expand Down
6 changes: 3 additions & 3 deletions src/test/e2e.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ contract e2eTest is Setup {
uint256 i;

for (i; i < toMake; ++i) {
asset = new ERC20Mock("Mock asset", "mcAsset", user, 0);
asset = new ERC20Mock();
yieldSource = new MockYieldSource(address(asset));
IMockStrategy newStrategy = IMockStrategy(setUpStrategy());

Expand Down Expand Up @@ -133,7 +133,7 @@ contract e2eTest is Setup {
uint256 i;

for (i; i < toMake; ++i) {
asset = new ERC20Mock("Mock asset", "mcAsset", user, 0);
asset = new ERC20Mock();
yieldSource = new MockYieldSource(address(asset));
IMockStrategy newStrategy = IMockStrategy(setUpStrategy());

Expand Down Expand Up @@ -247,7 +247,7 @@ contract e2eTest is Setup {
uint256 i;

for (i; i < toMake; ++i) {
asset = new ERC20Mock("Mock asset", "mcAsset", user, 0);
asset = new ERC20Mock();
yieldSource = new MockYieldSource(address(asset));
IMockStrategy newStrategy = IMockStrategy(setUpStrategy());

Expand Down
2 changes: 1 addition & 1 deletion src/test/utils/Setup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ contract Setup is ExtendedTest, IEvents {
tokenizedStrategy = new TokenizedStrategy();

// create asset we will be using as the underlying asset
asset = new ERC20Mock("Mock asset", "mcAsset", user, 0);
asset = new ERC20Mock();

// create a mock yield source to deposit into
yieldSource = new MockYieldSource(address(asset));
Expand Down

0 comments on commit 0821426

Please sign in to comment.