feat(PRT): Add `Prt` and `PrtFeeSplitExtension` #176

pblivin0x · 2024-06-11T01:10:05Z

No description provided.

ckoopmann

Good stuff.
I added a few comments on the Extension and StakingPool contracts. A lot of them just nit picking on event emission etc. But you might want to check out the alternative mechanism to calculate pending rewards that I linked.
Happy to elaborate.

contracts/adapters/PrtFeeSplitExtension.sol

contracts/staking/PrtStakingPool.sol

…emissions

ckoopmann

Looks good👍
Changes / comments all make sense to me. Snapshot logic should also work well enough given monthly updates and partial claim function.

Maybe try to fix ci or just skip the failing test.
Other than that no more issues from my side.

contracts/staking/PrtStakingPool.sol

ckoopmann

Mostly nitpicking. One more substantial comment / question on the accruerHistory and one suggestion / reminder regarding integration tests.

Great work 👍

contracts/adapters/PrtFeeSplitExtension.sol

ckoopmann · 2024-06-20T03:02:26Z

test/adapters/prtFeeSplitExtension.spec.ts

@@ -0,0 +1,760 @@
+import "module-alias/register";


Now that we have moved the staking pool to the other repo we don't have integration tests anywhere.
We should probably add those somewhere. (we could wait until one of them is deployed though to avoid having to import one repo into the other).

let's handle integration tests in https://github.com/IndexCoop/periphery

edkim

Great work! Mostly minor comments/suggestions. The one thing to look at more closely is if we should enforce that the StakingPool's stakeToken matches the address stored in the prt variable.

edkim · 2024-06-20T15:13:26Z

contracts/adapters/PrtFeeSplitExtension.sol

+        mutualUpgrade(manager.operator(), manager.methodologist()) 
+    {
+        require(address(_prtStakingPool) != address(0), "Zero address not valid");
+        require(_prtStakingPool.feeSplitExtension() == address(this), "PrtFeeSplitExtension must be set");


Note that in the newer version of SnapshotStakingPool.sol the public variable is called distributor and not feeSplitExtension

Should we also check to make sure the Staking Pool's stakeToken matches the address stored in prt?

In the constructor we check if prt.setToken() is correct, but I don't think that's effective unless we also check that the StakingPool uses that same prt.

added additional checks to match the SnapshotStakingPool here 515e783

edkim · 2024-06-20T15:24:56Z

contracts/adapters/PrtFeeSplitExtension.sol

+        bool[] memory _statuses
+    )
+        external
+        mutualUpgrade(manager.operator(), manager.methodologist())


Does this function have to be mutual upgrade? To me it feels like this is something the operator should be responsible over. Also, the Leverage Strategy extensions updateCallerStatus is onlyOperator rather than mutual upgrade.

updated to onlyOperator here 8015c24

edkim · 2024-06-20T15:26:22Z

contracts/adapters/PrtFeeSplitExtension.sol

+     */
+    function updateAnyoneAccrue(bool _status)
+        external
+        mutualUpgrade(manager.operator(), manager.methodologist())


Same comment as above for setAccruersStatus

updated to onlyOperator here 8015c24

edkim · 2024-06-20T15:27:47Z

contracts/interfaces/IPrtStakingPool.sol

