Skip to content

fix: add braces to one-line if statements in ERC721EnumerableFacet#242

Merged
mudgen merged 1 commit intoPerfect-Abstractions:mainfrom
panditdhamdhere:fix/one-line-if-statements-braces
Dec 5, 2025
Merged

fix: add braces to one-line if statements in ERC721EnumerableFacet#242
mudgen merged 1 commit intoPerfect-Abstractions:mainfrom
panditdhamdhere:fix/one-line-if-statements-braces

Conversation

@panditdhamdhere
Copy link
Collaborator

@panditdhamdhere panditdhamdhere commented Dec 5, 2025

Fix style guide violation by adding braces around one-line if statements that call revert. This ensures compliance with Compose's coding standards which require all if statements to use braces.

Fixes two instances in safeTransferFrom functions (lines 331 and 354).

Summary

This PR fixes a style guide violation in ERC721EnumerableFacet.sol where two one-line if statements were missing braces around their revert calls. According to Compose's style guide (STYLE.md and banned-solidity-features.mdx), all if statements must use braces, even for single-line statements.

Changes Made

  • Fixed line 331: Added braces around if (reason.length == 0) revert ERC721InvalidReceiver(_to); in the safeTransferFrom(address, address, uint256) function
  • Fixed line 354: Added braces around if (reason.length == 0) revert ERC721InvalidReceiver(_to); in the safeTransferFrom(address, address, uint256, bytes) function
  • Code formatting: Ran forge fmt to ensure proper formatting

Both instances were in the error handling catch blocks of the safe transfer functions, where empty revert reasons are checked before propagating the original revert reason.

Checklist

  • Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions, using for directives, or selfdestruct

  • Code follows Design Principles - Readable, uses diamond storage, favors composition over inheritance

  • Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)

  • Code is formatted with forge fmt - ✅ Formatted with forge fmt

  • Existing tests pass - ✅ All 38 tests in ERC721EnumerableFacet.t.sol pass

  • New tests are optional - N/A - This is a style fix, no functional changes

  • All tests pass - ✅ Verified with forge test --match-path "test/token/ERC721/ERC721Enumerable/ERC721EnumerableFacet.t.sol" (38 tests passed)

  • Documentation updated - N/A - No documentation changes needed for style fixes

Additional Notes

  • This is a pure style fix with no functional changes
  • The fix ensures compliance with the style guide rule: "No single if statements without braces" (from banned-solidity-features.mdx)
  • All existing tests continue to pass, confirming no behavioral changes
  • The change improves code consistency with the rest of the codebase

Fix style guide violation by adding braces around one-line if statements
that call revert. This ensures compliance with Compose's coding standards
which require all if statements to use braces.

Fixes two instances in safeTransferFrom functions (lines 331 and 354).
@netlify
Copy link

netlify bot commented Dec 5, 2025

👷 Deploy request for compose-diamonds pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 3187148

@github-actions
Copy link

github-actions bot commented Dec 5, 2025

Coverage Report

Coverage

Metric Coverage Details
Lines 88% 1455/1650 lines
Functions 91% 318/351 functions
Branches 69% 161/233 branches

Last updated: Fri, 05 Dec 2025 07:27:21 GMT for commit 3187148

@github-actions
Copy link

github-actions bot commented Dec 5, 2025

Gas Report

No gas usage changes detected between main and fix/one-line-if-statements-braces.

All functions maintain the same gas costs. ✅

Last updated: Fri, 05 Dec 2025 07:28:11 GMT for commit 3187148

@mudgen mudgen merged commit 3187148 into Perfect-Abstractions:main Dec 5, 2025
5 checks passed
@mudgen
Copy link
Contributor

mudgen commented Dec 5, 2025

@panditdhamdhere Thanks for this!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants