Conversation
Rebase Circuit Breaker
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
brandoniles
left a comment
brandoniles
left a comment
There was a problem hiding this comment.
I was wondering if we should use a circular array, rather than a linearly increasing hashmap. The flexibility of the map the gas benefits, though, I agree. It could still be moved to in the future if it became a concern
aalavandhan
force-pushed
the
rebase-hist
branch
from
666833a to
101ec2a
Compare
aalavandhan
force-pushed
the
rebase-hist
branch
from
101ec2a to
21060f8
Compare
brandoniles
left a comment
brandoniles
left a comment
There was a problem hiding this comment.
I like the idea of having an owner-callable history cleanup function. I imagine it would take a start epoch, end epoch, and wouldn't zero-out anything within the existing lookback window.
| * @notice Sets the parameters which control rebase circuit breaker. | ||
| * @param epochLookback_ The number of rebase epochs to look back. | ||
| * @param tolerableDeclinePercentage_ The supply decline percentage which is allowed | ||
| * within the defined look back period. |
There was a problem hiding this comment.
| * within the defined look back period. | |
| * from the beginning of the defined look back period. |
| uint8 epochLookback_, | ||
| int256 tolerableDeclinePercentage_ | ||
| ) external onlyOwner { | ||
| require(tolerableDeclinePercentage_ > 0 && tolerableDeclinePercentage_ <= ONE); |
There was a problem hiding this comment.
I guess 0 "should" be an allowable value... It's not a useful config per se, but there's no mechanical reason the implementation wouldn't allow it.
| ); | ||
| return uFrags.totalSupply().toInt256Safe().mul(rebasePercentage).div(ONE); | ||
|
|
||
| int256 currentSupply = uFrags.totalSupply().toInt256Safe(); // (or) supplyHistory[epoch] |
There was a problem hiding this comment.
| int256 currentSupply = uFrags.totalSupply().toInt256Safe(); // (or) supplyHistory[epoch] | |
| int256 currentSupply = uFrags.totalSupply().toInt256Safe(); |
I don't think the supplyHistory has been written to yet, in the core rebase flow.
| return uFrags.totalSupply().toInt256Safe().mul(rebasePercentage).div(ONE); | ||
|
|
||
| int256 currentSupply = uFrags.totalSupply().toInt256Safe(); // (or) supplyHistory[epoch] | ||
| int256 newSupply = ONE.add(rebasePercentage).mul(currentSupply).div(ONE); |
There was a problem hiding this comment.
Looking for it, but I don't see this change... Where did rebasePercentage move from being 1-centered to 0-centered? (We add one here but not in the previous version's line)
|
|
||
| // When supply is decreasing: | ||
| // We limit the supply delta, based on recent supply history. | ||
| if (rebasePercentage < 0 && epoch > epochLookback) { |
There was a problem hiding this comment.
In the case that the supply history hasn't been written to yet, and is 0, then the below code basically no-ops? That's what it seems like. Do we have an explicit test case for this?
i.e.
- Test for supply history lookup returns 0
- Test for when epochLookback equals 0 (should basically be same as above since history will return 0)
2 participants
Owner can limit the total supply contraction over an x-epoch period