Skip to content

Conversation

@CodeCryptX
Copy link
Contributor

Description

The toString function in ERC721Facet.sol is intended to convert a uint256 to its decimal string representation, but it incorrectly defines _SYMBOLS as:

bytes16 _SYMBOLS = "0123456789abcdef";

The characters 'a'..'f' are never used in the decimal conversion logic, making the code misleading.

Changes Made:

  • Updated _SYMBOLS to only include decimal characters:
bytes16 _SYMBOLS = "0123456789";

Reason:

  • Matches function documentation (decimal conversion).
  • Improves readability and adherence to Compose principles (clarity, simplicity).
  • No functional change, purely a clarity/maintenance improvement.

Test Plan:

  • Verified that toString continues to return correct decimal string outputs for several test values (e.g., 0, 1, 123, 987654321).
  • No regressions expected since logic is unchanged.

@mudgen
Copy link
Contributor

mudgen commented Oct 22, 2025

Thanks, I appreciate you finding this and making a pull request about it.
I understand what you are saying. I am not going to change this code for now

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

2 participants