-
Notifications
You must be signed in to change notification settings - Fork 19
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.
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
Add PoolController to handle pool admin tasks #90
Add PoolController to handle pool admin tasks #90
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I left some small suggestions / ideas.
function onActivated() external onlyPoolController { | ||
IPoolConfigurableSettings memory _settings = settings(); | ||
|
||
activatedAt = block.timestamp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the lifecycle state lives in the controller now, I can see an argument for this timestamp living there too? No strong feelings though!
address(_firstLossVault) | ||
); | ||
require(address(_firstLossVault) != address(0), "Pool: 0 address"); | ||
_liquidityAsset.safeTransferFrom( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also live in the pool controls too right? The PA would have to ERC20 approve the controls contract instead, I guess, but don't see any difference otherwise?
require(address(_firstLossVault) != address(0), "Pool: 0 address"); | ||
require(receiver != address(0), "Pool: 0 address"); | ||
|
||
_firstLossVault.withdraw(amount, receiver); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, can see an argument for the controls to manage this internally (the vault would have to be passed the controls contract instead), but no strong feelings!
} | ||
|
||
/** | ||
* @inheritdoc IPool | ||
*/ | ||
function updatePoolEndDate(uint256 endDate) external onlyAdmin { | ||
PoolLib.executeUpdateEndDate(endDate, _poolSettings); | ||
function numFundedLoans() external view returns (uint256) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also expose _fundedLoans
as a public variable and remove this method...can see how that might be useful regardless for inspecting things
*/ | ||
modifier onlyAdmin() { | ||
require( | ||
admin != address(0) && msg.sender == admin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this always be != address(0) since its set in the constructor?
There's still a few more methods to move (I don't want any reference to Admin in the Pool itself), but this gets the process started by moving all
state
(lifeCycleState) methods, thesettings
, and the FirstLoss logic over to the PoolController.Still has to call back into the Pool to actually transfer from the FirstLoss vault, which I may change to keep it completely separate.