feat: add split method to Splitter by marcomariscal · Pull Request #31 · ScopeLift/zksync-fee-flow

marcomariscal · 2025-12-16T01:18:22Z

Summary

Add permissionless split() method to Splitter contract
Burns configured percentage of tokens and distributes rest to weighted distributors
Uses SafeERC20 for safe token transfers

Changes

Added split(uint256 _amount) function using safeTransferFrom to pull tokens
Added Split(amount, burned, distributed) event
Added Splitter_AmountMismatch error for fee-on-transfer token protection
Rounding dust from distributor share calculations is burned (not left in contract)
Added 11 comprehensive fuzz tests covering all split scenarios

Test Plan

All 94 tests pass (forge test)
Linter passes (scopelint check)
Fuzz tests cover: burn calculations, single/multiple distributors, 0%/100% burn, dust burning, event emission, zero amount, insufficient approval

Resolves #16

alexkeating · 2025-12-17T15:49:55Z

+
+    // Burn the burn amount + any dust from rounding
+    uint256 _totalBurned = _amount - _totalDistributed;
+    if (_totalBurned > 0) $._splitToken.burn(_totalBurned);


Why wouldn't we call with _burnAmount here

This is documented in the @dev note: "Any dust from rounding is burned along with the burn portion." The _totalBurned captures both the intended burn amount and any dust from integer division in the distribution loop. Let me know if you'd like the inline comment or NatSpec to be clearer.

What numbers are we talking about when it comes to dust? Also, I see n divisions for distributing and 1 for burnings so I would guess there is more dust from distribution meaning we would be burning an excess amount? We can also try to improve the precision by adding a scale factor like we do in staker. It all really depends what the dust numbers look like

Addressed in 93b4daf: added explicit docs that burn() is called with burnAmount + dust (since dust is intentionally burned), and clarified the dust bound as <= distributors.length - 1.

alexkeating · 2025-12-17T15:50:47Z

  address internal admin;
  address internal emergencyAdmin;
  uint256 internal constant DEFAULT_BURN_BPS = 10_000;
+  uint256 internal constant BPS_DENOMINATOR = 10_000;


Why not reuse the constant above this?

They represent different concepts: DEFAULT_BURN_BPS is the default burn percentage (happens to be 100%), while BPS_DENOMINATOR is the denominator for basis point math (always 10,000). They have the same value but different semantics - using DEFAULT_BURN_BPS in calculations would be confusing since we're not dividing by the burn percentage.

alexkeating · 2025-12-17T15:51:50Z

+    splitter.setDistributors(_distributors);
+    vm.prank(admin);
+    splitter.setBurnPercentage(_burnBps);
+  }


I would split this into two methods and move to the base test contract. _addDistributors and _burn

Addressed in 42f2d93 - extracted _addDistributors and _setBurnBps helpers to the base SplitterTest contract.

alexkeating · 2025-12-17T15:52:19Z

+
+  function _mintToSplitter(uint256 _amount) internal {
+    splitToken.mint(address(splitter), _amount);
+  }


Is there an existign mint? We should probably reuse that.

Addressed in b4ec820 - moved _mintToSplitter to the base SplitterTest contract.

alexkeating · 2025-12-17T16:01:56Z

+    _setupDistributorAndBurn(_recipient, _weight, _burnBps);
+    _mintToSplitter(_amount);
+
+    uint256 _expectedBurn = (_amount * _burnBps) / BPS_DENOMINATOR;


We are testing the logic with the logic. If there is a bug in the logic we won't catch it.

Maybe have a couple tests with hardcoded weights

Addressed in 0c6c57f - added 5 concrete test cases with hardcoded expected values (e.g., 1000 tokens with 50% burn = 500 burned, 500 distributed).

We can probably remove these, with the hardcode comment I made

alexkeating · 2025-12-17T16:04:26Z

