feat: integrate feeTaker by vutt94 · Pull Request #2 · KyberNetwork/rfq-extension-sc

vutt94 · 2025-11-21T04:49:36Z

No description provided.

Copilot

Pull Request Overview

This PR integrates a FeeTaker contract that enables profit distribution from 1inch Limit Order Protocol orders. The feature allows takers to distribute trading profits to multiple whitelisted recipients according to configurable share percentages.

Key Changes:

New FeeTaker contract implementing IPostInteraction for automated profit distribution after order fills
Supporting infrastructure including ImmutableState base contract, CommonLibrary utilities, and IFeeTaker interface
Comprehensive test suite covering standard fills, partial fills, WETH unwrapping, and edge cases

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
`src/1inch/FeeTaker.sol`	Main contract implementing post-interaction profit distribution with whitelist validation and optional WETH unwrapping
`src/1inch/interfaces/IFeeTaker.sol`	Interface defining Distribution struct, events, and errors for the FeeTaker
`src/1inch/base/ImmutableState.sol`	Base contract providing immutable WETH reference and original contract address storage
`src/1inch/libraries/CommonLibrary.sol`	Utility library with token operations, delta arithmetic, and WETH wrapping/unwrapping functions
`test/1inch/FeeTaker.t.sol`	Comprehensive test suite with fuzz tests for various distribution scenarios
`test/1inch/Base.t.sol`	Updated base test infrastructure to support FeeTaker testing with additional addresses and extension building
`test/1inch/DecayCurveAmountGetter.sol`	Updated to accommodate new `_buildExtension` signature with three parameters
`remappings.txt`	Added remapping for 1inch dependencies
`.vscode/settings.json`	New IDE configuration for Solidity formatting and development settings

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-21T04:56:11Z

    takingAmountData = bytes.concat(bytes20(address(_amountGetter)), takingAmountData);
+    if (takingAmountData.length > 0) {
+      takingAmountData = bytes.concat(bytes20(address(_amountGetter)), takingAmountData);
+    }


Duplicate code that processes takingAmountData three times. Lines 124-126 already check and prepend the address, then line 127 unconditionally prepends it again, and lines 128-130 check and prepend it a third time. This will result in incorrect extension data with the address prepended multiple times. Remove lines 127-130 to fix this bug.

Suggested change

takingAmountData = bytes.concat(bytes20(address(_amountGetter)), takingAmountData);

if (takingAmountData.length > 0) {

takingAmountData = bytes.concat(bytes20(address(_amountGetter)), takingAmountData);

}

// Removed duplicate address prepending to takingAmountData

// Removed duplicate address prepending to takingAmountData

// Removed duplicate address prepending to takingAmountData

// Removed duplicate address prepending to takingAmountData

Copilot · 2025-11-21T04:56:12Z

+
+import {Address, AddressLib} from '1inch/solidity-utils/contracts/libraries/AddressLib.sol';
+
+import {CommonLibrary} from './libraries/CommonLibrary.sol';


The CommonLibrary is imported but never used in this contract. Consider removing this unused import to improve code clarity.

Suggested change

import {CommonLibrary} from './libraries/CommonLibrary.sol';

Copilot · 2025-11-21T04:56:12Z

