Skip to content
50 changes: 49 additions & 1 deletion contracts/lbp/LBPManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ import "../utils/interface/ILBP.sol";
*/
// solhint-disable-next-line max-states-count
contract LBPManager {
enum TaskState {
Waiting,
Started,
Ended
}
TaskState public state;
// Constants
uint256 private constant HUNDRED_PERCENT = 1e18; // Used in calculating the fee.

Expand Down Expand Up @@ -192,7 +198,7 @@ contract LBPManager {
startWeights,
swapFeePercentage,
address(this),
true // SwapEnabled is set to true at pool creation.
false // SwapEnabled is set to true at pool creation.
)
);

Expand Down Expand Up @@ -234,6 +240,47 @@ contract LBPManager {
vault.joinPool(lbp.getPoolId(), address(this), address(this), request);
}

/**
* @dev start LBP just before one block
* @notice it can be invoked by anyone only once
*/
function startLbp() external {
uint256 startTime;
uint256 blockTime = 5 minutes;
bool isSwapEnabled = lbp.getSwapEnabled();
(startTime, , ) = lbp.getGradualWeightUpdateParams();
require(
Copy link

Choose a reason for hiding this comment

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

image

!isSwapEnabled && state == TaskState.Waiting,
"LBPManager: swapping already active"
Copy link

Choose a reason for hiding this comment

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

if state is Ended, message would be wrong

Copy link

Choose a reason for hiding this comment

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

Should refer to "enabled" OR "disabled", not "active" (state terminology is internal to the contract)

Copy link

Choose a reason for hiding this comment

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

I don't think we should revert if !isSwapEnabled. Should just set the state to Started and return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have implemented the changes you requested.

);
require(
block.timestamp > startTime - blockTime,
"LBPManager: Only once block before start time"
Copy link

Choose a reason for hiding this comment

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

incorrect grammar and unclear. should be "LBPManager: More than one block before start time"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

improved the error string

);
state = TaskState.Started;
lbp.setSwapEnabled(true);
}

/**
* @dev ends LBP just after one block
* @notice it can be invoked by anyone only once
*/
function endLbp() external {
uint256 endTime;
bool isSwapEnabled = lbp.getSwapEnabled();
(, endTime, ) = lbp.getGradualWeightUpdateParams();
require(
isSwapEnabled && state == TaskState.Started,
"LBPManager: swapping already active"
Copy link

Choose a reason for hiding this comment

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

Should be "LBPManager: swapping is not enabled"

Copy link

Choose a reason for hiding this comment

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

anyway, I don't think we should revert if isSwapEnabled. Should just set the state to Ended and return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have implemented the changes you requested.

);
require(
block.timestamp > endTime,
"LBPManager: Only once block before start time"
Copy link

Choose a reason for hiding this comment

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

should be >=

incorrect grammar and unclear. should be "LBPManager: haven't reached endTime"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have implemented the changes you requested.

);
state = TaskState.Ended;
lbp.setSwapEnabled(false);
}

/**
* @dev Exit pool or remove liquidity from pool.
Copy link

Choose a reason for hiding this comment

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

This documentation is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

documentation for removeLiquidity?

Copy link

Choose a reason for hiding this comment

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

The documentation on the line linked to this comment. "Exit pool or remove liquidity from pool."

Copy link

@dkent600 dkent600 Jan 6, 2022

Choose a reason for hiding this comment

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

That documentation is completely wrong for setSwapEnabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, This documentation is for removeLiquidity. The lines between 286-349 are hidden due to no changes. The correct docs for setSwapEnabled starts at line 347.

* @param _receiver Address of the liquidity receiver, after exiting the LBP.
Expand Down Expand Up @@ -302,6 +349,7 @@ contract LBPManager {
* @param _swapEnabled Enables/disables swapping.
*/
function setSwapEnabled(bool _swapEnabled) external onlyAdmin {
require(TaskState.Ended != state, "LBPManager: LBP ended");
Copy link

Choose a reason for hiding this comment

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

Why? you can only call this function if the LBP is ended? For what purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can call this method only if the LBP is not ended. So that nobody can restart swapping after end time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved the condition, and is now not dependent on state.

lbp.setSwapEnabled(_swapEnabled);
}

Expand Down