-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: pack pf config #211
feat: pack pf config #211
Conversation
could also do this for the strategyParams struct in the 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.
Passing tests plz
8fa9cea
to
729c2b9
Compare
729c2b9
to
8fdd36e
Compare
contracts/VaultFactory.vy
Outdated
@@ -172,15 +170,44 @@ def protocol_fee_config(vault: address = msg.sender) -> PFConfig: | |||
@return The protocol fee config for the msg sender. |
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.
return values changed, so we may want to update to:
@return Fee in bps
@return Address of fee recipient
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.
contracts/VaultFactory.vy
Outdated
return config_data & 1 == 1 | ||
|
||
@internal | ||
def _pack_data(recipient: address, fee: uint16, custom: bool) -> uint256: |
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.
small:
could use a more descriptive function name like _pack_fee_config_data
for clarity if/when more slot packing functions are needed in future.
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.
|
||
@view | ||
@external | ||
def use_custom_protocol_fee(vault: address) -> bool: |
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.
would has_
be more precise than use_
here? to me it is a clearer and feels less like we are taking some action.
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.
while that likely is more accurate, would prefer to keep the ABI's the same to the previous versions that are already deployed with this same selector.
if not self.use_custom_protocol_fee[vault]: | ||
self.use_custom_protocol_fee[vault] = True | ||
self.custom_protocol_fee_data[vault] = self._pack_data( | ||
empty(address), |
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.
shouldn't this be equal to: self._unpack_fee_recipient(self.default_protocol_fee_data)
?
edit: nvm, you have a better approach on line 178
by getting the value from the default config. thus, we don't have to worry about the recipient ever being out-of-sync between custom/default.
if not self.use_custom_protocol_fee[vault]: | ||
self.use_custom_protocol_fee[vault] = True | ||
self.custom_protocol_fee_data[vault] = self._pack_data( | ||
empty(address), |
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.
Fee recipient is always stored in default_protocol_fee_data
and never in custom_protocol_fee_data
?
* chore: updgrade ape * build: allow max uint (#204) * build: allow max uint * fix: lint version * forge install: openzeppelin-contracts v4.9.5 * chore: oz sub module * forge install: tokenized-strategy v3.0.2 * test: fix foundry tests * chore: bump version * fix: workflow (#207) * chore: update wf version * chore: add statemind report * feat: update name and symbol (#206) * feat: set name and symbol * chore: spech * test: fix emergency * fix: test workflow (#208) * fix: config override * chore: manual setup * fix: requirements * fix: ape version * chore: rebase * chore: dont double pull from storage (#212) * feat: report on self (#205) * feat: report on self * chore: comment * chore: add back * build: auto allocate (#203) * build: auto allocate * build: dont revert debt increase * chore: remove decrease revert * chore: update spech * chore: spech * chore: comments * fix: event * fix: comments Co-authored-by: spalen0 <[email protected]> * feat: pack pf config (#211) * feat: pack pf config * chore: remove event * chore: formatting * test: interface updates * chore: comments * fix: deposit flow (#209) * forge install: openzeppelin-contracts v4.9.5 * chore: oz sub module * test: fix foundry tests * fix: deposit flow * fix: zero total assets * fix: flow * test: full loss * chore: comment * test: add invariants * fix: comments * fix: user msg sender * fix: comments * fix: comment * chore: add to interfaces * fix: comments * chore: match gov abi (#213) * chore: deployment * chore: deployed --------- Co-authored-by: FP <[email protected]> Co-authored-by: spalen0 <[email protected]>
Description
Pack and unpack PF config into one slot manually since vyper does not support storage packing to save an sload in all scenarios.
Fixes # (issue)
Checklist