-
Notifications
You must be signed in to change notification settings - Fork 0
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
New Features for 07/22 Audit #20
Conversation
jparklev
commented
Jul 18, 2024
•
edited
Loading
edited
- Add pToken minting fee
- Add reward token redemption fee, less minted pTokens
- Add point claiming delegation function
- Make pTokens pausable
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.
Mostly comments / thoughts / preference changes. I'll make PRs for some and link in the comments.
} | ||
|
||
emit FeesCollected(_pointsId, pTokenFee, rewardTokenFee); | ||
} |
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.
Fees - preference change
An alternate implementation that might afford a bit more flexibility would be to have this be an unguarded function and have a fee collector address. In this function fees would be hardcoded to go the the fee collector address. the fee collector address could be updated via a permissioned setFeeCollector()
function.
This could make collecting fees a bit simpler operationally. Esp if eventually fees are collected in a treasury contract or safe.
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.
Nice, yea no reason it needs to be restricted then. I like that
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.
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.
updated a bit in #26
/// @notice Redeems rewards for point tokens | ||
function trustClaimer(address _account, bool _isTrusted) public { | ||
trustedClaimers[msg.sender][_account] = _isTrusted; | ||
} |
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.
Delegate Claim general thoughts
Initially, I thought we needed a trustClaimerBySig()
.... but since the msg sender for this function is the smart wallet, this wont work.
When the wallet is created however, in the initiation script, we can potentially include a trustClaimer call so that this is setup right away in the deploy transaction.
It would be nice to include the vault upgrade script in-scope of the audit as well, to help confirm the upgrade process doesn't break anything with storage, etc. |
Perfect 🙇
Yes! We're using OZ's upgrades library after a note in the last audit, which should do that for us, but a good reminder to double check that |
* feat: cache ptoken * fix: remove extra spaces
* feat: combine roles * feat: add _role * feat: add _role to variable
* feat: setFeeCollector internal with event * feat: add test