feat: add Splitter contract with burn/distribute and FeeFlow integration#32
Conversation
d86c01a to
278d63b
Compare
914eb97 to
e8e3352
Compare
e8e3352 to
bdd8ffd
Compare
255ec3f to
a60c35c
Compare
bdd8ffd to
ed7df42
Compare
a60c35c to
aba48be
Compare
ed7df42 to
3482186
Compare
fbe2edc to
2a3635e
Compare
| - name: Run fork tests | ||
| env: | ||
| ZKSYNC_MAINNET_RPC_URL: ${{ secrets.ZKSYNC_MAINNET_RPC_URL }} | ||
| run: forge test --match-contract FeeFlowForkTest --zksync |
There was a problem hiding this comment.
We should probably split this by calling the integration test file rather than the contract
There was a problem hiding this comment.
Removed the fork-test split and the match/no-match plumbing by dropping the fork suite and consolidating on the mock integration tests. CI now just runs forge test.
Commit: 085b691
|
|
||
| - name: Run coverage | ||
| run: forge coverage --report summary --report lcov | ||
| run: forge coverage --no-match-contract FeeFlowForkTest --report summary --report lcov |
There was a problem hiding this comment.
Why exclude these from coverage? If it is taking too long lower the fuzz runs really low
There was a problem hiding this comment.
Only for this step though
There was a problem hiding this comment.
Fork tests are removed, so we no longer exclude them from coverage/test runs.
Also re your follow-up ("Only for this step though"): since we removed the fork suite/job entirely, there isn't any step-specific special-casing left to scope.
Commit: 085b691
| import {ERC1967Proxy} from "lib/openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol"; | ||
|
|
||
| /// @title Deploy | ||
| /// @notice Deployment script for FeeFlow and Splitter contracts. |
There was a problem hiding this comment.
We probably want more detailed natspec
There was a problem hiding this comment.
Expanded NatSpec and refactored Deploy script to a MultiGov-style shape: run() loads env config via an internal helper and passes it to a shared deploy function.
Commit: 8d3315b
| /// @notice Deploys the Splitter and FeeFlow contracts with proxies. | ||
| /// @param _params The deployment parameters. | ||
| /// @return _result The deployed contract instances. | ||
| function deploy(DeploymentParams memory _params) |
There was a problem hiding this comment.
Why not put this in the run? I recommend looking at the deploy scripts for something like multigov where we define the configuration in an internal function and pass it to a shared run
There was a problem hiding this comment.
Refactored to match the MultiGov pattern: run() now calls an internal config builder and then passes the params to a shared deploy routine.
Commit: 8d3315b
| /// @notice Sets up a claimer with ZK tokens and approval. | ||
| /// @param _claimer The claimer address. | ||
| /// @param _amount The amount of tokens to give and approve. | ||
| function _setupClaimer(address _claimer, uint256 _amount) internal { |
There was a problem hiding this comment.
| function _setupClaimer(address _claimer, uint256 _amount) internal { | |
| function _mintAndApproveZkTokens(address _claimer, uint256 _amount) internal { |
There was a problem hiding this comment.
Removed IntegrationTestBase along with the fork tests, so this suggestion is no longer applicable.
Commit: 085b691
| feeFlow.setClaimableToken(IERC20(address(feeToken)), true); | ||
| } | ||
|
|
||
| function test_Fork_FullFlow_ClaimTriggersSplitAndBurn() public { |
There was a problem hiding this comment.
We don't need fork here, and prefer fuzz tests
There was a problem hiding this comment.
Agreed. Dropped the fork tests and consolidated on the mock integration tests (+ fuzz) so we don't depend on forking/mainnet RPC.
Commit: 085b691
| ); | ||
| } | ||
|
|
||
| function test_Fork_FullFlow_ZeroPercentBurn() public { |
There was a problem hiding this comment.
Removed the fork suite as part of consolidating on the mock integration tests.
Commit: 085b691
|
|
||
| /// @title FeeFlow Fork Tests | ||
| /// @notice Fork tests against ZKsync Era mainnet with real ZK token. | ||
| contract FeeFlowForkTest is IntegrationTestBase { |
There was a problem hiding this comment.
It would be a little clearer if these were structured the same way as the unit tests
There was a problem hiding this comment.
Removed the fork-based FeeFlow suite and consolidated on the mock integration tests (plus fuzz coverage), so the integration tests are structured like the unit tests and don't require forking/ZKsync RPC setup.
Commit: 085b691
| /// @dev These tests use mock tokens to verify the full integration flow. | ||
| /// For fork tests against ZKsync Era mainnet, use foundry-zksync: | ||
| /// https://github.com/matter-labs/foundry-zksync | ||
| contract FeeFlowIntegrationTest is Test { |
There was a problem hiding this comment.
To me the integration and fork tests can be thought of as the same thing
There was a problem hiding this comment.
Agreed. Consolidated by removing the fork suite and keeping a single mock-based integration suite with fuzz coverage.
Commit: 085b691
|
Coverage after merging feat/feeflow-split into feat/split-method will be
Coverage Report
|
|||||||||||||||||||||||||
|
@alexkeating quick question on burn ABI assumptions. On ZKsync Sepolia, the ZK token at We currently model the bid token as Do you want |
Got confused looking at the sepolia zk contract and the abi/contract is different from actual zksync mainnet ZK token. No action needed here. |
* Add Splitter contract with burn/distribute and FeeFlow integration
Summary
claim()automatically callssplit()on the destinationTest plan
ZKSYNC_MAINNET_RPC_URLsecret)