Skip to content

standardize internal function names#43

Closed
ely67 wants to merge 3 commits intoPerfect-Abstractions:mainfrom
ely67:refactor/internal-function-names
Closed

standardize internal function names#43
ely67 wants to merge 3 commits intoPerfect-Abstractions:mainfrom
ely67:refactor/internal-function-names

Conversation

@ely67
Copy link
Copy Markdown

@ely67 ely67 commented Oct 20, 2025

Summary

Refactor internal function names to use an underscore prefix across facets and libraries (ERC20, ERC721, ERC173). Improves readability and follows common Solidity style. No external ABI changes, no storage layout changes.

Copy link
Copy Markdown
Collaborator

@adamgall adamgall left a comment

Choose a reason for hiding this comment

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

lgtm!

/// @param _tokenId The token ID to approve.
function approve(address _to, uint256 _tokenId) external {
<<<<<<< HEAD
ERC721Storage storage s = _getStorage();
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ok

@mudgen
Copy link
Copy Markdown
Contributor

mudgen commented Oct 24, 2025

Hi guys, I think that most internal functions (not including getStorage()) in Compose will come from libraries like LibERC20.sol. These libraries are intended to be used by users of Compose when they write their own custom facets that integrate with our facets (like ERC20Facet). So these internal functions will already be prefixed with the library name when they are used, for example: LibERC20.transferFrom.

Because of how we are mainly using internal functions, I do not want to prefix them with "_".

In Compose internal functions within facets (not libraries) are prefixed with "internal", but there should be none or few internal functions within facets.

One of the things we need is a STYLE.md file that provides a concise style guide of Compose based on ERC20Facet.sol, LibERC20.sol, ERC721Facet and LibERC721.sol.

@mudgen mudgen closed this Oct 24, 2025
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.

3 participants