Feature/fee manager #136
Feature/fee manager #136
Conversation
There was a problem hiding this comment.
Some of my comments are mostly based on my different conception of how this would be implemented. I'm worried mostly about governance and upgradability. Primarily I want to be able to upgrade the fee setup without redeploying new implementations of the escrow. This might be difficult without changing the factory, so we should think of a middle ground perhaps. Thoughts?
| import "@openzeppelin/contracts/access/Ownable.sol"; | ||
|
|
||
| contract FeeManager is Ownable { | ||
| uint256 public version = 1; |
There was a problem hiding this comment.
How does this versioning work? Is it used for anything?
There was a problem hiding this comment.
Not yet, a stable versioning set-up is at the top of the current to-do list before this is merged
| uint256 endDate; | ||
| } | ||
|
|
||
| uint256 public feePercentage = 0; |
There was a problem hiding this comment.
I think the fee should be in basis points. This wouldn't allow for something like 0.75%
| function setExemption( | ||
| ExemptionType exemptionType, | ||
| uint256 endDate, | ||
| address _address | ||
| ) external onlyOwner { |
There was a problem hiding this comment.
Will this be set to the protocol DAO? Just wondering how the governance is set to work.
There was a problem hiding this comment.
yes, that's the current plan
| function _calculateFee(uint256 _amount) | ||
| internal | ||
| view | ||
| returns (uint256 fee, uint256 adjustedAmount) | ||
| { | ||
| fee = (_amount * feePercentage) / 100; | ||
| adjustedAmount = _amount - fee; | ||
| return (fee, adjustedAmount); | ||
| } |
There was a problem hiding this comment.
I feel like this should be part of the FeeManager rather than in the escrow implementation.
There was a problem hiding this comment.
Yes that makes sense, especially if there are more complicated fee structures down the road that are reused among different types
There was a problem hiding this comment.
The issue here is that we don't want to change the fee on an existing invoice, so we'll have to make things immutable.
| FeeManager feeManager = FeeManager( | ||
| 0x8787678DB688eaD8D53F8F96f33Ceeb0FD821d5a | ||
| ); |
There was a problem hiding this comment.
This feels odd to me, hardcoding addresses in source seems like an anti-pattern.
There was a problem hiding this comment.
Passing in the FeeManager address seemed like a large vulnerability. Hardcoding it takes that away; if the FeeManager is updated then we update the escrow type on the factory to use the FeeManager. The only way we could securely pass I saw would have been to have a setter on the factory.
There was a problem hiding this comment.
Can we maybe add a layer of indirection here? If we're hardcoding an address it should be to something that will never change. That way we can register a new FeeManager there and allow the invoice to look up the current version to set as it's FeeManager. I guess this looks something like a FeeManagerRegistry or FeeManagerFactory.
|
closing for being old! |
Testing FeeManager Implementation