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

Add settleAndRefund #2

Merged
merged 5 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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/VaultTest#Vault.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
6762
7018
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
122540
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#registerPoolManager.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24484
24506
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#testLock_NoOp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
11502
11597
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
79085
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
36485
23 changes: 22 additions & 1 deletion src/Vault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,35 @@ contract Vault is IVault, VaultToken, Ownable {
}

/// @inheritdoc IVault
function settle(Currency currency) external payable override isLocked returns (uint256 paid) {
function settle(Currency currency) public payable override isLocked returns (uint256 paid) {
uint256 reservesBefore = reservesOfVault[currency];
reservesOfVault[currency] = currency.balanceOfSelf();
paid = reservesOfVault[currency] - reservesBefore;
// subtraction must be safe
SettlementGuard.accountDelta(msg.sender, currency, -(paid.toInt128()));
}

function settleAndRefund(Currency currency, address to)
external
payable
isLocked
returns (uint256 paid, uint256 refund)
{
paid = settle(currency);
Copy link
Collaborator

@ChefSnoopy ChefSnoopy Mar 20, 2024

Choose a reason for hiding this comment

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

Should we calculate the amount we need to refund in advance? then just need to execute SettlementGuard.accountDelta once if have excess token.

function settleAndRefund(Currency currency, address to)
        external
        payable
        override
        isLocked
        returns (uint256 paid, uint256 refund)
    {
        paid = currency.balanceOfSelf() - reservesOfVault[currency];
        // need to consider the case when the currencyDelta is negative ?.
        uint256 currentCurrencyDelta = (SettlementGuard.getCurrencyDelta(msg.sender, currency)).toUint256();
       
        if (paid > currentCurrencyDelta) {
            refund = paid - currentCurrencyDelta;
            paid = currentCurrencyDelta;
        }
        reservesOfVault[currency] += paid;
        SettlementGuard.accountDelta(msg.sender, currency, -(paid.toInt128()));
        if (refund > 0) {
            currency.transfer(to, refund);
        }
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks! adopted this idea since it'll save 1 sload for refund case. though did a small tweak to handle negative currency delta as settle() wont revert with SafeCastOverflow for negative balance delta


int256 afterCurrencyDelta = SettlementGuard.getCurrencyDelta(msg.sender, currency);
if (afterCurrencyDelta < 0) {
/// If user currencyDelta is negative (vault owes owner) after settle: either user have overpaid
/// or someone have transfer token to the vault.
refund = uint256(-afterCurrencyDelta);

/// thus refund msg.sender and update accountDelta
SettlementGuard.accountDelta(msg.sender, currency, refund.toInt128());
reservesOfVault[currency] -= refund;
currency.transfer(to, refund);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to think about whether there are re-entrant issues.
Let me think about this. maybe need to put this in the end after updated reservesOfVault and SettlementGuard.

}
}

/// @inheritdoc IVault
function settleFor(Currency currency, address target, uint256 amount) external isLocked {
/// @notice settle all outstanding debt if amount is 0
Expand Down
5 changes: 5 additions & 0 deletions src/interfaces/IVault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ interface IVault is IVaultToken {
/// @notice Called by the user to pay what is owed
function settle(Currency token) external payable returns (uint256 paid);

/// @notice Called by the user to pay what is owed. If the payment is more than the debt, the surplus is refunded
/// @param currency The currency to settle
/// @param to The address to refund the surplus to
function settleAndRefund(Currency currency, address to) external payable returns (uint256 paid, uint256 refund);

/// @notice move the delta from target to the msg.sender, only payment delta can be moved
/// @param currency The currency to settle
/// @param target The address whose delta will be updated
Expand Down
5 changes: 5 additions & 0 deletions test/vault/FakePoolManagerRouter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ contract FakePoolManagerRouter {
// settle ETH
vault.settle{value: 5 ether}(CurrencyLibrary.NATIVE);
vault.take(CurrencyLibrary.NATIVE, address(this), 5 ether);
} else if (data[0] == 0x18) {
// call this method via vault.lock(abi.encodePacked(hex"18", alice));
address to = address(uint160(uint256(bytes32(data[1:0x15]) >> 96)));
vault.settleAndRefund(poolKey.currency0, to);
vault.settleAndRefund(poolKey.currency1, to);
}

return "";
Expand Down
38 changes: 37 additions & 1 deletion test/vault/Vault.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,42 @@ contract VaultTest is Test, GasSnapshot {
vault.lock(hex"02");
}

function testSettleAndRefund_WithErc20Transfer() public {
address alice = makeAddr("alice");

// simulate someone transferred token to vault
currency0.transfer(address(vault), 10 ether);
assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(fakePoolManagerRouter)), 0 ether);
assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(alice)), 0 ether);

// settle and refund
vm.prank(address(fakePoolManagerRouter));
snapStart("VaultTest#testSettleAndRefund_WithErc20Transfer");
vault.lock(abi.encodePacked(hex"18", alice));
snapEnd();

// verify
assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(fakePoolManagerRouter)), 0 ether);
assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(alice)), 10 ether);
}

