Chore/increase test coverage and add natspec #314

alex0207s · 2024-07-04T09:27:08Z

fix the logic in LimitOrderSwap.sol to not allow zero takerToken orders or zero makerToken orders
increase test coverage to near 100%; comment on the reasons why some branches cannot be covered.
remove dead codes from the Tokenlon contracts.
fix typos.
add a script to package.json for generating coverage - coverage.
add natspec to Tokenlon contracts (ongoing)

github-actions · 2024-07-04T09:28:21Z

Changes to gas cost

Generated at commit: 16235e6618ed731dfec3294cbd00dd4776643882, compared to commit: 019650b2eaf9c86d0e3fce3c04acfe001f5f4cd9

🧾 Summary (20% most significant diffs)

Contract	Method	Avg (+/-)	%
AllowanceTarget	spendFromUserTo unpause	+1,756 ❌ +2,113 ❌	+3.55% +9.02%

Full diff report 👇

Contract	Deployment Cost (+/-)	Method	Min (+/-)	%	Avg (+/-)	%	Median (+/-)	%	Max (+/-)	%	# Calls (+/-)
AllowanceTarget	613,454 (0)	pause spendFromUserTo unpause	23,422 (0) 25,029 (0) 23,423 (0)	0.00% 0.00% 0.00%	26,608 (+354) 51,264 (+1,756) 25,536 (+2,113)	+1.35% +3.55% +9.02%	27,671 (0) 49,849 (+13,708) 25,536 (+2,113)	0.00% +37.93% +9.02%	27,671 (0) 91,606 (0) 27,649 (+4,226)	0.00% 0.00% +18.04%	4 (+1) 8 (+1) 2 (+1)

github-actions · 2024-07-04T09:28:22Z

Changes to gas cost

Generated at commit: 16235e6618ed731dfec3294cbd00dd4776643882, compared to commit: 019650b2eaf9c86d0e3fce3c04acfe001f5f4cd9

🧾 Summary (20% most significant diffs)

Contract	Method	Avg (+/-)	%
LimitOrderSwap	fillLimitOrder fillLimitOrderGroup	-246,932 ✅ -68,112 ✅	-17.31% -28.05%

Full diff report 👇

Contract	Deployment Cost (+/-)	Method	Min (+/-)	%	Avg (+/-)	%	Median (+/-)	%	Max (+/-)	%	# Calls (+/-)
LimitOrderSwap	2,730,640 (+36,206)	fillLimitOrder fillLimitOrderFullOrKill fillLimitOrderGroup	36,094 (0) 48,304 (+42) 31,168 (-166,968)	0.00% +0.09% -84.27%	1,179,473 (-246,932) 104,056 (+34) 174,739 (-68,112)	-17.31% +0.03% -28.05%	161,770 (-6,889) 48,304 (+42) 198,266 (-47,337)	-4.08% +0.09% -19.27%	29,534,880 (+1) 215,562 (+19) 291,975 (+195)	+0.00% +0.01% +0.07%	28 (+5) 3 (0) 9 (+3)
RFQ	2,465,421 (+21,622)	cancelRFQOffer feeCollector fillRFQ setFeeCollector	23,996 (+22) 382 (-44) 31,822 (0) 23,793 (+22)	+0.09% -10.33% 0.00% +0.09%	37,923 (+22) 382 (-44) 131,171 (+1,527) 25,964 (+22)	+0.06% -10.33% +1.18% +0.08%	38,918 (+22) 382 (-44) 147,976 (0) 23,998 (+22)	+0.06% -10.33% 0.00% +0.09%	49,859 (+22) 382 (-44) 238,908 (+15,387) 30,102 (+22)	+0.04% -10.33% +6.88% +0.07%	4 (0) 2 (+1) 24 (+2) 3 (0)
SmartOrderStrategy	1,226,691 (0)	approveTokens executeStrategy	56,282 (0) 23,234 (0)	0.00% 0.00%	185,084 (+10,734) 123,683 (+3,118)	+6.16% +2.59%	268,806 (0) 90,796 (+51,578)	0.00% +131.52%	268,806 (0) 610,220 (0)	0.00% 0.00%	33 (+6) 18 (+3)
CoordinatedTaker	2,367,349 (+1,248)	approveTokens submitLimitOrderFill	27,095 (0) 33,103 (0)	0.00% 0.00%	169,261 (+3,043) 128,473 (+9)	+1.83% +0.01%	189,038 (0) 70,073 (0)	0.00% 0.00%	189,038 (0) 256,667 (+19)	0.00% +0.01%	15 (+2) 7 (0)
GenericSwap	1,756,522 (+22,941)	executeSwap executeSwapWithSig	32,322 (+45) 39,528 (+44)	+0.14% +0.11%	117,670 (+45) 168,429 (+44)	+0.04% +0.03%	115,374 (+45) 175,155 (+44)	+0.04% +0.03%	252,012 (+45) 283,880 (+44)	+0.02% +0.02%	12 (0) 4 (0)