+    assertEq(splitToken.balanceOf(address(splitter)), 0);
+  }
+
+  function testFuzz_Split_100PercentBurn(uint256 _amount) public {


Make the names more sentence like so they read better in scopelint spec

Addressed in 255ec3f - renamed tests to be more sentence-like (e.g., testFuzz_Split_BurnsAllTokensWhenBurnIsOneHundredPercent).

…ntract

alexkeating · 2025-12-19T17:02:49Z

+
+    // Burn the burn amount + any dust from rounding
+    uint256 _totalBurned = _amount - _totalDistributed;
+    if (_totalBurned > 0) $._splitToken.burn(_totalBurned);


What numbers are we talking about when it comes to dust? Also, I see n divisions for distributing and 1 for burnings so I would guess there is more dust from distribution meaning we would be burning an excess amount? We can also try to improve the precision by adding a scale factor like we do in staker. It all really depends what the dust numbers look like

alexkeating · 2026-01-08T00:13:27Z

+    _weight2 = _boundWeight(_weight2);
+    // Ensure weights don't overflow when added
+    vm.assume(_weight1 <= type(uint96).max / 2);
+    vm.assume(_weight2 <= type(uint96).max / 2);


It is probably best to remove these assumes and rely on more flexible bounding for 786 and 787

alexkeating · 2026-01-08T00:24:28Z

+    assertEq(splitToken.balanceOf(address(splitter)), 0);
+  }
+
+  function testFuzz_Split_BurnsDustFromRounding(


This test doesn't test dust burning with a set burn percentage. We should test that case as well

Also, Isn't this case tested in the earlier tests?

Good call. Addressed in 2b90467: added an assume(_amount % 3 != 0) so the dust-rounding fuzz test always exercises the non-zero dust path (instead of occasionally duplicating the “no dust” cases).

alexkeating · 2026-01-08T00:25:37Z

+    splitter.split();
+
+    assertEq(splitToken.balanceOf(_recipient), _expectedDistribute);
+    assertEq(splitToken.balanceOf(address(splitter)), 0);


We don't assert that the burn happened. We should see total supply decrease by the burn

alexkeating · 2026-01-08T00:26:05Z

+
+    splitter.split();
+
+    assertEq(splitToken.balanceOf(_recipient), _expectedDistribute);


The recipient also could have a balance before the split

Also we don't need to assert this instead we could assert burned + distributed == amount where burned = totalSupplyBefore - totalSupplyAfter and distrbuted = recipientBalanceAfter - recipientBalanceBefore. This would avoid us having to recreate the logic of the tests

We should be checking these invariants in other tests as well

alexkeating · 2026-01-08T00:31:20Z

+    _setupDistributorAndBurn(_recipient, _weight, _burnBps);
+    _mintToSplitter(_amount);
+
+    uint256 _expectedBurn = (_amount * _burnBps) / BPS_DENOMINATOR;


We can probably remove these, with the hardcode comment I made

Thread: #31 (comment)

alexkeating · 2026-02-10T18:01:28Z

 }