+
+interface IPrtStakingPool {
+    function accrue(uint256 _amount) external;
+    function feeSplitExtension() external view returns (address);


Noting the newer version of SnapshotStakingPool uses the variable name distributor instead of feeSplitExtension

fixed here 515e783

edkim · 2024-06-20T15:30:13Z

contracts/adapters/PrtFeeSplitExtension.sol

+     * sufficient for accounting for all collected fees. If the PRT take is greater than 0, the PRT Staking Pool will accrue the fees
+     * and update the snapshot.
+     */
+    function accrueFeesAndDistribute() public override onlyAllowedAccruer {


Maybe we should add a line for require(address(prtStakingPool) != address(0), "PRT Staking Pool not set"), as a reminder that this function can be called before the prtStakingPool is defined?

added here e7a8a8c

edkim · 2024-06-20T15:32:08Z

contracts/token/Prt.sol

+    {
+        setToken = _setToken;
+        _mint(_distributor, _totalSupply);
+        _setupDecimals(18);


Nit: Not really necessary, since the default is 18. https://docs.openzeppelin.com/contracts/3.x/api/token/erc20#ERC20-_setupDecimals-uint8-

removed here 932489a

edkim

Changes LGTM 🔥

ckoopmann

LGTM ! 🛳️

add prt staking pool system

ffa3671

ckoopmann reviewed Jun 11, 2024

View reviewed changes

pblivin0x added 9 commits June 11, 2024 16:00

make nontransferrable, test cleanups

d30288a

cleanup immutables

2b589d2

check for 0 accrual on PrtStakingPool

4e6d20b

add total supply check on PrtStakingPool.accrue

fecfa95

make PrtFeeSplitExtension update functions mutual upgrade, add event …

fe30c96

…emissions

add optional allow list on accrueFeeAndDistribute()

fc4b6aa

update lastSnapshotId when staking

09abd4f

add partial rewards claiming

a0c9bcc

add optional snapshot delay to PrtStakingPool.accrue()

df8ce65

ckoopmann self-requested a review June 12, 2024 12:50

ckoopmann approved these changes Jun 12, 2024

View reviewed changes

ckoopmann reviewed Jun 12, 2024

View reviewed changes

contracts/staking/PrtStakingPool.sol Outdated Show resolved Hide resolved

pblivin0x commented Jun 13, 2024

View reviewed changes

contracts/staking/PrtStakingPool.sol Outdated Show resolved Hide resolved

pblivin0x commented Jun 13, 2024

View reviewed changes

contracts/staking/PrtStakingPool.sol Outdated Show resolved Hide resolved

pblivin0x commented Jun 14, 2024

View reviewed changes

contracts/staking/PrtStakingPool.sol Outdated Show resolved Hide resolved

pblivin0x added 2 commits June 14, 2024 12:16

remove lastSnapshotId from stake

ca1a5e8

remove PrtStakingPool

0996cfa

pblivin0x changed the title ~~feat(PRT): Add PrtStakingPool System~~ feat(PRT): Add Prt and PrtFeeSplitExtension Jun 19, 2024

pblivin0x marked this pull request as ready for review June 19, 2024 20:58

pblivin0x requested a review from edkim June 19, 2024 20:58

ckoopmann reviewed Jun 20, 2024

View reviewed changes

edkim requested changes Jun 20, 2024

View reviewed changes

pblivin0x added 6 commits June 20, 2024 12:37

fix event variables

66abe3f

update accrueAllowList and accrueAllowMap

7169de6

add extra checks to updatePrtStakingPool()

515e783

remove unnecessary _setupDecimals()

932489a

add PrtStakingPool address(0) check on accrueFeesAndDistribute()

e7a8a8c

fix test

67bc64a

update accrue permissions to onlyOperator

8015c24

pblivin0x requested review from edkim and ckoopmann June 20, 2024 19:17

edkim approved these changes Jun 20, 2024

View reviewed changes

ckoopmann approved these changes Jun 23, 2024

View reviewed changes

pblivin0x merged commit 6477886 into master Jun 24, 2024
5 checks passed

pblivin0x deleted the prt-base branch June 24, 2024 02:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(PRT): Add `Prt` and `PrtFeeSplitExtension` #176

feat(PRT): Add `Prt` and `PrtFeeSplitExtension` #176

pblivin0x commented Jun 11, 2024

ckoopmann left a comment

ckoopmann left a comment •

edited

Loading

ckoopmann left a comment

ckoopmann Jun 20, 2024 •

edited

Loading

pblivin0x Jun 20, 2024

edkim left a comment

edkim Jun 20, 2024

edkim Jun 20, 2024

pblivin0x Jun 20, 2024

edkim Jun 20, 2024

pblivin0x Jun 20, 2024

edkim Jun 20, 2024

pblivin0x Jun 20, 2024

edkim Jun 20, 2024

pblivin0x Jun 20, 2024

edkim Jun 20, 2024

pblivin0x Jun 20, 2024

edkim Jun 20, 2024

pblivin0x Jun 20, 2024

edkim left a comment

ckoopmann left a comment

feat(PRT): Add Prt and PrtFeeSplitExtension #176

feat(PRT): Add Prt and PrtFeeSplitExtension #176

Conversation

pblivin0x commented Jun 11, 2024

ckoopmann left a comment

Choose a reason for hiding this comment

ckoopmann left a comment • edited Loading

Choose a reason for hiding this comment

ckoopmann left a comment

Choose a reason for hiding this comment

ckoopmann Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edkim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edkim left a comment

Choose a reason for hiding this comment

ckoopmann left a comment

Choose a reason for hiding this comment

feat(PRT): Add `Prt` and `PrtFeeSplitExtension` #176

feat(PRT): Add `Prt` and `PrtFeeSplitExtension` #176

ckoopmann left a comment •

edited

Loading

ckoopmann Jun 20, 2024 •

edited

Loading