NIC619 · 2024-07-05T03:01:40Z

contracts/LimitOrderSwap.sol

@@ -111,6 +110,8 @@ contract LimitOrderSwap is ILimitOrderSwap, Ownable, TokenCollector, EIP712, Ree

        // unwrap extra WETH in order to pay for ETH taker token and profit
        uint256 wethBalance = weth.balanceOf(address(this));
+        // the if statement cannot be covered by tests
+        // because the WETH withdrawal amount is always greater than the balance this contract has


because the WETH withdrawal amount is always greater than the balance this contract has

Doesn't this means the balance cannot cover the amount being withdrawn hence should revert?

How is wethBalance > wethToPay guaranteed? Can't I pass in orders with total weth taker amount (i.e., wethToPay) greater than the weth maker amount collected (i.e., wethBalance)?

I think the if statement here is just to prevent calling the withdraw function with a zero amount, instead of guaranteeing that wethBalance is greater than wethToPay. This is because we can craft a case to make wethBalance equal to wethToPay. However, we cannot craft a case to make wethToPay greater than wethBalance, as wethToPay is accumulated by (makingAmount * order.takerTokenAmount) / order.makerTokenAmount, and makingAmount is bounded by orderAvailableAmount.

The if statement can't cover it here due to the internal require statement implemented in the withdraw function of WETH, which we can't trigger under the condition wethBalance - wethToPay when wethBalance > wethToPay.

However, we cannot craft a case to make wethToPay greater than wethBalance, as wethToPay is accumulated by (makingAmount * order.takerTokenAmount) / order.makerTokenAmount, and makingAmount is bounded by orderAvailableAmount.

makingAmount and orderAvailableAmount only affect what percentage of order.takerTokenAmount. It can be 50% or 100% whatever, but order.takerTokenAmount is a user passed in parameter so I don't see why I can't pass in a large enough order.takerTokenAmount that will result in wethToPay > wethBalance.

Yeah, you're right. We could make wethToPay > wethBalance by passing a large order.takerTokenAmount. However, as I mentioned before, the if statement here is to avoid calling the withdraw function with a zero amount, rather than guaranteeing that wethBalance is always greater than wethToPay. Therefore, the if statement cannot be covered because we cannot satisfy the internal require statement in WETH.

I see. I misunderstood your comment. Then maybe move the comment into the if code block and rephrase a bit:

if (wethBalance > wethToPay) { // this if code block cannot be fully covered because WETH withdraw will always succeed as we have checked that wethBalance > wethToPay unchecked { weth.withdraw(wethBalance - wethToPay); } }

NIC619 · 2024-07-05T03:05:24Z

contracts/LimitOrderSwap.sol

        // get the quote of the fill
+        // orderAvailableAmount must be greater than 0 here, or it will be reverted by the _validateOrder function


Is this comment for coverage?

Nope, it just reminds developers or auditors of the implicit condition.

But why do you need a comment for it? You are not using unchecked to do the arithmetics.

It just helps me clarify what _makerTokenAmount could be if orderAvailableAmount is always greater than 0. However, we could actually use an unchecked block to wrap the arithmetic operations."

If it's for unchecked then no problem. Otherwise it might not help much as reviewers still have to go over the code to check the conditions themselves.

NIC619 · 2024-07-05T03:09:31Z

contracts/RFQ.sol

@@ -184,6 +187,8 @@ contract RFQ is IRFQ, Ownable, TokenCollector, EIP712 {
            weth.deposit{ value: amount }();
            weth.transfer(to, amount);
        } else {
+            // this branch cannot be covered because we cannot trigger the sendValue internal revert,


Maybe have the to be a contract and revert at receive or fallback?

Adding the test case cannot improve the branch coverage because we cannot trigger the internal error AddressInsufficientBalance here.

Suggested change

// this branch cannot be covered because we cannot trigger the sendValue internal revert,

// this branch cannot be covered because we cannot trigger the AddressInsufficientBalance error in sendValue,

NIC619 · 2024-07-12T09:16:38Z

contracts/AllowanceTarget.sol

 contract AllowanceTarget is IAllowanceTarget, Pausable, Ownable {
    using SafeERC20 for IERC20;

+    /// @notice Mapping of authorized addresses permitted to call spendFromUserTo.
+    /// @dev Tracks the authorization status of each address to execute the spendFromUserTo function.


The @dev comment is basically the same as @notice. In this case we can omit it.

NIC619 · 2024-07-12T09:16:52Z

contracts/AllowanceTarget.sol

 contract AllowanceTarget is IAllowanceTarget, Pausable, Ownable {
    using SafeERC20 for IERC20;

+    /// @notice Mapping of authorized addresses permitted to call spendFromUserTo.
+    /// @dev Tracks the authorization status of each address to execute the spendFromUserTo function.
    mapping(address => bool) public authorized;


Suggested change

mapping(address => bool) public authorized;

mapping(address trustedCaller => bool isAuthorized) public authorized;

NIC619 · 2024-07-12T09:17:37Z

contracts/AllowanceTarget.sol

    mapping(address => bool) public authorized;

+    /// @notice Constructor to initialize the contract with the owner and trusted callers.
+    /// @dev Sets up the initial contract owner and authorizes a list of trusted callers.


NIC619 · 2024-07-12T09:17:57Z

contracts/AllowanceTarget.sol

    constructor(address _owner, address[] memory trustedCaller) Ownable(_owner) {
        uint256 callerCount = trustedCaller.length;
        for (uint256 i = 0; i < callerCount; ++i) {
            authorized[trustedCaller[i]] = true;
        }
    }

+    /// @notice Pauses the contract, preventing the execution of spendFromUserTo.
+    /// @dev Pauses the Contract. Only the owner can call this function.


Suggested change

/// @dev Pauses the Contract. Only the owner can call this function.

/// @dev Only the owner can call this function.

NIC619 · 2024-07-12T09:19:13Z

contracts/AllowanceTarget.sol

    function unpause() external onlyOwner {
        _unpause();
    }

+    /// @notice Transfers tokens from a user to a specified address.
+    /// @dev Transfers `amount` of `token` from `from` to `to`. The caller must be authorized.


Suggested change

/// @dev Transfers `amount` of `token` from `from` to `to`. The caller must be authorized.

/// @dev The caller must be authorized.

NIC619 · 2024-07-12T10:10:45Z

contracts/GenericSwap.sol

+    /// @param swapData The swap data containing details of the swap.
+    /// @param takerTokenPermit The permit for the taker token.
+    /// @return returnAmount The output amount of the swap.
+    /// @inheritdoc IGenericSwap


Ditto. Why have natspec here too?

NIC619 · 2024-07-12T10:10:58Z

contracts/GenericSwap.sol

+    /// @param taker The address of the taker.
+    /// @param takerSig The signature of the taker.
+    /// @return returnAmount The output amount of the swap.
+    /// @inheritdoc IGenericSwap


NIC619 · 2024-07-12T10:11:42Z

contracts/GenericSwap.sol

-    /// @param taker Claimed taker address
-    /// @param takerSig Taker signature
-    /// @return returnAmount Output amount of the swap
+    /// @notice Executes a generic swap using predefined strategies with a signature.


Suggested change

/// @notice Executes a generic swap using predefined strategies with a signature.

/// @notice Executes a generic swap using predefined strategies with taker's signature.

And you can add a explanation for why is takerSig needed in this case using @notice or @dev.

NIC619 · 2024-07-12T10:12:50Z

contracts/GenericSwap.sol

+    /// @notice Executes a generic swap.
+    /// @dev Internal function to handle the swap execution.


Ditto. They can be omitted.

NIC619 · 2024-07-12T10:13:03Z

contracts/GenericSwap.sol

@@ -78,6 +101,12 @@ contract GenericSwap is IGenericSwap, TokenCollector, EIP712 {
        _outputToken.transferTo(_swapData.recipient, returnAmount);
    }

+    /// @notice Emits the Swap event after executing a generic swap.
+    /// @dev Internal function to emit the Swap event.


NIC619 · 2024-07-16T00:44:31Z

My bad. I accidentally hit the merge button.

contracts/CoordinatedTaker.sol

contracts/interfaces/ICoordinatedTaker.sol

NIC619 · 2024-07-16T01:34:20Z

contracts/interfaces/ICoordinatedTaker.sol

@@ -5,25 +5,59 @@ import { LimitOrder } from "../libraries/LimitOrder.sol";

 /// @title ICoordinatedTaker Interface
 /// @author imToken Labs
+/// @notice This interface defines the functions and events for a coordinated taker contract.
+/// @dev The contract handles limit order fills with additional coordination parameters.


This comment reads like a limit order swap contract that has additional parameters for "coordination". Then, what does "coordination" mean? What's the difference between this and a limit order swap contract?

contracts/interfaces/ICoordinatedTaker.sol

contracts/interfaces/IWETH.sol

contracts/libraries/Asset.sol

NIC619 · 2024-07-16T02:12:44Z

@alex0207s Sorry I accidentally merged the PR. Please open a new PR and applies the feedbacks above.

chore: increase test coverage to near 100%

ad3db9b

alex0207s requested review from 108356037 and NIC619 July 4, 2024 09:27

alex0207s requested a review from shawnbc July 4, 2024 09:32

alex0207s changed the base branch from master to chore/cleanup-and-upgrade-codebase July 4, 2024 09:43

NIC619 reviewed Jul 5, 2024

View reviewed changes

alex0207s added 2 commits July 8, 2024 17:55

refine code based on PR comments

76f72c1

include NatSpec comments in contracts, libraries and interfaces

d5fea53

NIC619 reviewed Jul 12, 2024

View reviewed changes

refine code base on PR comments

3c437d6

NIC619 merged commit 25d088e into chore/cleanup-and-upgrade-codebase Jul 16, 2024
2 checks passed

NIC619 reviewed Jul 16, 2024

View reviewed changes

alex0207s deleted the chore/increase-test-coverage-and-add-natspec branch July 16, 2024 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore/increase test coverage and add natspec #314

Chore/increase test coverage and add natspec #314

alex0207s commented Jul 4, 2024

github-actions bot commented Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 4, 2024 •

edited

Loading

NIC619 Jul 5, 2024

NIC619 Jul 5, 2024

alex0207s Jul 5, 2024

NIC619 Jul 8, 2024

alex0207s Jul 8, 2024

NIC619 Jul 8, 2024

NIC619 Jul 5, 2024

alex0207s Jul 5, 2024

NIC619 Jul 8, 2024

alex0207s Jul 8, 2024

NIC619 Jul 8, 2024

NIC619 Jul 5, 2024

alex0207s Jul 5, 2024

NIC619 Jul 8, 2024

alex0207s Jul 8, 2024

NIC619 Jul 12, 2024

NIC619 Jul 12, 2024

NIC619 Jul 12, 2024

NIC619 Jul 12, 2024

NIC619 Jul 12, 2024

NIC619 Jul 12, 2024

NIC619 Jul 12, 2024

NIC619 Jul 12, 2024

NIC619 Jul 12, 2024

NIC619 Jul 12, 2024

NIC619 Jul 12, 2024

NIC619 commented Jul 16, 2024

NIC619 Jul 16, 2024

NIC619 commented Jul 16, 2024

		// get the quote of the fill
		// orderAvailableAmount must be greater than 0 here, or it will be reverted by the _validateOrder function

	// this branch cannot be covered because we cannot trigger the sendValue internal revert,
	// this branch cannot be covered because we cannot trigger the AddressInsufficientBalance error in sendValue,

	mapping(address => bool) public authorized;
	mapping(address trustedCaller => bool isAuthorized) public authorized;

	/// @dev Pauses the Contract. Only the owner can call this function.
	/// @dev Only the owner can call this function.

	/// @dev Transfers `amount` of `token` from `from` to `to`. The caller must be authorized.
	/// @dev The caller must be authorized.

	/// @notice Executes a generic swap using predefined strategies with a signature.
	/// @notice Executes a generic swap using predefined strategies with taker's signature.

		/// @notice Executes a generic swap.
		/// @dev Internal function to handle the swap execution.

Chore/increase test coverage and add natspec #314

Chore/increase test coverage and add natspec #314

Conversation

alex0207s commented Jul 4, 2024

github-actions bot commented Jul 4, 2024 • edited Loading

Changes to gas cost

🧾 Summary (20% most significant diffs)

github-actions bot commented Jul 4, 2024 • edited Loading

Changes to gas cost

🧾 Summary (20% most significant diffs)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NIC619 commented Jul 16, 2024

Choose a reason for hiding this comment

NIC619 commented Jul 16, 2024

github-actions bot commented Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 4, 2024 •

edited

Loading