Skip to content

Fix comments in DiamondUpgradeFacet - #391

Merged
mudgen merged 1 commit into
mainfrom
fix-diamond-upgrade-comments
Aug 23, 2026
Merged

Fix comments in DiamondUpgradeFacet#391
mudgen merged 1 commit into
mainfrom
fix-diamond-upgrade-comments

Conversation

@mudgen

@mudgen mudgen commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes several inaccurate code comments in src/diamond/DiamondUpgradeFacet.sol. Comment-only change; no code behavior is affected.

Changes Made

  • Corrected the pointer-calculation explanation: ptr = selectors + 32 + (4 * index) (was incorrectly selectorsLength + ...)
  • Fixed the addFacetSelectors return-value description (it returns the number of selectors processed, not excluding the first)
  • Corrected packedSelectorsimportSelectors in the free-memory-pointer comment
  • Corrected facetList.firstFacetNodeIdfacetList.headFacetNodeId
  • Fixed duplicated step numbering (3, 3, 4, 5 → 3, 4, 5, 6) in the facet loop description

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
  • Existing tests pass - Comment-only change; no code affected
  • New tests are optional - Not applicable (comment-only)
  • All tests pass
  • Documentation updated - Not applicable
  • Changesets — Not needed; comment-only change, no published behavior affected

Additional Notes

None.

@changeset-bot

changeset-bot Bot commented Aug 23, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 460250c

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

@netlify

netlify Bot commented Aug 23, 2026

Copy link
Copy Markdown

Deploy Preview for compose-diamonds canceled.

Name Link
🔨 Latest commit 460250c
🔍 Latest deploy log https://app.netlify.com/projects/compose-diamonds/deploys/6a8b7b9c51610e0008523dd8

@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

Coverage

Metric Coverage Details
Lines 85% 2063/2419 lines
Functions 90% 560/621 functions
Branches 89% 207/233 branches

Last updated: Sun, 23 Aug 2026 23:02:10 GMT for commit 460250c

@github-actions

Copy link
Copy Markdown
Contributor

Gas Report

No gas usage changes detected between main and fix-diamond-upgrade-comments.

All functions maintain the same gas costs. ✅

Last updated: Sun, 23 Aug 2026 23:02:36 GMT for commit 460250c

@mudgen
mudgen merged commit f9c3b0c into main Aug 23, 2026
10 checks passed
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.

1 participant