Remove PSM and ERC20allocator, Consolidate GRLM and GSERL #197
Conversation
…volt-protocol-core into feat/pcv-deposit-v2-base
…volt-protocol-core into feat/pcv-deposit-v2-base
* checkpoint * fix unit tests * fix failing integration tests * fix failing unit tests * unit tests fixed * rename functions * remove getTotalLastRecordedPcv * update tests * increase test coverage * add pcv oracle hook to mock, increase unit test coverage * add additional tests and fix existing ones * import ordering * update mock and fix units * PCV Guardian todo * remove outdated todo * oracle invalid comment * oracle interface addition * Global Reentrancy Modifier Changes (#196) * 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
…volt-protocol-core into feat/remove-psm-erc20allocator
ElliotFriedman
changed the base branch from
feat/market-governance
to
feat/core-coreref-cleanup
| // Read oracle to get USD values | ||
| address oracle = venueToOracle[venue]; | ||
|
|
||
| require(oracle != address(0), "PCVOracle: invalid caller deposit"); |
There was a problem hiding this comment.
nit: it's not really a caller deposit
|
|
||
| require(oracleValid, "PCVOracle: invalid oracle value"); | ||
|
|
||
| return venueRecord[venue].lastRecordedPCV / oracleValue; |
There was a problem hiding this comment.
Does that work ? If there is 1000$ in an USDC venue, lastRecordedPCV should be 1000e18 and oracleValue should be 1e30, so it will return 0. I think you should return venueRecord[venue].lastRecordedPCV * 1e18 / oracleValue
I see you added a test for this but it must be weird because USD balances are incremented by balance * oracleValue / 1e18 so the formula for reversing should be the one I said above
| /// TODO add governance markdown flow | ||
| /// need a flow to create a phantom PCV deposit that doesn't exist but has a balance |
| /// @notice returns decimal normalized version of a given venues USD balance | ||
| function getVenueBalance( |
There was a problem hiding this comment.
nice add, could we name it getVenuePcv ? it returns an USD amount and not the balance
| VenueData storage ptr = venueRecord[venue]; | ||
|
|
||
| uint128 newLastRecordedPCV = (ptr.lastRecordedPCV.toInt256() + | ||
| deltaBalanceUSD).toUint256().toUint128(); | ||
| int128 newLastRecordedProfit = ptr.lastRecordedProfit + | ||
| deltaProfitUSD.toInt128(); | ||
|
|
||
| /// single SSTORE | ||
| ptr.lastRecordedPCV = newLastRecordedPCV; | ||
| ptr.lastRecordedProfit = newLastRecordedProfit; |
There was a problem hiding this comment.
I think you can also do just one SLOAD if you declare the variable as memory ? (and set it with venueRecord[venue] = ptr;)
| /// Rate limit contract has a mid point that it tries to maintain. | ||
| /// When the stored buffer is above the mid point, time depletes the buffer | ||
| /// When the stored buffer is below the mid point, time replenishes the buffer | ||
| /// When buffer stored is at the mid point, do nothing |
There was a problem hiding this comment.
It's starting to differ a lot from the initial RateLimited contract, instead of naming it V2, we could rename RateLimitedMidpoint ?
| uint128 public rateLimitPerSecond; | ||
| uint64 public rateLimitPerSecond; | ||
|
|
||
| /// @notice the cap of the buffer that can be used at once | ||
| uint128 public bufferCap; | ||
| /// @notice buffer upper limit | ||
| uint96 public bufferCap; | ||
|
|
||
| /// @notice buffercap / 2 | ||
| uint96 public midPoint; | ||
|
|
There was a problem hiding this comment.
uint64 for rateLimitPerSecond is too restrictive, that is 1.5M (e18) per day
Same of uint96 for the bufferCap (that's 79B)
I know the protocol will probably never need these high values, but we're starting to enter the realm of values that are possible, I'd rather optimize less for gas, and keep the larger variable sizes
| @@ -128,7 +149,7 @@ abstract contract RateLimitedV2 is IRateLimitedV2, CoreRefV2 { | |||
| emit BufferReplenished(amount, bufferStored); | |||
There was a problem hiding this comment.
also use newBufferStored when you emit BufferReplenished to save a warm sload (same logic as the change you did for _depleteBuffer)
| token1.mint(address(deposit1), 100e18); | ||
| entry.deposit(address(deposit1)); | ||
|
|
||
| vm.expectRevert("PCVOracle: cannot read while entered"); | ||
| pcvOracle.getTotalPcv(); | ||
| assertEq(pcvOracle.lastRecordedPCV(address(deposit1)), 100e12); /// with scale up | ||
| assertEq(pcvOracle.lastRecordedPCVRaw(address(deposit1)), 100); /// remove scale up |
There was a problem hiding this comment.
That seems messed up, is 100e18 tokens are minted to the deposit, last balance should be 100e18 and lastRecordedPCV should be whatever the oracle value is
| function testTotalPcvNegativeReverts() public { | ||
| vm.expectRevert("SafeCast: value must be positive"); | ||
| deposit1.setLastRecordedProfit(-1); |
Cleaning things up here further.
We're removing the PSM, ERC20 Allocator, Volt Migrator, Volt Migrator Router, and consolidating the logic for global system exit and entry into a single contract and role.
The RateLimited contract had to be reworked to support this consolidation of entry and exit into a single contract. Instead of only reverting on depletion, the rate limiter now has a mid point which it starts at, and any time the price goes below the mid point, it will go back up, and when it goes above the mid point, the replenishing will push it back towards the mid point. If the buffer cap is exceeded on a replenish action, this will cause a revert.
This version of the system will not support a hard cap on the Volt supply through the rate limit. That check will have to live elsewhere.