+
+contract Split is SplitterTest {
+  function testFuzz_Split_DistributesToSingleDistributor(


We don't need the split in these tests

Addressed in 1755681: removed the redundant Split_ prefix from test names inside contract Split (e.g. testFuzz_Split_... -> testFuzz_...).

alexkeating · 2026-02-10T18:02:42Z

+    vm.prank(admin);
+    splitter.setDistributors(_distributors);
+    vm.prank(admin);
+    splitter.setBurnPercentage(0);


Can we use the setBurn bips helper here?

Addressed in 84015c3: switched the direct setBurnPercentage(...) call to the existing _setBurnBps(...) helper for consistency.

alexkeating · 2026-02-10T18:03:28Z

+    uint256 _burned = _totalSupplyBefore - splitToken.totalSupply();
+
+    assertEq(_distributed + _burned, _splitterBalanceBefore);
+    assertLt(_burned, 2);


Maye be clear to do lte 1 for this and other tests.

Addressed in dc4a1f3: tightened dust assertions from < 2 to <= 1 wei (and made the intent explicit in comments).

alexkeating · 2026-02-10T18:04:13Z

+    assertEq(splitToken.balanceOf(address(splitter)), 0);
+  }
+
+  function testFuzz_Split_DistributesAllTokensWhenBurnIsZeroPercent(


Don't we test this in the first 2 tests

Addressed in 963ceb4: removed the redundant 100% burn test since testFuzz_Split_NoDistributors already asserts the 100% burn + event path.

alexkeating · 2026-02-10T18:07:01Z

+    assertEq(splitToken.balanceOf(address(splitter)), 0);
+  }
+
+  function testFuzz_Split_EmitsEvent(


We should speciify the event being tested in the name

Addressed in 6f0749c: renamed the test to testFuzz_Split_EmitsSplitEvent to make it explicit which event is being asserted.

alexkeating · 2026-02-10T18:08:04Z

+    splitter.split();
+  }
+
+  function testFuzz_Split_ZeroBalance(address _recipient, uint256 _weight) public {


Good question, should we allow splitting with a 0 balance

Addressed in 2d6e4a7: split() is explicitly documented and tested as a no-op when the splitter balance is 0 (no state changes and no Split event emitted).

alexkeating · 2026-02-10T18:08:37Z

+    assertEq(splitToken.balanceOf(address(splitter)), 0);
+  }
+
+  function testFuzz_Split_NoDistributors(uint256 _amount) public {


Isn't this tested in the 100% burn test

Addressed in 433b17f: kept the no-distributors case as an explicit event-emission assertion for the “no distributors / 100% burn” branch, and renamed the test to make that purpose clear (...NoDistributors_EmitsSplitEventAndBurnsAll).

alexkeating · 2026-02-10T18:09:21Z

+    assertEq(splitToken.balanceOf(address(splitter)), 0);
+  }
+
+  function testFuzz_Split_AnyoneCanCall(


This should be in one of the default cases, I don't think we need this explicit test

Addressed in c3a44d9: removed the dedicated testFuzz_Split_AnyoneCanCall test and folded the permissionless-call assertion into testFuzz_Split_DistributesToSingleDistributor by fuzzing _caller and calling split() under vm.prank(_caller).

alexkeating · 2026-02-10T18:11:07Z

+  }
+
+  // Concrete test cases with hardcoded expected values
+  function test_Split_BurnsAndDistributesFiftyPercentEach() public {


I don't think we need these tests. We can fuzz the burn rate and assert the invariant we want to enforce. which is burn equals the burn percentage of the previous balance give or take 1 wei

Addressed in 90c057e: removed the concrete hardcoded split cases and replaced them with a single fuzz invariant testFuzz_Split_BurnedIsExpectedBurnPlusAtMostOneWei (2 distributors so dust <= 1 wei, assert burned is expectedBurn or expectedBurn + 1).

alexkeating · 2026-02-10T18:11:35Z

+    vm.prank(admin);
+    splitter.setDistributors(_distributors);
+    vm.prank(admin);
+    splitter.setBurnPercentage(_burnBps);


Can we reuse some helpers here?

Addressed in cf9628b: introduced _setDistributors(...) + _addTwoDistributors(...) helpers and refactored the 2-distributor dust/burn tests to use them (also switched those spots to _setBurnBps(...) for consistency).

github-actions · 2026-02-16T20:14:17Z

Coverage after merging feat/split-method into main will be

100.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines
src
FeeFlow.sol	100%	100%	100%	100%
Splitter.sol	100%	100%	100%	100%

marcomariscal force-pushed the feat/burn-percentage branch 2 times, most recently from d0ca8b9 to a88e08c Compare December 16, 2025 19:47

marcomariscal force-pushed the feat/split-method branch from f0f33bc to b3ca582 Compare December 16, 2025 20:08

marcomariscal marked this pull request as ready for review December 16, 2025 20:17

marcomariscal force-pushed the feat/split-method branch from a98b4a6 to d86c01a Compare December 16, 2025 20:19

marcomariscal linked an issue Dec 16, 2025 that may be closed by this pull request

Splitter Contract-03: Add split method #16

Closed

marcomariscal requested review from alexkeating December 16, 2025 20:22

marcomariscal force-pushed the feat/split-method branch from d86c01a to 278d63b Compare December 16, 2025 20:22

alexkeating reviewed Dec 17, 2025

View reviewed changes

marcomariscal force-pushed the feat/burn-percentage branch from 707d49b to 633a6a9 Compare December 23, 2025 03:57

marcomariscal force-pushed the feat/split-method branch from 255ec3f to a60c35c Compare December 23, 2025 04:07

marcomariscal changed the base branch from feat/burn-percentage to main January 6, 2026 18:04

marcomariscal added 7 commits January 6, 2026 10:07

feat: add split method to Splitter

3032a7c

refactor: simplify split to push-based pattern

312ea3f

style: use modern vm.expectEmit and add BPS_DENOMINATOR constant

0b8ef9f

refactor: extract _addDistributors and _setBurnBps helpers to base co…

26272b4

…ntract

refactor: move _mintToSplitter to base test contract

8183615

test: add concrete test cases with hardcoded expected values

bfd1d0d

style: rename split tests for better scopelint spec readability

aba48be

marcomariscal force-pushed the feat/split-method branch from a60c35c to aba48be Compare January 6, 2026 18:08

alexkeating reviewed Jan 8, 2026

View reviewed changes

marcomariscal added 8 commits January 8, 2026 09:32

test: drop redundant fuzz burn assertion

c4f6a27

Thread: #31 (comment)

test: assert split invariants via deltas

9161df6

Thread: #31 (comment)

test: assert burn via total supply delta

03128eb

Thread: #31 (comment)

test: document burn rounding bound

90eed4c

test: cover dust burn with nonzero burn bps

6ce3bff

Thread: #31 (comment)

test: remove weight overflow assumes

cd2e5d9

Thread: #31 (comment)

docs: clarify dust bound and burn accounting

722e689

Thread: #31 (comment)

style: scopelint fmt

fb25442

test: assert exact distributions

2a3635e

marcomariscal force-pushed the feat/split-method branch from fbe2edc to 2a3635e Compare January 16, 2026 18:16

marcomariscal requested a review from alexkeating January 16, 2026 18:30

alexkeating reviewed Feb 10, 2026

View reviewed changes

marcomariscal added 13 commits February 10, 2026 11:55

test: use _setBurnBps helper

84015c3

test: tighten dust bound to <= 1 wei

dc4a1f3

test: remove redundant 100% burn split test

963ceb4

test: ensure dust rounding fuzz always produces dust

2b90467

test: rename split event test

6f0749c

docs: document split() zero-balance no-op

2d6e4a7

test: clarify no-distributors split event test

433b17f

test: fold permissionless split into existing fuzz

c3a44d9

test: replace hardcoded split cases with burn invariant fuzz

90c057e

test: reuse two-distributor helpers

cf9628b

test: drop redundant Split_ prefix in Split suite

1755681

docs: clarify dust bound and burn includes dust

93b4daf

chore: scopelint fmt

4bd630c

marcomariscal requested a review from alexkeating February 11, 2026 00:19

Minor changes

c0a5087

alexkeating merged commit 120e5da into main Feb 16, 2026
5 checks passed


		splitter.split();

		assertEq(splitToken.balanceOf(_recipient), _expectedDistribute);

Conversation

marcomariscal commented Dec 16, 2025

Summary

Changes

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexkeating Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexkeating Feb 10, 2026 •

edited

Loading