PCV Oracle Refactor#195
Conversation
| mapping(address => VenueData) public venueRecord; | ||
|
|
||
| /// @notice cached total PCV amount | ||
| uint256 public totalRecordedPcv; |
There was a problem hiding this comment.
i'd rename to lastRecordedTotalPcv and remove the getter getTotalCachedPcv because it should have the same value and the state variable is public
eswak
left a comment
eswak
left a comment
There was a problem hiding this comment.
Would be nice to maintain the good coverage this contract has, and add tests for the new functions you added:
Coverage before:
| File | % Lines | % Statements | % Branches | % Funcs |
|-------------------------------------------------|-----------------|------------------|-----------------|-----------------|
| src/oracle/PCVOracle.sol | 100.00% (63/63) | 100.00% (80/80) | 75.00% (24/32) | 100.00% (12/12) |
Coverage after:
| File | % Lines | % Statements | % Branches | % Funcs |
|-------------------------------------------------|-----------------|-------------------|-----------------|----------------|
| src/oracle/PCVOracle.sol | 90.59% (77/85) | 90.74% (98/108) | 66.67% (24/36) | 76.47% (13/17) |
The following functions don't have unit tests :
venueRecord(address)getNumVenues()lastRecordedProfit()getVenueBalance()+ its revert cases- new revert case on
updateBalance(...)(impossible to have negative balances)
You can get the coverage for your current work by running something like:
$> forge coverage --match-contract PCVOracleUnitTest --report lcov
And then use a VS code extension like Coverage Gutters to show the coverage of the contracts
Also you mentioned on Discord that getTotalLastRecordedPcv has been deleted, which is not the case in the diff, so maybe you have some local commits that aren't in the PR
Added tests, at 100% function coverage and 77.78% of branches: |
* remove reentrancy checks when lock is set to address 0 * add lock tests * add additional reentrancy tests when lock is set to 0 * update comments
2 participants
Refactor PCV Oracle to track last recorded profit, last recorded balance and last recorded global PCV.