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.

Sign up for GitHub

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

build: deposit limit #17

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

build: deposit limit #17

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15e78c8
build: deposit limit
Schlagonia Aug 20, 2024
dc7488e
test: deposit limit
Schlagonia Aug 21, 2024
db53977
fix: uint 128
Schlagonia Aug 29, 2024
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
39 changes: 36 additions & 3 deletions src/L1YearnEscrow.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ contract L1YearnEscrow is L1Escrow {
*/
event UpdateMinimumBuffer(uint256 newMinimumBuffer);

/**
* @dev Emitted when the deposit limit is updated.
*/
event UpdateDepositLimit(uint256 newDepositLimit);

// ****************************
// * ERC-7201 Storage *
// **************************
Expand All @@ -38,7 +43,8 @@ contract L1YearnEscrow is L1Escrow {
struct VaultStorage {
IVault vaultAddress;
uint256 deposited;
uint256 minimumBuffer;
uint128 minimumBuffer;
uint128 depositLimit;
}

// keccak256(abi.encode(uint256(keccak256("yearn.storage.vault")) - 1)) & ~bytes32(uint256(0xff))
Expand Down Expand Up @@ -66,6 +72,11 @@ contract L1YearnEscrow is L1Escrow {
return $.minimumBuffer;
}

function depositLimit() public view returns (uint256) {
VaultStorage storage $ = _getVaultStorage();
return $.depositLimit;
}

// ****************************
// * Initializer *
// ****************************
Expand Down Expand Up @@ -107,6 +118,9 @@ contract L1YearnEscrow is L1Escrow {
// Set the vault variable
VaultStorage storage $ = _getVaultStorage();
$.vaultAddress = IVault(_vaultAddress);

// Default to no deposit limit.
$.depositLimit = type(uint128).max;
}

// ****************************
Expand All @@ -120,10 +134,12 @@ contract L1YearnEscrow is L1Escrow {
function _receiveTokens(
uint256 amount
) internal virtual override whenNotPaused {
VaultStorage storage $ = _getVaultStorage();
require($.deposited + amount <= $.depositLimit, "deposit limit");
Copy link

Choose a reason for hiding this comment

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

I personally like to use string error codes when using requires (e.g. "DEPOSIT_LIMIT") - or custom errors with reverts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure i follow.

Are you saying just change the revert string to "DEPOSIT_LIMIT" instead of "deposit limit" ?

Copy link

Choose a reason for hiding this comment

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

yeah, but it's minor

I find it easier to integrate with frontends etc


IERC20 originToken = originTokenAddress();
originToken.safeTransferFrom(msg.sender, address(this), amount);

VaultStorage storage $ = _getVaultStorage();
unchecked {
$.deposited += amount;
}
Expand Down Expand Up @@ -281,17 +297,34 @@ contract L1YearnEscrow is L1Escrow {

/**
* @dev Update the minimum buffer to keep in the escrow.
* uint128 max would be the max buffer.
* @param _minimumBuffer The new minimum buffer to enforce.
*/
function updateMinimumBuffer(
uint256 _minimumBuffer
) external virtual onlyRole(DEFAULT_ADMIN_ROLE) {
VaultStorage storage $ = _getVaultStorage();
$.minimumBuffer = _minimumBuffer;
require(_minimumBuffer <= type(uint128).max, "max size");
Copy link

Choose a reason for hiding this comment

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

why not change the parameter's type to uint128?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tend to prefer keeping the external types the generic versions to make integrations and interfaces easier.

But can change it to 128 to make is clearer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

db53977

$.minimumBuffer = uint128(_minimumBuffer);

emit UpdateMinimumBuffer(_minimumBuffer);
}

/**
* @dev Update the deposit limit to use for the escrow.
* uint128 is the max and means no deposit limit.
* @param _depositLimit The new deposit limit to enforce.
*/
function updateDepositLimit(
uint256 _depositLimit
) external virtual onlyRole(DEFAULT_ADMIN_ROLE) {
VaultStorage storage $ = _getVaultStorage();
require(_depositLimit <= type(uint128).max, "max size");
$.depositLimit = uint128(_depositLimit);

emit UpdateDepositLimit(_depositLimit);
}

/**
* @notice Rebalance the funds to support the minimum buffer.
* @dev Will revert if the difference is over the maxDeposit.
Expand Down
87 changes: 86 additions & 1 deletion test/L1Escrow.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,92 @@ contract EscrowTest is Setup {
assertEq(vault.balanceOf(address(mockEscrow)), 0);
}

function test_bridgeAsset_maxDepositLimit(uint256 _amount) public {
function test_bridgeAsset_escrowDepositLimit(uint256 _amount) public {
_amount = bound(_amount, minFuzzAmount, maxFuzzAmount);
Copy link

Choose a reason for hiding this comment

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

can _amount be 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, will add a section to show that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Test using a 0 deposit limit is here https://github.com/yearn/yearn-stb/pull/17/files#diff-52b3b649a24d8bf5f33c53bf9546c7aee0a6133a5503d8525a1a1e5dab36f656R170

address counterPart = l1Deployer.getL2EscrowAddress(
l2RollupID,
address(asset)
);
mockEscrow = deployMockL1Escrow();

vm.expectRevert();
Copy link

Choose a reason for hiding this comment

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

some comments would be helpful :)

// reverts because caller does is not allowed to change the deposit limit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

db53977

mockEscrow.updateDepositLimit(0);

vm.prank(governator);
mockEscrow.updateDepositLimit(0);

// Simulate a bridge txn
airdrop(asset, user, _amount);

vm.prank(user);
asset.approve(address(mockEscrow), _amount);

vm.expectRevert("deposit limit");
vm.prank(user);
mockEscrow.bridgeToken(user, _amount, true);

vm.prank(governator);
mockEscrow.updateDepositLimit(_amount);

bytes memory data = abi.encode(user, _amount);
uint256 depositCount = polygonZkEVMBridge.depositCount();
vm.expectEmit(true, true, true, true, address(polygonZkEVMBridge));
emit BridgeEvent(
1,
l1RollupID,
address(mockEscrow),
l2RollupID,
counterPart,
0,
data,
uint32(depositCount)
);
vm.prank(user);
mockEscrow.bridgeToken(user, _amount, true);

assertEq(vault.totalAssets(), _amount);
assertEq(mockEscrow.deposited(), _amount);
assertEq(asset.balanceOf(user), 0);
assertEq(asset.balanceOf(address(mockEscrow)), 0);
assertEq(vault.balanceOf(address(mockEscrow)), _amount);

airdrop(asset, user, _amount);

vm.prank(user);
asset.approve(address(mockEscrow), _amount);

vm.expectRevert("deposit limit");
vm.prank(user);
mockEscrow.bridgeToken(user, _amount, true);

vm.prank(governator);
mockEscrow.updateDepositLimit(_amount * 2);

vm.prank(user);
mockEscrow.bridgeToken(user, _amount, true);

assertEq(vault.totalAssets(), _amount * 2);
assertEq(mockEscrow.deposited(), _amount * 2);
assertEq(asset.balanceOf(user), 0);
assertEq(asset.balanceOf(address(mockEscrow)), 0);
assertEq(vault.balanceOf(address(mockEscrow)), _amount * 2);

// Withdraw half
uint256 toWithdraw = _amount + 10;

data = abi.encode(user, toWithdraw);

vm.prank(address(polygonZkEVMBridge));
mockEscrow.onMessageReceived(counterPart, l2RollupID, data);

assertEq(vault.totalAssets(), _amount - 10);
assertEq(mockEscrow.deposited(), _amount * 2 - toWithdraw);
assertEq(asset.balanceOf(user), toWithdraw);
assertEq(asset.balanceOf(address(mockEscrow)), 0);
assertEq(vault.balanceOf(address(mockEscrow)), _amount - 10);
}

function test_bridgeAsset_vaultDepositLimit(uint256 _amount) public {
_amount = bound(_amount, minFuzzAmount, maxFuzzAmount);
address counterPart = l1Deployer.getL2EscrowAddress(
l2RollupID,
Expand Down
Loading