feat: add FeeDripper for smoothing protocol fee inflows to TokenJar#126
feat: add FeeDripper for smoothing protocol fee inflows to TokenJar#126mgretzke wants to merge 19 commits into
Conversation
🤖 Claude Code Review
SummaryThis PR introduces a
Critical IssueTest compilation error: The test file references an event Code QualityThe contract is well-structured with:
Feedback from Prior ReviewSeveral items from the prior review appear to be addressed:
The test file needs to be fixed before this can merge. 💡 Want a fresh review? Add a comment containing |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only. 1 inline comment(s) are attached below.
github-actions
Bot
changed the title
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
| /// Griefing note: an attacker can still delay flow by adding enough balance to meet the | ||
| /// reset threshold and repeatedly calling `drip()`, but cannot steal or redirect funds. | ||
| /// @param currency The currency to drip | ||
| function drip(Currency currency) external; |
There was a problem hiding this comment.
I dont really like the difference in name between drip and release
If a currency has a Drip set up for it, and some tokens are accrued already, then they are ready to "drip" out... so it feels wrong that release is the name of that function.
Much more boring but I feel like the functions should just be called like drip (for release), and dripAndReset or dripAndUpdate or something. But maybe thats too boring 😆
There was a problem hiding this comment.
I am happy to change drip into something else, but on a first read, drip and dripAndUpdate feel harder to differentiate for me.
There was a problem hiding this comment.
Maybe a larger change but, what do you think of having drip() call release before updating the Drip state?
Would make it clearer when each should be used. I think it's possible but maybe missing something
There was a problem hiding this comment.
Do you feel like this is worth the additional sstore and sload just for clarity? I tried to already combine as much as possible within _prepareRelease and _releaseTokens.
I feel like we can expect integrators to differentiate between those two easily enough since the interface is so tiny with only three exposed functions in total.
There was a problem hiding this comment.
Tbh I think it could be worth it for readability, but ultimately leave the decision up to you. To me, it seems much cleaner for these to be able to be called sequentially
|
|
||
| // If the remaining balance is less than the release window, immediately release the remaining | ||
| // balance to skip dust accumulation | ||
| if (remainingBalance < _releaseSettings.releaseWindow) { |
There was a problem hiding this comment.
This feels off to me? Why are we comparing a balance with a period of time?
There was a problem hiding this comment.
This is just to not have any uncollectible dust.
if remainingBalance < _releaseSettings.releaseWindow, the per block drip rate would be zero:
uint160 updatedPerBlockRate = uint160(remainingBalance / _releaseWindow);
There was a problem hiding this comment.
This comparison (line 209) makes sense: if postDripBalance (token amount) is less than releaseWindow (number of blocks), the per-block rate would be 0, meaning dust that would never drip out. Flushing it immediately avoids dust accumulation.
There was a problem hiding this comment.
📋 Review verdict: COMMENT
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only. 2 inline comment(s) are attached below.
| // Copy the drip state to memory | ||
| Drip memory dripState = drips[currency]; | ||
|
|
||
| (, uint256 releasedAmount,) = _prepareRelease(currency, dripState); |
There was a problem hiding this comment.
This calls _prepareRelease which may trigger dust flush (adding remainingBalance to releasedAmount), but only latestReleaseBlock is updated here. If dust is flushed mid-stream, perBlockRate and endReleaseBlock remain stale, potentially causing underflow on the next call when currentBalance - releasedAmount is computed at line 139.
Consider either:
- Moving dust flush logic only to
drip()as hensha256 suggested - Or updating
perBlockRate/endReleaseBlockwhen dust flush occurs inrelease()
|
|
||
| // If the remaining balance is less than the release window, immediately release the remaining | ||
| // balance to skip dust accumulation | ||
| if (remainingBalance < _releaseSettings.releaseWindow) { |
There was a problem hiding this comment.
This comparison (balance vs block count) works because it effectively checks if per-block rate would be < 1 token/block. A brief comment explaining this would address hensha256's concern about the seemingly mismatched types.
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
|
please can you push a new claude version to upgrade the CI flow to use opus 4.6? |
| returns (uint160 perBlockRate, uint48 endReleaseBlock, uint48 latestReleaseBlock) | ||
| { | ||
| // Read the drip state from storage and unpack into stack variables | ||
| assembly ("memory-safe") { |
There was a problem hiding this comment.
why does drip state now need to be in assembly? cant this just be a packed struct?
There was a problem hiding this comment.
given the plan is for this contract to handle many millions of dollars i feel like saving a tiny bit of gas and sacrificing readability might not be worth it
There was a problem hiding this comment.
When using solidity structs directly, whether reading or writing, it will compile so that all data will be written into memory first, even if the struct is only declared from stack variables to be packed and stored in a single slot. Changing this saved around 500 gas just by skipping the unnecessary memory allocations.
It seemed to me like a very low hanging fruit (more so then the branchless logic) since its very isolated in its helper function and rather simple assembly code imo.
| /// Griefing note: an attacker can still delay flow by adding enough balance to meet the | ||
| /// reset threshold and repeatedly calling `drip()`, but cannot steal or redirect funds. | ||
| /// @param currency The currency to drip | ||
| function drip(Currency currency) external; |
There was a problem hiding this comment.
Maybe a larger change but, what do you think of having drip() call release before updating the Drip state?
Would make it clearer when each should be used. I think it's possible but maybe missing something
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
There was a problem hiding this comment.
📋 Review verdict: REQUEST_CHANGES
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only. 1 inline comment(s) are attached below.
| uint48 expectedEnd = uint48(block.number + window); | ||
|
|
||
| vm.expectEmit(true, true, true, true); | ||
| emit IFeeDripper.DripStarted(Currency.unwrap(currency), expectedEnd, expectedRate); |
There was a problem hiding this comment.
This references IFeeDripper.DripStarted but the interface only defines DripUpdated. This will cause a compilation error.
| emit IFeeDripper.DripStarted(Currency.unwrap(currency), expectedEnd, expectedRate); | |
| emit IFeeDripper.DripUpdated(Currency.unwrap(currency), expectedEnd, expectedRate); |
🤖 Claude Code ReviewStatus: The review was completed but there was an error posting inline comments. Raw Review OutputPlease check the workflow logs for details. |
🤖 Claude Code ReviewStatus: ❌ Error
|
🤖 Claude Code ReviewStatus: The review was completed but there was an error posting inline comments. Raw Review OutputPlease check the workflow logs for details. |
🤖 Claude Code ReviewStatus: ❌ Error
|
🤖 Claude Code ReviewStatus: The review was completed but there was an error posting inline comments. Raw Review OutputPlease check the workflow logs for details. |
🤖 Claude Code ReviewStatus: ❌ Error
|
3 participants
PR Description: FeeDripper
Summary
Introduce
FeeDripper, a single-stream fee smoothing adapter that meters protocol fee inflows toTokenJarover a configurable window.Motivation
Large fee inflows can overfill
TokenJarand reduce capital efficiency around release mechanisms.FeeDrippersmooths inflows per currency over time while keeping operations permissionless and storage-efficient.Key Design Choices
Dripstate in one slot):perBlockRateendReleaseBlocklatestReleaseBlockdrip(currency): release accrued amount and (re)compute stream for remaining balance.release(currency): release accrued amount only, without recomputing stream.releaseWindowwindowResetBps(threshold for mid-window reset behavior)releaseWindow, flush immediately to avoid dust accumulation.Notable Behavior
drip()is permissionless and can reset or keep a running window based onwindowResetBps.releaseWindow.DripStarted.Security / Risk Notes
dripwith sufficient deposits), but funds cannot be stolen or redirected. The griefing slows the flow, which aligns with the metering goal. The real harm is reduced capital efficiency: fees sit idle inFeeDripper, and for volatile tokens the burn mechanism may miss favorable selling windows. The griefing can be made more expensive by increasing thewindowResetBpsthreshold.✨ Claude-Generated Content
Summary
Introduces
FeeDripper, a single-stream fee smoothing adapter that meters protocol fee inflows toTokenJarover a configurable block window. Large fee inflows can overfillTokenJarand reduce capital efficiency around release mechanisms.FeeDrippersmooths inflows per currency over time while keeping operations permissionless and storage-efficient.Changes
New Contracts:
src/feeAdapters/FeeDripper.sol- Single-stream fee metering contract with configurable release window and basis point threshold for window resetssrc/interfaces/IFeeDripper.sol- Interface with events (DripUpdated,Released,ReleaseSettingsSet), errors, and function signaturesTests:
test/FeeDripper.t.sol- Comprehensive unit tests covering constructor validation, release settings, drip/release mechanics, dust handling, griefing scenarios, currency independence, and gas benchmarksGas Snapshots:
snapshots/FeeDripperTest.json- Gas snapshots for drip and release operationsOther:
.github/workflows/claude-code-review.yml- Update Claude model versionKey Design
Dripstate packed in one slot:perBlockRate(uint160),endReleaseBlock(uint48),latestReleaseBlock(uint48)drip(currency)releases accrued + recomputes stream;release(currency)releases accrued only without recomputingreleaseWindow(default: 2000 blocks),windowResetBps(default: 50 bps threshold for mid-window reset)releaseWindowflush immediately to avoid dust accumulationpostDripBalancecapped atuint160.maxinstead of revertingNotes
drip()andrelease()are callable by anyonewindowResetBpsthreshold keep the existing end block; equal/above-threshold deposits reset to a freshreleaseWindowdripwith sufficient deposits), but funds cannot be stolen or redirected. Griefing can be made more expensive by increasing thewindowResetBpsthreshold.