-
Notifications
You must be signed in to change notification settings - Fork 43
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
Balancer pause helper #1211
base: main
Are you sure you want to change the base?
Balancer pause helper #1211
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.
First pass; didn't look at the tests yet.
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
…alancer-v3-monorepo into balancer-pause-helper
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.
Shaping up; some naming and style suggestions. Some comments from earlier might have gotten lost.
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
…alancer-v3-monorepo into balancer-pause-helper
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.
LGTM!
import { IVaultAdmin } from "@balancer-labs/v3-interfaces/contracts/vault/IVaultAdmin.sol"; | ||
import { SingletonAuthentication } from "@balancer-labs/v3-vault/contracts/SingletonAuthentication.sol"; | ||
|
||
contract PauseHelper is SingletonAuthentication { |
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.
Should we have an IPauseHelper, that defines all errors, events, and document all functions? Just to make this file cleaner.
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.
In the same vein, is this confusable with Vault pausing? (i.e., will people think you can also pause the Vault with this?) Maybe it should be called PausePoolHelper
.
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.
Agree it would be good to also define an interface; we usually do for these kinds of contracts. Also had a note on potentially renaming to PausePoolHelper, and some function/comment suggestions
} | ||
|
||
/*************************************************************************** | ||
Manage Pools |
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.
Manage Pools | |
Manage Pools |
ASCII art :)
} | ||
|
||
/** | ||
* @notice Get a range of pools. |
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.
* @notice Get a range of pools. | |
* @notice Get a range of pools. | |
* @dev Indexes are 0-based and [start, end) (i.e., inclusive of `start`; exclusive of `end`). |
Still think we should clarify this.
* @param to End index | ||
* @return pools List of pools | ||
*/ | ||
function getPools(uint256 from, uint256 to) public view returns (address[] memory pools) { |
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.
Do we also want a getPoolAt(uint256 index)
?
Could also have a getPools() returns (address[] memory pools)
that just returns them all.
That way it supports 3 ways of using it:
- simple
getPools()
if you know there isn't a pagination issue; - generic iteration: for(i = 0; i < getPoolsCount(); ++i) { address pool = getPoolAt(i); }
- pagination if needed, using getPools(from, to);
import { IVaultAdmin } from "@balancer-labs/v3-interfaces/contracts/vault/IVaultAdmin.sol"; | ||
import { SingletonAuthentication } from "@balancer-labs/v3-vault/contracts/SingletonAuthentication.sol"; | ||
|
||
contract PauseHelper is SingletonAuthentication { |
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.
In the same vein, is this confusable with Vault pausing? (i.e., will people think you can also pause the Vault with this?) Maybe it should be called PausePoolHelper
.
Description
A helper to stop pools. Only pools that are added to the list can be stopped. The Hypernative keeper must be added to Authentication to access the PauseHelper. The PauseHelper must be added to Authentication to access the Vault pausePool.
Type of change
Checklist:
main
, or there's a description of how to mergeIssue Resolution
Closes #1209