feat: add getSharesFromQueuedWithdrawal
#1078
Conversation
0xClandestine
changed the title
getSharesFromQueuedWithdrawal
| withdrawal = queuedWithdrawals[withdrawalRoot]; | ||
| shares = new uint256[](withdrawal.strategies.length); | ||
|
|
||
| address operator = delegatedTo[withdrawal.staker]; | ||
| uint32 slashableUntil = withdrawal.startBlock + MIN_WITHDRAWAL_DELAY_BLOCKS; | ||
|
|
||
| uint256[] memory slashingFactors; | ||
| // If slashableUntil block is in the past, read the slashing factors at that block | ||
| // Otherwise read the current slashing factors. Note that if the slashableUntil block is the current block | ||
| // or in the future then the slashing factors are still subject to change before the withdrawal is completable | ||
| // and the shares withdrawn to be less | ||
| if (slashableUntil < uint32(block.number)) { | ||
| slashingFactors = _getSlashingFactorsAtBlock({ | ||
| staker: withdrawal.staker, | ||
| operator: operator, | ||
| strategies: withdrawal.strategies, | ||
| blockNumber: slashableUntil | ||
| }); | ||
| } else { | ||
| slashingFactors = | ||
| _getSlashingFactors({staker: withdrawal.staker, operator: operator, strategies: withdrawal.strategies}); | ||
| } | ||
|
|
||
| for (uint256 j; j < withdrawal.strategies.length; ++j) { | ||
| shares[j] = SlashingLib.scaleForCompleteWithdrawal({ | ||
| scaledShares: withdrawal.scaledShares[j], | ||
| slashingFactor: slashingFactors[j] | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
my concern is that this view function is essentially doing some heavy computing on the fly, which is
1/ really inefficient, eg consecutive read with same root would require it to compute again and again
2/ it's essentially not "view" function anymore. view function IMHO should be an read only operation that expose an already existance state, to make it vieweable by others. But in this case, the state to be read does not exist at all and have to be computed on the fly, so it's more like a write operation
Whereas a more proper way is to precompute everything and the view function only does a quick lookup read
is there a way to make it the 2nd pattern?
There was a problem hiding this comment.
Are you suggesting tracking this in storage?
There was a problem hiding this comment.
Ya, a bit confused on the ask here @bowenli86? This value can't be stored because its dynamic (slashing while in the queue)
There was a problem hiding this comment.
Remember, not all view functions need to be called on-chain, some are designed to be called off-chain too, where the gas limit is not the same
There was a problem hiding this comment.
yea the concern is this seems not to be a view function for either onchain or offchain, b/c it's doing a bunch of complex logic rather than just get sth out of shelf.
but i see your point of it being dynamic, especially it's expensive to update the value every block
not sure what's the best design pattern for such cases.
not a blocker
| function _getSharesFromQueuedWithdrawal( | ||
| bytes32 withdrawalRoot | ||
| ) internal view returns (Withdrawal memory withdrawal, uint256[] memory shares) { | ||
| withdrawal = queuedWithdrawals[withdrawalRoot]; |
There was a problem hiding this comment.
what happens if the withdrawal is already completed or does not exist in queue?
| withdrawal = queuedWithdrawals[withdrawalRoot]; | ||
| shares = new uint256[](withdrawal.strategies.length); | ||
|
|
||
| address operator = delegatedTo[withdrawal.staker]; | ||
| uint32 slashableUntil = withdrawal.startBlock + MIN_WITHDRAWAL_DELAY_BLOCKS; | ||
|
|
||
| uint256[] memory slashingFactors; | ||
| // If slashableUntil block is in the past, read the slashing factors at that block | ||
| // Otherwise read the current slashing factors. Note that if the slashableUntil block is the current block | ||
| // or in the future then the slashing factors are still subject to change before the withdrawal is completable | ||
| // and the shares withdrawn to be less | ||
| if (slashableUntil < uint32(block.number)) { | ||
| slashingFactors = _getSlashingFactorsAtBlock({ | ||
| staker: withdrawal.staker, | ||
| operator: operator, | ||
| strategies: withdrawal.strategies, | ||
| blockNumber: slashableUntil | ||
| }); | ||
| } else { | ||
| slashingFactors = | ||
| _getSlashingFactors({staker: withdrawal.staker, operator: operator, strategies: withdrawal.strategies}); | ||
| } | ||
|
|
||
| for (uint256 j; j < withdrawal.strategies.length; ++j) { | ||
| shares[j] = SlashingLib.scaleForCompleteWithdrawal({ | ||
| scaledShares: withdrawal.scaledShares[j], | ||
| slashingFactor: slashingFactors[j] | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
yea the concern is this seems not to be a view function for either onchain or offchain, b/c it's doing a bunch of complex logic rather than just get sth out of shelf.
but i see your point of it being dynamic, especially it's expensive to update the value every block
not sure what's the best design pattern for such cases.
not a blocker
Motivation:
Users want a cost effective way to query withdrawable shares.
Modifications:
getSharesFromQueuedWithdrawal(bytes32 withdrawalRoot)getQueuedWithdrawalstogetSharesFromQueuedWithdrawals.Result:
Users can query withdrawable shares with
withdrawalRootalone.