Feature/withdrawals PoC #446
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Remove: - Aragon vault recovery - Insurance fund - EL rewards setter (use setProtocolContracts) - staking limit views Update docs about Eth2 and Ethereum 2.0
| /// @dev Credentials which allows the DAO to withdraw Ether on the 2.0 side | ||
| bytes32 internal constant WITHDRAWAL_CREDENTIALS_POSITION = keccak256("lido.Lido.withdrawalCredentials"); | ||
|
|
||
| /// @dev Amount of eth in deposit buffer to reserve for withdrawals | ||
| bytes32 internal constant WITHDRAWAL_RESERVE_POSITION = keccak256("lido.Lido.withdrawalReserve"); |
There was a problem hiding this comment.
BUFFERED_ETHER_WITHDRAWALS_RESERVE_POSITION?
There was a problem hiding this comment.
Imo, it's a bit overkill. Too long and still not really clear. Should figure out some concise abstraction here
There was a problem hiding this comment.
ok, let's find another one later
contracts/0.4.24/Lido.sol
Outdated
| @@ -458,59 +472,53 @@ contract Lido is ILido, StETH, AragonApp { | |||
| emit ELRewardsWithdrawalLimitSet(_limitPoints); | |||
| } | |||
|
|
|||
| function getBufferWithdrawalsReserve() public returns (uint256) { | |||
There was a problem hiding this comment.
getBufferedEtherWithdrawalsReserve?
contracts/0.4.24/Lido.sol
Outdated
|
|
||
| for (uint256 i = 0; i < _requestIdToFinalizeUpTo.length; i++) { | ||
| uint256 lastIdToFinalize = _requestIdToFinalizeUpTo[i]; | ||
| require(lastIdToFinalize >= withdrawal.finalizedRequestsCounter(), "BAD_FINALIZATION_PARAMS"); |
There was a problem hiding this comment.
For the future: should check the block of the request
|
|
||
| uint256 lockedEtherAccumulator = 0; | ||
|
|
||
| for (uint256 i = 0; i < _requestIdToFinalizeUpTo.length; i++) { |
There was a problem hiding this comment.
Looks like all storage-access ops are performed within a loop.
Maybe worth move out them (I know that currently every oracle report is supposed to invoke only a single iteration, though, may change in the future).
There was a problem hiding this comment.
Yep, it's naive implementation and subject to further optimisation.
There was a problem hiding this comment.
Rechecked it and didn't get which ops can be moved out exactly. Can you elaborate?
There was a problem hiding this comment.
Ok, the major consideration was about burnShares.
Other possible directions:
- require(lastIdToFinalize >= withdrawal.finalizedRequestsCounter(), "BAD_FINALIZATION_PARAMS") (don't call each iteration)
- withdrawal.calculateFinalizationParams (accept vector params)
- withdrawal.finalize.value(transferToWithdrawalBuffer) (accept vector params)
| /// The bitmask of the oracle members that pushed their reports | ||
| bytes32 internal constant REPORTS_BITMASK_POSITION = keccak256("lido.CommitteeQuorum.reportsBitmask"); | ||
|
|
||
| ///! STRUCTURED STORAGE OF THE CONTRACT |
There was a problem hiding this comment.
maybe go with unstructured since we use a contemporary solidity version
https://gist.github.com/skozin/344d9c7d2270cac0c3f9373c2f43c846
| } | ||
|
|
||
|
|
||
| function _handleMemberReport(address _reporter, bytes memory _report) |
There was a problem hiding this comment.
can simplify if forbid <50% participation quorum
| } | ||
|
|
||
|
|
||
| function _handleMemberReport(address _reporter, bytes memory _report) |
There was a problem hiding this comment.
maybe should emit an event
| return MEMBER_NOT_FOUND; | ||
| } | ||
|
|
||
| function _clearReporting() internal { |
There was a problem hiding this comment.
maybe should emit an event either
contracts/0.8.9/LidoOracleNew.sol
Outdated
| // decision | ||
| uint256 newDepositBufferWithdrawalsReserve; | ||
| uint256[] requestIdToFinalizeUpTo; | ||
| uint256[] finalizationPooledEtherAmount; |
There was a problem hiding this comment.
should be reduced to the single value (holding the ratio)
| ) | ||
| external | ||
| { | ||
| assert(1 == ((1 << (MAX_MEMBERS - 1)) >> (MAX_MEMBERS - 1))); // static assert |
There was a problem hiding this comment.
maybe rewrite this to be more readable?
| MemberReport calldata _report | ||
| ) external { | ||
| BeaconSpec memory beaconSpec = _getBeaconSpec(); | ||
| bool hasEpochAdvanced = _validateAndUpdateExpectedEpoch(_report.epochId, beaconSpec); |
There was a problem hiding this comment.
looks like the same member can advance the epoch preventing for reports finalization
maybe address this by setting cooldown time or checking the epoch number against block number / timestamp?
| } | ||
|
|
||
|
|
||
| function _doWorkAfterReportingToLido( |
There was a problem hiding this comment.
_handleStETHRebase
| internal | ||
| view | ||
| { | ||
| // TODO: update sanity checks |
| ); | ||
|
|
||
| event CommitteeMemberReported( | ||
| address[] stakingModules, |
There was a problem hiding this comment.
should evaluate data packing efficiency
| using LimitUnstructuredStorage for bytes32; | ||
|
|
||
| event ValidatorExitRequest( | ||
| address indexed stakingModule, |
There was a problem hiding this comment.
decided to go with module id, not address
|
Closing as it's merged into #482. To be continued in a new branch |
Hazardous areas, be careful