feat(contracts): remove withdrawal threshold and add withdrawToL2 #232
Conversation
| function withdrawToL2() external virtual { | ||
| uint256 amount = _processWithdrawal(); | ||
|
|
||
| bool success = SafeCall.call(msg.sender, gasleft(), amount, bytes("")); |
There was a problem hiding this comment.
This is just a curiosity. It doesn't seem to require any modification.
Can't we just use call instead of safecall?
It seems like it would be cheaper on gas, even though regular call is marginal.
I'm just wondering if it's more codewise meaningful or what your intentions are.
| bool success = SafeCall.call(msg.sender, gasleft(), amount, bytes("")); | |
| (bool success,) = RECIPIENT.call{ value: amount }(hex""); |
There was a problem hiding this comment.
I'm not sure, but assembly call is better in that it does not copy return data as written in SafeCall.sol.
Maybe @Pangssu can answer this?
There was a problem hiding this comment.
In this project, transfer is also using SafeCall, so it makes sense to unify them.
| @@ -134,7 +134,7 @@ contract ValidatorRewardVault is FeeVault, Semver { | |||
| * @notice Withdraws all of the sender's balance to L2. | |||
| * Reverts if the balance is less than the minimum withdrawal amount. | |||
| */ | |||
| function withdrawToL2() external { | |||
| function withdrawToL2() external override { | |||
There was a problem hiding this comment.
Could you change the function name processWithdrawal to _processWithdrawal because it's internal function?
There was a problem hiding this comment.
Well,, we don't use underscore in the internal function name, only private function.
There was a problem hiding this comment.
It seems that underscore is used in internal function in general. So i'll keep this way.
0xHansLee
force-pushed
the
feat/remote-vault-threshold
branch
from
a1fd885 to
3229930
Compare
This is based on v1.0.2.
2 changes have been made:
I made
withdrawToL2as virtual function to make compatible with the existingValidatorRewardVaultcontract.Let's keep in mind that after merging, the commits in this PR should be cherry-picked to v1.1.0 as well.