-
Notifications
You must be signed in to change notification settings - Fork 40
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 helper to set protocol fees by factory #897
Open
EndymionJkb
wants to merge
80
commits into
main
Choose a base branch
from
factory-fee
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+570
−16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EndymionJkb
commented
Aug 18, 2024
# Conflicts: # pkg/interfaces/contracts/vault/IProtocolFeeController.sol
12 tasks
elshan-eth
approved these changes
Dec 17, 2024
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!
EndymionJkb
force-pushed
the
factory-fee
branch
from
January 3, 2025 05:54
30ca45b
to
6cbd390
Compare
# Conflicts: # pkg/interfaces/contracts/vault/IBalancerContractRegistry.sol
# Conflicts: # pkg/interfaces/contracts/vault/IBalancerContractRegistry.sol # pkg/vault/contracts/BalancerContractRegistry.sol # pkg/vault/test/foundry/BalancerContractRegistry.t.sol
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This adds a
ProtocolFeePercentagesProvider
helper contract that allows governance to set protocol fee percentages at the factory level, and then a permissionless function to propagate these fee percentages to individual pools. (Well, permissionless after governance grants the provider contract permission to set pool fees.)An important design constraint was building this on top of the existing infrastructure, with no new dependencies (e.g., not changing existing factories, or adding new requirements for factories, such as keeping an enumerable set of pools, or calling back into anything to register new pools, etc.)
There are no changes to existing contracts: except adding a getter for the maximum swap/yield fee constants to the ProtocolFeeController, which it arguably should have anyway. This avoids duplicating constants, and divergence from the current ProtocolFeeController.
It has two important functions:
setFactorySpecificProtocolFeePercentages
: a permissioned call to set fee percentages by factory. It does some validation that isn't strictly necessary: ensuring that the fees being set are <= max allowed values, and a "best effort" check that the factory is a real factory (i.e., that responds correctly toisPoolFromFactory
calls). Otherwise, you could set values that would fail when you tried to apply them.setProtocolFeePercentagesForPools
: a permissionless call to apply the fee percentages to a set of pools from a given factory. This was over-engineered at first (as I tend to do), taking just a set of pools, and having the contract figure out which factories they belonged to and fetch the corresponding fee percentages. But that required an enumerable set of factories, a "find" function, etc. Probably people are going to set the fees for one factory at a time, using the subgraph, so I think making it completely generic is overkill.It should cut down on governance calls, since they need only approve the factory-wide defaults, not the list of individual pools. No further governance action would be needed for new pools from that factory, as the "propagation" call is permissionless.
Type of change
Checklist:
main
, or there's a description of how to mergeIssue Resolution
Resolves #888