Skip to content

fix(tokens): l-08 misleading comments#437

Merged
0xisk merged 2 commits intomainfrom
fix/l-08
Apr 10, 2026
Merged

fix(tokens): l-08 misleading comments#437
0xisk merged 2 commits intomainfrom
fix/l-08

Conversation

@0xisk
Copy link
Copy Markdown
Member

@0xisk 0xisk commented Apr 9, 2026

Types of changes

What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Fix #433

PR Checklist

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Summary by CodeRabbit

  • Documentation
    • Updated documentation for token minting functionality to ensure consistency with function signatures.
    • Corrected wording in token approval documentation for improved clarity.

@0xisk 0xisk self-assigned this Apr 9, 2026
@0xisk 0xisk added the audit Issues reported by an audit label Apr 9, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 9, 2026

Walkthrough

Two Compact contract files received documentation updates correcting misleading or incomplete parameter descriptions in circuit documentation comments. No executable logic, control flow, or functional behavior was modified.

Changes

Cohort / File(s) Summary
Token Contract Documentation Fixes
contracts/src/token/FungibleToken.compact, contracts/src/token/NonFungibleToken.compact
Updated circuit documentation: corrected parameter name reference in _mint from to to account, and fixed grammatical error in _approve docstring by adding missing "of" in phrase about token ownership.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A comment's typo caught the eye,
Where words were wrong and phrases dry,
We fixed each phrase with gentle care,
Now clarity floats through the air! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR addresses two of four issues from #433: fixing the FungibleToken._mint docstring parameter name and the NonFungibleToken._approve docstring wording; however, the error message and Bytes8._toUint64 comment issues remain unresolved. Address the remaining two misleading comments: fix the NonFungibleToken error message mismatch and the Bytes8._toUint64 title comment to fully resolve audit finding L-08.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'fix(tokens): l-08 misleading comments' directly relates to the main changes—fixing misleading comments in FungibleToken and NonFungibleToken as identified in audit finding L-08.
Out of Scope Changes check ✅ Passed All changes in the PR are directly in-scope, addressing specific misleading comments identified in issue #433 with no extraneous modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/l-08

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

@0xisk 0xisk moved this from Backlog to Needs Review in OZ Development for Midnight Apr 9, 2026
Copy link
Copy Markdown
Contributor

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

LGTM! I'd cross out, delete, or address the nft comment in the issue since it's okay to disregard (I was wondering why it wasn't addressed in here)

Also, I suggest not using "fixes" bc the _toUint64 comment is on the other repo and merging this would close the issue

@0xisk 0xisk requested review from a team as code owners April 10, 2026 10:55
@0xisk
Copy link
Copy Markdown
Member Author

0xisk commented Apr 10, 2026

LGTM! I'd cross out, delete, or address the nft comment in the issue since it's okay to disregard (I was wondering why it wasn't addressed in here)

Also, I suggest not using "fixes" bc the _toUint64 comment is on the other repo and merging this would close the issue

The _toUint64 fix will be covered in the N-01

@0xisk 0xisk merged commit 736e289 into main Apr 10, 2026
9 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Complete/Stable Release in OZ Development for Midnight Apr 10, 2026
@0xisk 0xisk deleted the fix/l-08 branch April 10, 2026 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit Issues reported by an audit

Projects

Status: Complete/Stable Release

Development

Successfully merging this pull request may close these issues.

L-08: Misleading Comments

2 participants