build: deposit limit #17
Conversation
| @@ -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"); | |||
There was a problem hiding this comment.
I personally like to use string error codes when using requires (e.g. "DEPOSIT_LIMIT") - or custom errors with reverts.
There was a problem hiding this comment.
not sure i follow.
Are you saying just change the revert string to "DEPOSIT_LIMIT" instead of "deposit limit" ?
There was a problem hiding this comment.
yeah, but it's minor
I find it easier to integrate with frontends etc
src/L1YearnEscrow.sol
Outdated
| * @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"); |
There was a problem hiding this comment.
why not change the parameter's type to uint128?
There was a problem hiding this comment.
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
| ); | ||
| mockEscrow = deployMockL1Escrow(); | ||
|
|
||
| vm.expectRevert(); |
There was a problem hiding this comment.
some comments would be helpful :)
// reverts because caller does is not allowed to change the deposit limit
| @@ -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); | |||
There was a problem hiding this comment.
yes, will add a section to show that
There was a problem hiding this comment.
Test using a 0 deposit limit is here https://github.com/yearn/yearn-stb/pull/17/files#diff-52b3b649a24d8bf5f33c53bf9546c7aee0a6133a5503d8525a1a1e5dab36f656R170
No description provided.