function testSettleAndRefund_WithoutErc20Transfer() public {
address alice = makeAddr("alice");

assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(fakePoolManagerRouter)), 0 ether);
assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(alice)), 0 ether);

// settleAndRefund works even if there's no excess currency
vm.prank(address(fakePoolManagerRouter));
snapStart("VaultTest#testSettleAndRefund_WithoutErc20Transfer");
vault.lock(abi.encodePacked(hex"18", alice));
snapEnd();

// verify
assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(fakePoolManagerRouter)), 0 ether);
assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(alice)), 0 ether);
}

function testNotCorrectPoolManager() public {
// router => vault.lock
// vault.lock => periphery.lockAcquired
Expand Down Expand Up @@ -208,7 +244,7 @@ contract VaultTest is Test, GasSnapshot {
vm.prank(address(fakePoolManagerRouter));
snapStart("VaultTest#lockSettledWhenAddLiquidity");
vault.lock(hex"02");
snapStart("end");
snapEnd();

assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(vault)), 10 ether);
assertEq(IERC20(Currency.unwrap(currency1)).balanceOf(address(vault)), 10 ether);
Expand Down
28 changes: 27 additions & 1 deletion test/vault/VaultInvariant.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ contract VaultPoolManager is Test {
enum ActionType {
Take,
Settle,
SettleAndRefund,
SettleFor,
Mint,
Burn
Expand Down Expand Up @@ -84,6 +85,21 @@ contract VaultPoolManager is Test {
vault.lock(abi.encode(Action(ActionType.Settle, uint128(amt0), uint128(amt1))));
}

/// @dev In settleAndRefund case, assume user add liquidity and paying to the vault
/// but theres another folk who minted extra token to the vault
function settleAndRefund(uint256 amt0, uint256 amt1, bool sendToVault) public {
amt0 = bound(amt0, 0, MAX_TOKEN_BALANCE - 1 ether);
amt1 = bound(amt1, 0, MAX_TOKEN_BALANCE - 1 ether);

// someone send some token directly to vault
if (sendToVault) token0.mint(address(vault), 1 ether);

// mint token to VaultPoolManager, so VaultPoolManager can pay to the vault
token0.mint(address(this), amt0);
token1.mint(address(this), amt1);
vault.lock(abi.encode(Action(ActionType.SettleAndRefund, uint128(amt0), uint128(amt1))));
}

/// @dev In settleFor case, assume user is paying for hook
function settleFor(uint256 amt0, uint256 amt1) public {
amt0 = bound(amt0, 0, MAX_TOKEN_BALANCE);
Expand Down Expand Up @@ -165,6 +181,15 @@ contract VaultPoolManager is Test {

vault.settle(currency0);
vault.settle(currency1);
} else if (action.actionType == ActionType.SettleAndRefund) {
BalanceDelta delta = toBalanceDelta(int128(action.amt0), int128(action.amt1));
vault.accountPoolBalanceDelta(poolKey, delta, address(this));

token0.transfer(address(vault), action.amt0);
token1.transfer(address(vault), action.amt1);

vault.settleAndRefund(currency0, address(this));
vault.settleAndRefund(currency1, address(this));
} else if (action.actionType == ActionType.SettleFor) {
// hook cash out the fee ahead
BalanceDelta delta = toBalanceDelta(int128(action.amt0), int128(action.amt1));
Expand Down Expand Up @@ -212,13 +237,14 @@ contract VaultInvariant is Test, GasSnapshot {
// Only call vaultPoolManager, otherwise all other contracts deployed in setUp will be called
targetContract(address(vaultPoolManager));

bytes4[] memory selectors = new bytes4[](6);
bytes4[] memory selectors = new bytes4[](7);
selectors[0] = VaultPoolManager.take.selector;
selectors[1] = VaultPoolManager.mint.selector;
selectors[2] = VaultPoolManager.settle.selector;
selectors[3] = VaultPoolManager.burn.selector;
selectors[4] = VaultPoolManager.settleFor.selector;
selectors[5] = VaultPoolManager.collectFee.selector;
selectors[6] = VaultPoolManager.settleAndRefund.selector;
targetSelector(FuzzSelector({addr: address(vaultPoolManager), selectors: selectors}));
}

Expand Down
Loading