+contract FeeTaker is
+  IPostInteraction,
+  IFeeTaker,
+  ManagementRescuable,
+  ManagementPausable,
+  ImmutableState
+{


Missing contract-level documentation. The FeeTaker contract should include NatSpec documentation explaining its purpose, how it integrates with the LimitOrderProtocol, and the role of the Distribution struct in profit distribution. This would help developers understand the contract's functionality.

Copilot · 2025-11-21T04:56:12Z

+
+    vm.recordLogs();
+
+    actualFillAmount = bound(actualFillAmount, takingAmount / makingAmount + 1, takingAmount);


Potential division by zero. When makingAmount is very small relative to takingAmount, the expression takingAmount / makingAmount could result in 0, making the lower bound 0 + 1 = 1. However, if takingAmount is 1 (which is excluded by line 113's lower bound of 1_000_000), this is likely safe. Still, the logic could be clearer. Consider using a more explicit minimum value or documenting why this bound is safe.

Copilot · 2025-11-21T04:56:12Z

+pragma solidity 0.8.30;
+
+import './Base.t.sol';
+import {console} from 'forge-std/console.sol';


The console import from forge-std is unused in this test file. Consider removing it to improve code clarity.

Suggested change

import {console} from 'forge-std/console.sol';

Copilot · 2025-11-21T04:56:14Z

+    address[] recipients;
+    uint256[] shareBps; // basis points (e.g., 10000 = 100%)
+    bool unwrapWeth;
+    address tail;


The tail field in the Distribution struct is defined but never used in the FeeTaker implementation. Consider either implementing its intended functionality, documenting it as reserved for future use, or removing it if it's not needed. Unused fields can confuse developers and add unnecessary complexity.

Suggested change

address tail;

Copilot · 2025-11-21T04:56:14Z

+    address _LIMIT_ORDER_PROTOCOL
+  ) ManagementBase(0, initialAdmin) ImmutableState(_WETH) {
+    _batchGrantRole(KSRoles.RESCUER_ROLE, initialRescuers);
+    WETH = IWETH(_WETH);


Duplicate assignment to WETH. The WETH immutable variable is already set in the ImmutableState constructor (line 13 of ImmutableState.sol), but it's being reassigned here. In Solidity, immutable variables can only be assigned once during construction. This will cause a compilation error.

Suggested change

WETH = IWETH(_WETH);

Copilot · 2025-11-21T04:56:14Z

+  constructor(
+    address initialAdmin,
+    address[] memory initialRescuers,
+    address _WETH,
+    address _LIMIT_ORDER_PROTOCOL
+  ) ManagementBase(0, initialAdmin) ImmutableState(_WETH) {
+    _batchGrantRole(KSRoles.RESCUER_ROLE, initialRescuers);
+    WETH = IWETH(_WETH);
+    LIMIT_ORDER_PROTOCOL = _LIMIT_ORDER_PROTOCOL;
+  }


Missing input validation for critical constructor parameters. The constructor should validate that _WETH and _LIMIT_ORDER_PROTOCOL are not zero addresses to prevent deployment with invalid configuration.

Copilot · 2025-11-21T04:56:15Z

+      uint256 remainingProfit = profitAmount;
+      for (uint256 i = 0; i < distribution.recipients.length; i++) {
+        if (!whitelistedRecipients[distribution.recipients[i]]) {
+          revert NotWhitelisted();
+        }
+


Inefficient whitelist validation in the distribution loop. The whitelist check is performed for each recipient during order execution (potentially hot path). Consider moving the whitelist validation to when the order is created or validating all recipients before starting the distribution loop to fail fast and save gas if any recipient is not whitelisted.

Suggested change

uint256 remainingProfit = profitAmount;

for (uint256 i = 0; i < distribution.recipients.length; i++) {

if (!whitelistedRecipients[distribution.recipients[i]]) {

revert NotWhitelisted();

}

// Validate all recipients are whitelisted before starting distribution

for (uint256 i = 0; i < distribution.recipients.length; i++) {

if (!whitelistedRecipients[distribution.recipients[i]]) {

revert NotWhitelisted();

}

}

uint256 remainingProfit = profitAmount;

for (uint256 i = 0; i < distribution.recipients.length; i++) {

Copilot · 2025-11-21T04:56:15Z

+  struct Distribution {
+    uint256 expectedAmount;
+    address[] recipients;
+    uint256[] shareBps; // basis points (e.g., 10000 = 100%)


Missing documentation for the Distribution struct. Add NatSpec comments explaining each field's purpose, especially the tail field which is not used in the current implementation but is part of the struct definition. Document the expected behavior when unwrapWeth is true and what the tail address is intended for.

Suggested change

struct Distribution {

uint256 expectedAmount;

address[] recipients;

uint256[] shareBps; // basis points (e.g., 10000 = 100%)

/**

* @dev Struct describing how profits are distributed among recipients.

* @param expectedAmount The total amount expected to be distributed.

* @param recipients The list of recipient addresses to receive shares of the profit.

* @param shareBps The share of each recipient in basis points (e.g., 10000 = 100%).

* @param unwrapWeth If true, WETH will be unwrapped to ETH before distribution.

* When true, recipients will receive ETH instead of WETH.

* @param tail An additional address for future extension or custom logic.

* Not used in the current implementation, but reserved for potential use.

*/

struct Distribution {

uint256 expectedAmount;

address[] recipients;

uint256[] shareBps;

minhtr09 · 2025-11-24T04:00:09Z

+  uint256 constant BPS = 10_000;
+  address private immutable LIMIT_ORDER_PROTOCOL;
+
+  mapping(address => bool) public whitelistedRecipients;


why dont we use RECIPIENT_ROLE here?

recipient dont have any privilege, using ROLE for it is a bit overkill. we just need to whitelist/unwhitelist so a mapping should be fine

feat: integrate feeTaker

c0297f8

vutt94 requested review from Copilot and minhtr09 November 21, 2025 04:49

Copilot started reviewing on behalf of vutt94 November 21, 2025 04:50 View session

Merge branch 'main' into jensen-add-feeTaker

e11c732

Copilot finished reviewing on behalf of vutt94 November 21, 2025 04:53

Copilot AI reviewed Nov 21, 2025

View reviewed changes

fix: merge conflicts

352f995

minhtr09 reviewed Nov 24, 2025

View reviewed changes

fix: remove transfer profit

bb48967

-    takingAmountData = bytes.concat(bytes20(address(_amountGetter)), takingAmountData);
-    if (takingAmountData.length > 0) {
-      takingAmountData = bytes.concat(bytes20(address(_amountGetter)), takingAmountData);
-    }
+    // Removed duplicate address prepending to takingAmountData
+    // Removed duplicate address prepending to takingAmountData
+    // Removed duplicate address prepending to takingAmountData
+    // Removed duplicate address prepending to takingAmountData


		import {Address, AddressLib} from '1inch/solidity-utils/contracts/libraries/AddressLib.sol';

		import {CommonLibrary} from './libraries/CommonLibrary.sol';


		vm.recordLogs();

		actualFillAmount = bound(actualFillAmount, takingAmount / makingAmount + 1, takingAmount);

Conversation

vutt94 commented Nov 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

minhtr09 Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

vutt94 Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants