-
Notifications
You must be signed in to change notification settings - Fork 49
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
Fee controller migration contracts #1299
Open
EndymionJkb
wants to merge
45
commits into
main
Choose a base branch
from
fee-controller-script
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.
Open
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
…on structure to simplify future migrations
…less error-prone)
EndymionJkb
commented
Feb 18, 2025
jubeira
reviewed
Feb 18, 2025
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.
Thanks for putting this together so quickly @EndymionJkb.
Overall I think it looks good. I do have a few high level comments:
- I think reviving the script to migrate in stages would be a good idea here. Just set everything offchain, then hit migrate. Governance only grants admin permissions, and the script can only do what was indicated during deployment time. No possibility to leave permissions hanging, nor requirements around granting more permissions.
- I think there's an issue with the code introduced in
registerPool
. Not sure why it didn't make any test fail. - The implementation for
migratePool
makes sense. Perhaps we can migrate the bulk of the pools as part of the script but leave that one open (i.e. permissionless). Given that it can only port pools from the old controller only once the behavior seems to be pretty well constrained.
# Conflicts: # pkg/interfaces/contracts/vault/IProtocolFeeController.sol # pkg/vault/contracts/ProtocolFeeController.sol
# Conflicts: # pkg/pool-stable/test/foundry/E2eBatchSwap.t.sol # pkg/pool-stable/test/foundry/E2eErc4626Swaps.t.sol # pkg/pool-stable/test/foundry/E2eSwapRateProvider.t.sol # pkg/pool-weighted/test/foundry/E2ESwapRateProvider.t.sol
# Conflicts: # pkg/pool-weighted/test/foundry/E2eSwap.t.sol
# Conflicts: # pkg/interfaces/contracts/vault/IProtocolFeeController.sol # pkg/vault/contracts/ProtocolFeeController.sol # pkg/vault/test/foundry/ProtocolFeeController.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
Based on #1307, this is the monorepo part of the ProtocolFeeController migration. It introduces a /governance-scripts package, and adds contracts used for the migration.
Turns out this is actually quite tricky! We might not have thought this all the way through during the original design.
With this in mind, there are two migration scripts. The original motivation for migrating was missing events that made it impossible to track protocol fee exempt pools from the beginning. There are no exempt pools yet, but this would be needed to support them. This PR adds those events - and also another one that will let us easily track pool creators (more on that later).
The first migration (from the one we launched with to the current version) can be simple, since we know there are no pools with creators yet, and no protocol fee exemptions or overrides. Given this, the migration is one-step, and can be performed using only functions available in the original deployment. The only assumption, besides those above, is that all the pools can be migrated within a block. Mainnet now has about 55 pools, so presumably that's not too many, since all we need to pass is the addresses, and they don't need to be stored.
However, future migrations cannot make these assumptions. There will eventually be pool creators, fee exemptions, and overrides. The problem is this state is only written by the Vault at registration time, or by pool creators, and there is no other way to initialize it. Supporting future migrations therefore requires adding some infrastructure to the new ProtocolFeeController so that this state becomes (safely) writeable.
See #1307 for the ProtocolFeeController changes.
Type of change
Checklist:
main
, or there's a description of how to mergeIssue Resolution