feat (hubpool): Add protocol fee capture method #31
Conversation
Signed-off-by: chrismaree <christopher.maree@gmail.com>
| bytes32 destinationDistributionRoot, | ||
| bytes32 slowRelayFulfillmentRoot | ||
| ) public noActiveRequests { | ||
| ) public nonReentrant noActiveRequests { |
There was a problem hiding this comment.
modifiers were missing.
| // Interest rate payment that scales the amount of pending fees per second paid to LPs. 0.0000015e18 will pay out | ||
| // the full amount of fees entitled to LPs in ~ 7.72 days, just over the standard L2 7 day liveness. | ||
| uint256 lpFeeRatePerSecond = 1500000000000; | ||
| uint256 public lpFeeRatePerSecond = 1500000000000; |
There was a problem hiding this comment.
this should be public.
There was a problem hiding this comment.
can we make this uint64 to match the other pcts?
| @@ -1,108 +0,0 @@ | |||
| import { toWei, toBNWei, SignerWithAddress, seedWallet, expect, Contract, ethers } from "./utils"; | |||
There was a problem hiding this comment.
this file was a dup of the HubPool HubPool.LiquidityProvisionFees.ts
| // Create an action that will send an L1->L2 tokens transfer and bundle. For this, create a relayer repayment bundle | ||
| // and check that at it's finalization the L2 bridge contracts are called as expected. | ||
| const { leafs, tree, tokensSendToL2 } = await constructSingleChainTree(dai, 1, arbitrumChainId); | ||
| await hubPool.connect(dataWorker).initiateRelayerRefund([3117], 1, tree.getHexRoot(), consts.mockTreeRoot); |
There was a problem hiding this comment.
there was a merge issue from the previous PR. this is unrelated to the main change.
| address public protocolFeeCaptureAddress; | ||
|
|
||
| // Percentage of lpFees that are captured by the protocol and claimable by the protocolFeeCaptureAddress. | ||
| uint256 public protocolFeeCapturePct; |
There was a problem hiding this comment.
Same, should change to uint64
There was a problem hiding this comment.
Can we also put all the uint64 variables next to one another to decrease the cost of reading? Ideally, if we're smart about it, we can group variables that are often read (or written) in the same txn to reduce the overall number of words read/written.
contracts/HubPool.sol
Outdated
| * ADMIN FUNCTIONS * | ||
| *************************************************/ | ||
|
|
||
| function setProtocolFeeCaptureAddress(address newProtocolFeeCaptureAddress) public onlyOwner { |
There was a problem hiding this comment.
Should this function logic be collapsed with the following one?
There was a problem hiding this comment.
ye, good point. can do how we do the setBond method.
contracts/HubPool.sol
Outdated
| } | ||
|
|
||
| function claimProtocolFeesCaptured(address l1Token) public nonReentrant { | ||
| if (unclaimedAccumulatedProtocolFees[l1Token] > 0) { |
There was a problem hiding this comment.
do we need this check..? whats the harm of sending 0, the caller will jsut pay gas
There was a problem hiding this comment.
it's cheaper. it's just for gas optimisation.
I agree with you though that in this context we can remove this. In the context of the fee allocation I think it's better to keep this to remove gas usage.
Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>
Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>
| address public protocolFeeCaptureAddress; | ||
|
|
||
| // Percentage of lpFees that are captured by the protocol and claimable by the protocolFeeCaptureAddress. | ||
| uint256 public protocolFeeCapturePct; |
There was a problem hiding this comment.
Can we also put all the uint64 variables next to one another to decrease the cost of reading? Ideally, if we're smart about it, we can group variables that are often read (or written) in the same txn to reduce the overall number of words read/written.
| onlyOwner | ||
| { | ||
| protocolFeeCaptureAddress = newProtocolFeeCaptureAddress; | ||
| protocolFeeCapturePct = newProtocolFeeCapturePct; |
There was a problem hiding this comment.
Should we check that this is <= 1e18?
contracts/HubPool.sol
Outdated
| pooledTokens[l1Token].undistributedLpFees += lpFeesCaptured; | ||
| pooledTokens[l1Token].utilizedReserves += int256(lpFeesCaptured); |
There was a problem hiding this comment.
Should we wrap this in if (lpFeesCaptured > 0)?
we need the ability for the protocol to capture fees. This PR adds this and associated unit tests.