Skip to content

Conversation

sashaodessa
Copy link

Problem:
The _flashFee function in ERC20FlashMint was using unused parameter suppression by referencing token and value variables without using them. This approach adds unnecessary bytecode and creates misleading code that suggests these parameters are used.

Solution:

  • Removed unused parameter references (token; and value;)
  • Replaced parameter names with anonymous placeholders (address, uint256)
  • Maintained the same functionality while reducing bytecode size

Benefits:

  • Cleaner, more readable code
  • Reduced bytecode size (no dead code)
  • Modern Solidity best practices for unused parameters

@sashaodessa sashaodessa requested a review from a team as a code owner October 9, 2025 14:19
Copy link

changeset-bot bot commented Oct 9, 2025

⚠️ No Changeset found

Latest commit: f8fa15b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

coderabbitai bot commented Oct 9, 2025

Walkthrough

The change updates contracts/token/ERC20/extensions/ERC20FlashMint.sol by modifying the internal helper function signature from function _flashFee(address token, uint256 value) to function _flashFee(address, uint256). Only the first parameter’s name is removed; types, visibility, and implementation remain the same. The function still returns 0 and exhibits no control-flow or error-handling changes. There are no alterations to public or exported interfaces.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly identifies the refactor of the ERC20FlashMint contract’s _flashFee function and specifies that unused parameter suppression is being removed, directly reflecting the main change described in the pull request. It is concise, specific, and free of extraneous details, making the intent immediately clear to reviewers.
Description Check ✅ Passed The pull request description directly addresses the change to the _flashFee function by explaining the problem of unused parameters, the exact modifications made, and the resulting benefits, which aligns precisely with the content of the changeset. It is clearly related to the code modifications and provides sufficient context without being off-topic.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
contracts/token/ERC20/extensions/ERC20FlashMint.sol (1)

67-77: Fix NatSpec documentation to match function signature.

The pipeline failure indicates that the NatSpec documentation at lines 71-72 still references @param token and @param value, but the function signature at line 75 no longer has named parameters. This causes a documentation mismatch error.

Apply this diff to fix the documentation:

     /**
      * @dev Returns the fee applied when doing flash loans. By default this
      * implementation has 0 fees. This function can be overloaded to make
      * the flash loan mechanism deflationary.
-     * @param token The token to be flash loaned.
-     * @param value The amount of tokens to be loaned.
      * @return The fees applied to the corresponding flash loan.
      */
     function _flashFee(address, uint256) internal view virtual returns (uint256) {
         return 0;
     }
🧹 Nitpick comments (1)
contracts/token/ERC20/extensions/ERC20FlashMint.sol (1)

75-75: Reconsider removing parameter names from virtual functions.

While anonymous parameters are valid for unused parameters, this function is internal view virtual and explicitly documented (lines 68-70) to be overridden by child contracts to implement custom fee logic. Removing parameter names reduces clarity for implementers who need to understand what the parameters represent without referring to the caller or interface documentation.

Consider keeping the parameter names for better developer experience:

-    function _flashFee(address, uint256) internal view virtual returns (uint256) {
+    function _flashFee(address token, uint256 value) internal view virtual returns (uint256) {
         return 0;
     }

If the goal is to avoid unused-parameter warnings in older Solidity versions, modern compilers (0.8.0+) already handle this gracefully without requiring suppression statements like token; or value;.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45558b8 and 89c374f.

📒 Files selected for processing (1)
  • contracts/token/ERC20/extensions/ERC20FlashMint.sol (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: formal verification
contracts/token/ERC20/extensions/ERC20FlashMint.sol

[error] 67-67: Documented parameter "token" not found in the parameter list of the function. Documented parameter "value" not found in the parameter list of the function.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: slither

Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

The CI was failing because the @param NatSpec tags were referencing unnamed arguments. I removed them since our documentation doesn't render them anyway.

LGTM but it would need a second approval

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants