Skip to content
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

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Balancer pause helper #1211

wants to merge 32 commits into from

Conversation

elshan-eth
Copy link
Contributor

@elshan-eth elshan-eth commented Jan 7, 2025

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

  • Bug fix
  • New feature
  • Breaking change
  • Dependency changes
  • Code refactor / cleanup
  • Optimization: [ ] gas / [ ] bytecode
  • Documentation or wording changes
  • Other

Checklist:

  • The diff is legible and has no extraneous changes
  • Complex code has been commented, including external interfaces
  • Tests have 100% code coverage
  • The base branch is either main, or there's a description of how to merge

Issue Resolution

Closes #1209

Copy link
Collaborator

@EndymionJkb EndymionJkb left a 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.

pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/foundry.toml Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@EndymionJkb EndymionJkb left a 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.

pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@joaobrunoah joaobrunoah left a comment

Choose a reason for hiding this comment

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

LGTM!

pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
joaobrunoah and others added 4 commits January 21, 2025 11:49
Co-authored-by: bohendo <[email protected]>
Co-authored-by: João Bruno <[email protected]>
Co-authored-by: Jeff Bennett <[email protected]>
Co-authored-by: Juan Ignacio Ubeira <[email protected]>
Co-authored-by: EndymionJkb <[email protected]>
Co-authored-by: Steffen Schuldenzucker <[email protected]>
Copy link
Collaborator

@EndymionJkb EndymionJkb left a 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

pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
* @param to End index
* @return pools List of pools
*/
function getPools(uint256 from, uint256 to) public view returns (address[] memory pools) {
Copy link
Collaborator

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:

  1. simple getPools() if you know there isn't a pagination issue;
  2. generic iteration: for(i = 0; i < getPoolsCount(); ++i) { address pool = getPoolAt(i); }
  3. pagination if needed, using getPools(from, to);

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 couldn't come up with a reason why getPoolAt(uint256 index) might be needed. It seems unnecessary to me.

As for getPools(), I think that if it can potentially break at some point, it's better not to implement such a function.

pkg/standalone-utils/contracts/PauseHelper.sol Outdated Show resolved Hide resolved
EndymionJkb and others added 3 commits January 29, 2025 14:16
# Conflicts:
#	pkg/pool-gyro/contracts/Gyro2CLPPool.sol
#	pkg/pool-gyro/test/foundry/utils/Gyro2ClpPoolDeployer.sol
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve pause helper
4 participants