Skip to content

Allows deposit size to scale with minipool queue space #251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Valdorff wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Allows deposit size to scale with minipool queue space #251

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46b1a46
Allows deposit size to scale with minipool queue space in addition to…
Jul 23, 2022
3e3d594
Remove unused functions
Jul 27, 2022
27ee57a
Address comments from Kane
Valdorff Aug 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 50 additions & 13 deletions contracts/contract/deposit/RocketDepositPool.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,20 @@ contract RocketDepositPool is RocketBase, RocketDepositPoolInterface, RocketVaul
require(rocketDAOProtocolSettingsDeposit.getDepositEnabled(), "Deposits into Rocket Pool are currently disabled");
require(msg.value >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit(), "The deposited amount is less than the minimum deposit size");
RocketVaultInterface rocketVault = RocketVaultInterface(getContractAddress("rocketVault"));
require(rocketVault.balanceOf("rocketDepositPool").add(msg.value) <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize(), "The deposit pool size after depositing exceeds the maximum size");
uint256 capacityNeeded = rocketVault.balanceOf("rocketDepositPool").add(msg.value);
if (capacityNeeded > rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize()) {
// Doing a conditional require() instead of a single one optimizes for the common
// 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(),
Copy link
Member

@kanewallmann kanewallmann Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capacity should be capacityNeeded

"The deposit pool size after depositing (and matching with minipools) exceeds the maximum size");
} else {
require(capacity <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize(),
Copy link
Member

@kanewallmann kanewallmann Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can replace this require with a revert because this condition is already tested above.

"The deposit pool size after depositing exceeds the maximum size");
}
}

// Record last deposit to time delay ability to withdraw
setUint(keccak256(abi.encodePacked("user.deposit.block", msg.sender)), block.number);
// Mint rETH to user account
Expand Down Expand Up @@ -138,6 +151,7 @@ contract RocketDepositPool is RocketBase, RocketDepositPoolInterface, RocketVaul
}

