Allows deposit size to scale with minipool queue space #251
Conversation
… deposit pool space - Also allows assignment to scale with value of deposit
| MinipoolAssignment[] memory assignments = new MinipoolAssignment[](maxAssignments); | ||
|
|
||
| // Prepare half deposit assignments | ||
| count = rocketMinipoolQueue.getLength(MinipoolDeposit.Half); |
There was a problem hiding this comment.
If we could iterate across the deposit type, we wouldn't need to copy-paste the code for half/full
| // Prepare full deposit assignments | ||
| count = rocketMinipoolQueue.getLength(MinipoolDeposit.Full); | ||
| minipoolCapacity = rocketDAOProtocolSettingsMinipool.getDepositUserAmount(MinipoolDeposit.Full); | ||
| for (i; i < i + count; ++i) { // NOTE - this is a weird line - we continue the indexing from the half deposit loop |
There was a problem hiding this comment.
Wasn't sure if this weirdness was worth it. The alternative would be to have an assignment index variable and track/increment that across both loops but let i reset. The current way saves a variable 🤷
| uint256 count = 0; | ||
| uint256 minipoolCapacity = 0; | ||
| for (uint256 i = 0; i < maxAssignments; ++i) { | ||
| // Optimised for multiple of the same deposit type | ||
| if (count == 0) { |
There was a problem hiding this comment.
It seems there's no way to handle running out of one deposit type and moving on to the next one?
We only getNextDeposit() on the first loop, so then we're stuck with that deposit type.
If we run out of that queue, I'm not really sure what happens when dequeueMinipoolByDeposit fails because there's nothing more in the queue (the require in that function fails, but I'm not sure where that ends up putting us cuz I'm a solidity newbie).
| require(capacity <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() + rocketMinipoolQueue.getEffectiveCapacity(), | ||
| "The deposit pool size after depositing (and matching with minipools) exceeds the maximum size"); | ||
| } else { | ||
| require(capacity <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize(), |
There was a problem hiding this comment.
Can replace this require with a revert because this condition is already tested above.
| // case where capacityNeeded fits in the deposit pool without looking at the queue | ||
| if (rocketDAOProtocolSettingsDeposit.getAssignDepositsEnabled()) { | ||
| RocketMinipoolQueueInterface rocketMinipoolQueue = RocketMinipoolQueueInterface(getContractAddress("rocketMinipoolQueue")); | ||
| require(capacity <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() + rocketMinipoolQueue.getEffectiveCapacity(), |
There was a problem hiding this comment.
capacity should be capacityNeeded
- 2 comments in PR - Need for a hard max on assignments to ensure there's enough gas in a block - Unrelated; did away with the weird counter-spanning-for-loops trick to be more explicit
Supports option 2 that @darrenlangley mentioned in https://dao.rocketpool.net/t/proposal-to-increase-the-deposit-pool-dp-limit/817/19?u=valdorff