-
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
ProtocolFeeSweeper
: use fee recipient as admin
#1315
base: main
Are you sure you want to change the base?
Conversation
- Add tests - Refactor common auth to vault
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.
Tests failed (lint) but LGTM
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.
Ideally would have separated into authentication infra vs. fee-sweeper PRs, but looks good (have some mostly rename-related comments).
@@ -114,4 +107,28 @@ contract PoolMock is IBasePool, IPoolLiquidity, BalancerPoolToken, BasePoolAuthe | |||
function getRate() public view override returns (uint256) { | |||
return _mockRate == 0 ? super.getRate() : _mockRate; | |||
} | |||
|
|||
function getTokenInfo() |
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.
Why not keep the PoolInfo base contract, which already defines getTokenInfo
and getAggregateFeePercentages
the same way?
Not sure why it had BasePoolAuthentication; seems fine to remove that, as there are no permissioned functions at this level.
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.
because it makes the vault package implicitly depend on pool-utils
, whereas actually it's the other way around.
It didn't cause issues here, but BasePoolAuthentication
was throwing a very confusing one; this should prevent that from happening.
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!
Description
This PR:
Type of change
Checklist:
main
, or there's a description of how to mergeIssue Resolution
N/A