// Assigns deposits to available minipools, returns false if assignment is currently disabled
// Can assign deposits up to the value of the deposit plus getMaximumDepositAssignments()
function _assignDeposits(RocketVaultInterface rocketVault, RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit) private returns (bool) {
// Check if assigning deposits is enabled
if (!rocketDAOProtocolSettingsDeposit.getAssignDepositsEnabled()) {
Expand All @@ -150,28 +164,51 @@ contract RocketDepositPool is RocketBase, RocketDepositPoolInterface, RocketVaul
uint256 balance = rocketVault.balanceOf("rocketDepositPool");
uint256 totalEther = 0;
// Calculate minipool assignments
uint256 maxAssignments = rocketDAOProtocolSettingsDeposit.getMaximumDepositAssignments();
MinipoolAssignment[] memory assignments = new MinipoolAssignment[](maxAssignments);
MinipoolDeposit depositType = MinipoolDeposit.None;

uint256 count = 0;
uint256 minipoolCapacity = 0;
for (uint256 i = 0; i < maxAssignments; ++i) {
// Optimised for multiple of the same deposit type
if (count == 0) {
Copy link
Contributor Author

@Valdorff Valdorff Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

(depositType, count) = rocketMinipoolQueue.getNextDeposit();
if (depositType == MinipoolDeposit.None) { break; }
minipoolCapacity = rocketDAOProtocolSettingsMinipool.getDepositUserAmount(depositType);
uint256 depositValueForAssignments = msg.value;
uint256 socializedAssignments = rocketDAOProtocolSettingsDeposit.getMaximumDepositAssignments();
MinipoolAssignment[] memory assignments = new MinipoolAssignment[](maxAssignments);

// Prepare half deposit assignments
count = rocketMinipoolQueue.getLength(MinipoolDeposit.Half);
Copy link
Contributor Author

@Valdorff Valdorff Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we could iterate across the deposit type, we wouldn't need to copy-paste the code for half/full

minipoolCapacity = rocketDAOProtocolSettingsMinipool.getDepositUserAmount(MinipoolDeposit.Half);
for (uint256 i=0; i < count; ++i) { // (see note in full deposit loop)
if (depositValueForAssignments < minipoolCapacity) {
if (socializedAssignments == 0) { break; }
else {socializedAssignments--;}
} else {
depositValueForAssignments.sub(minipoolCapacity);
}
count--;
if (minipoolCapacity == 0 || balance.sub(totalEther) < minipoolCapacity) { break; }
if (balance.sub(totalEther) < minipoolCapacity) { break; }
// Dequeue the minipool
address minipoolAddress = rocketMinipoolQueue.dequeueMinipoolByDeposit(depositType);
address minipoolAddress = rocketMinipoolQueue.dequeueMinipoolByDeposit(MinipoolDeposit.Half);
// Update running total
totalEther = totalEther.add(minipoolCapacity);
// Add assignment
assignments[i].etherAssigned = minipoolCapacity;
assignments[i].minipoolAddress = minipoolAddress;
}

// 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
Copy link
Contributor Author

@Valdorff Valdorff Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

if (depositValueForAssignments < minipoolCapacity) {
if (socializedAssignments == 0) { break; }
else {socializedAssignments--;}
}
if (balance.sub(totalEther) < minipoolCapacity) { break; }
// Dequeue the minipool
address minipoolAddress = rocketMinipoolQueue.dequeueMinipoolByDeposit(MinipoolDeposit.Full);
// Update running total
totalEther = totalEther.add(minipoolCapacity);
// Add assignment
assignments[i].etherAssigned = minipoolCapacity;
assignments[i].minipoolAddress = minipoolAddress;
}

if (totalEther > 0) {
// Withdraw ETH from vault
rocketVault.withdrawEther(totalEther);
Expand Down
36 changes: 2 additions & 34 deletions contracts/contract/minipool/RocketMinipoolQueue.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,18 +57,6 @@ contract RocketMinipoolQueue is RocketBase, RocketMinipoolQueueInterface {
return addressQueueStorage.getLength(_key);
}

// Get the total combined capacity of the queues
function getTotalCapacity() override external view returns (uint256) {
RocketDAOProtocolSettingsMinipoolInterface rocketDAOProtocolSettingsMinipool = RocketDAOProtocolSettingsMinipoolInterface(getContractAddress("rocketDAOProtocolSettingsMinipool"));
return (
getLength(queueKeyFull).mul(rocketDAOProtocolSettingsMinipool.getFullDepositUserAmount())
).add(
getLength(queueKeyHalf).mul(rocketDAOProtocolSettingsMinipool.getHalfDepositUserAmount())
).add(
getLength(queueKeyEmpty).mul(rocketDAOProtocolSettingsMinipool.getEmptyDepositUserAmount())
);
}

// Get the total effective capacity of the queues (used in node demand calculation)
function getEffectiveCapacity() override external view returns (uint256) {
RocketDAOProtocolSettingsMinipoolInterface rocketDAOProtocolSettingsMinipool = RocketDAOProtocolSettingsMinipoolInterface(getContractAddress("rocketDAOProtocolSettingsMinipool"));
Expand All @@ -79,28 +67,6 @@ contract RocketMinipoolQueue is RocketBase, RocketMinipoolQueueInterface {
);
}

// Get the capacity of the next available minipool
// Returns 0 if no minipools are available
function getNextCapacity() override external view returns (uint256) {
RocketDAOProtocolSettingsMinipoolInterface rocketDAOProtocolSettingsMinipool = RocketDAOProtocolSettingsMinipoolInterface(getContractAddress("rocketDAOProtocolSettingsMinipool"));
if (getLength(queueKeyHalf) > 0) { return rocketDAOProtocolSettingsMinipool.getHalfDepositUserAmount(); }
if (getLength(queueKeyFull) > 0) { return rocketDAOProtocolSettingsMinipool.getFullDepositUserAmount(); }
if (getLength(queueKeyEmpty) > 0) { return rocketDAOProtocolSettingsMinipool.getEmptyDepositUserAmount(); }
return 0;
}

// Get the deposit type of the next available minipool and the number of deposits in that queue
// Returns None if no minipools are available
function getNextDeposit() override external view returns (MinipoolDeposit, uint256) {
uint256 length = getLength(queueKeyHalf);
if (length > 0) { return (MinipoolDeposit.Half, length); }
length = getLength(queueKeyFull);
if (length > 0) { return (MinipoolDeposit.Full, length); }
length = getLength(queueKeyEmpty);
if (length > 0) { return (MinipoolDeposit.Empty, length); }
return (MinipoolDeposit.None, 0);
}

// Add a minipool to the end of the appropriate queue
// Only accepts calls from the RocketMinipoolManager contract
function enqueueMinipool(MinipoolDeposit _depositType, address _minipool) override external onlyLatestContract("rocketMinipoolQueue", address(this)) onlyLatestContract("rocketMinipoolManager", msg.sender) {
Expand Down Expand Up @@ -143,6 +109,8 @@ contract RocketMinipoolQueue is RocketBase, RocketMinipoolQueueInterface {

// Remove a minipool from a queue
// Only accepts calls from registered minipools
// Note: this removal is made computationally efficient by swapping with the last item in the
// queue. This is acceptable because removing minipools should be rare.
function removeMinipool(MinipoolDeposit _depositType) override external onlyLatestContract("rocketMinipoolQueue", address(this)) onlyRegisteredMinipool(msg.sender) {
// Remove minipool from queue
if (_depositType == MinipoolDeposit.Half) { return removeMinipool(queueKeyHalf, msg.sender); }
Expand Down
3 changes: 0 additions & 3 deletions contracts/interface/minipool/RocketMinipoolQueueInterface.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,7 @@ import "../../types/MinipoolDeposit.sol";
interface RocketMinipoolQueueInterface {
function getTotalLength() external view returns (uint256);
function getLength(MinipoolDeposit _depositType) external view returns (uint256);
function getTotalCapacity() external view returns (uint256);
function getEffectiveCapacity() external view returns (uint256);
function getNextCapacity() external view returns (uint256);
function getNextDeposit() external view returns (MinipoolDeposit, uint256);
function enqueueMinipool(MinipoolDeposit _depositType, address _minipool) external;
function dequeueMinipool() external returns (address minipoolAddress);
function dequeueMinipoolByDeposit(MinipoolDeposit _depositType) external returns (address minipoolAddress);
Expand Down
6 changes: 2 additions & 4 deletions test/deposit/scenario-assign-deposits.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,9 @@ export async function assignDeposits(txOptions) {
function getMinipoolQueueDetails() {
return Promise.all([
rocketMinipoolQueue.getTotalLength.call(),
rocketMinipoolQueue.getTotalCapacity.call(),
]).then(
([totalLength, totalCapacity]) =>
({totalLength, totalCapacity})
([totalLength]) =>
({totalLength})
);
}

Expand All @@ -94,7 +93,6 @@ export async function assignDeposits(txOptions) {

// Check minipool queues
assert(queue2.totalLength.eq(queue1.totalLength.sub(web3.utils.toBN(expectedDepositAssignments))), 'Incorrect updated minipool queue length');
assert(queue2.totalCapacity.eq(queue1.totalCapacity.sub(expectedEthAssigned)), 'Incorrect updated minipool queue capacity');

}