Skip to content

feat: add FeeTapper#118

Open
zhongeric wants to merge 1 commit intomainfrom
feat/FeeTapper
Open

feat: add FeeTapper#118
zhongeric wants to merge 1 commit intomainfrom
feat/FeeTapper

Conversation

@zhongeric
Copy link
Collaborator

@zhongeric zhongeric commented Jan 17, 2026

The recipient is intended to be FeeTapper, a singleton contract which holds a multitude of active deposits. Each currency has a unique Tap, which tracks its balance over time. Each call to sync creates a new deposit, also called Keg. Each Keg holds a per block release amount and an end block. Kegs can be released in batch (all at once per currency, or one by one). Kegs are stored in a flat linked list in storage and Taps hold pointers to the head/tail within the list.

FeeTapper streams fees over time, ensuring that large fee payments are incrementally sent to the fee recipient. For more details on the token jar implementation, check out the https://github.com/Uniswap/protocol-fees/blob/main/src/releasers/ExchangeReleaser.sol


✨ Claude-Generated Content

Summary

Adds the FeeTapper contract, a singleton that handles streaming protocol fees to TokenJar using a configurable per-block release rate to smooth fee distribution.

Changes

  • Add src/feeAdapters/FeeTapper.sol - Singleton contract managing fee streaming with linked-list "kegs" for each currency "tap"
  • Add src/interfaces/IFeeTapper.sol - Interface defining Tap/Keg structs and contract methods
  • Add test/FeeTapper.t.sol - Comprehensive test suite (573 lines) covering sync, release, and edge cases
  • Add snapshots/FeeTapperTest.json - Gas benchmarks for operations

Key Features

  • Linear fee release: Fees are released over time based on perBlockReleaseRate (default 10 bps/block = 1000 blocks for full release)
  • Multi-currency support: Handles both native ETH and ERC20 tokens
  • Keg management: Each deposit creates a "keg" with its own release schedule; empty kegs are automatically cleaned up via linked list operations
  • Owner controls: Release rate configurable by owner (must evenly divide 10000 bps)
  • Gas efficient: Single keg release ~70k gas, multiple keg release with cleanup ~88k gas

@github-actions
Copy link

github-actions bot commented Jan 17, 2026

🤖 Claude Code Review

Review complete

Review Summary

This PR introduces FeeTapper, a streaming mechanism for releasing protocol fees to TokenJar over time. The contract uses a linked-list of "kegs" per currency to track multiple deposits, each with its own linear release schedule.

The design is sound and the tests are thorough. The linked-list management for keg cleanup is well-implemented, handling edge cases like middle-keg deletion and tail updates correctly.

Key Observations

Potential issue with sync() when balance decreases:

In sync() at line 72, the calculation uint192 amount = balance - oldBalance will underflow if the current balance is less than oldBalance. This could happen if:

  • The tokenJar or other address drains funds via an external call
  • A previous release() transferred funds out

However, looking at the flow: release() calls _process() which updates $tap.balance after transfer. So the tap's balance tracks what should be there, not what is there. If someone externally withdraws funds, the next sync() would underflow.

After more thought: sync() uses balanceOfSelf() which is the actual contract balance, but $tap.balance tracks what's been synced. When _process() does $_taps[_currency].balance -= toRelease, it's reducing the tracked balance. So after a release, if you call sync():

  • balance = actual balance (reduced by transfer out)
  • oldBalance = tracked balance (also reduced by _process)

These should stay in sync, so this is fine. The subtraction at line 72 is safe because new deposits can only increase the actual balance above the tracked balance.

nextId overflow: The unchecked { nextId++; } at line 66-68 will wrap around after 2^32 syncs. This is ~4 billion operations, unlikely to be hit in practice. When it wraps to 0, the next increment makes it 1, which would overwrite keg ID 1 if it still exists. This is a theoretical concern but extremely unlikely given gas costs.

No access control on sync(): Anyone can call sync() for any currency. This is by design (noted in interface docs) and is safe since it only affects the internal accounting of deposits already received.

Minor items

  • Typo at line 123: "Itereate" → "Iterate"
  • The release(uint32 id) function doesn't validate that id corresponds to an active keg. Calling with an invalid ID will return 0 (since default keg has all zeros), which is acceptable behavior.

Overall this is clean, well-tested code with good gas optimization via deferred BPS division.


💡 Want a fresh review? Add a comment containing @request-claude-review to trigger a new review at any time.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📋 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.

uint32 prev = 0;
uint32 curr = $tap.head;
uint256 blockNumber = block.number;
// Itereate through all kegs. This can be very costly if there are lot of kegs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "Itereate" should be "Iterate".

Suggested change
// Itereate through all kegs. This can be very costly if there are lot of kegs.
// Iterate through all kegs. This can be very costly if there are lot of kegs.

/// @param id The id of the keg to release
function _release(Keg memory keg, uint32 id) internal returns (uint192 releasedAmount) {
uint48 minBlock = uint48(_min(block.number, keg.endBlock));
releasedAmount = uint192(keg.perBlockReleaseAmount * (minBlock - keg.lastReleaseBlock));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why arent minBlock and releasedAmount just uint256s? I think smaller types are more expensive because the compiler has to clean upper bits

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i see why, its packing in Keg struct

/// @notice Mapping of currencies to Taps
mapping(Currency => Tap) private $_taps;
/// @notice Linked list of kegs. Taps manage the head and tail of their respective kegs.
mapping(uint32 => Keg) private $_kegs;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a high level - whats the motivation to have multiple kegs per currency? If the contract has say $1m USDC in it, dont we want to limit the total USDC that can get dripped in each block? That to me feels like 1 tap that just gets topped up over time with each deposit and no separation of different release schedules for different sets of tokens. But maybe theres a specific reason for that?

Currency currency; /// @notice The currency associated with the keg
uint48 endBlock; /// @notice The block at which the deposit will be fully released
uint48 lastReleaseBlock; /// @notice The block at which the last release was made
uint192 perBlockReleaseAmount; /// @notice The absolute amount of the currency released per block

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment isnt right - its confusingly still multiplied by BPS


/// @notice Transfers the released amount to the token jar
function _process(Currency _currency, uint192 _releasedAmount) internal returns (uint192) {
// Because we deferred dividing by BPS when storing the perBlockReleaseAmount, we need to divide

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats the reason for this? it feels potentially bug-prone to leave it multiplied by BPS unless comments and naming are both clearer about that in the rest of the contract

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants