Skip to content

Conversation

@phrwlk
Copy link

@phrwlk phrwlk commented Oct 31, 2025

The comment for the RoleGranted event incorrectly stated that for existing
members, the since parameter indicates "the execution delay", when in fact
it represents the timestamp when the execution delay update takes effect.

This correction aligns the documentation with:

  • The actual implementation in AccessManager._grantRole() which uses
    Delay.withUpdate() returning an effect timestamp
  • Test cases that verify since equals the timestamp when the delay update
    becomes effective (grantTimestamp + setback)
  • The consistent pattern used in similar events (RoleGrantDelayChanged,
    TargetAdminDelayUpdated) where since clearly indicates a timestamp

This fixes potential confusion for developers interpreting the event parameters.

@phrwlk phrwlk requested a review from a team as a code owner October 31, 2025 21:51
@changeset-bot
Copy link

changeset-bot bot commented Oct 31, 2025

⚠️ No Changeset found

Latest commit: ca960f7

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

Walkthrough

The documentation comment for the RoleGranted event in IAccessManager.sol was updated. Specifically, the description of the since parameter was modified to indicate that it represents the timestamp when the execution delay update takes effect, replacing the previous description that stated it was the execution delay for an account and roleId when the member is not new. No functional changes or signature modifications were made to the contract.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "fix: correct RoleGranted event documentation for existing members" is directly related to the main change in the changeset. The raw summary confirms that only the documentation comment for the RoleGranted event was updated, specifically regarding the since parameter description. The title is concise, specific, and clearly communicates the primary change without vague language or noise, allowing teammates to quickly understand the intent of the PR.
Description Check ✅ Passed The pull request description is clearly related to the changeset and provides meaningful information about the documentation correction. It explains what was incorrect in the RoleGranted event documentation (the since parameter description), what it should be, and provides supporting context by referencing the implementation, test cases, and similar event patterns. The description is neither vague nor off-topic; it directly addresses the documentation update made in the PR.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8631702 and e4f63cf.

📒 Files selected for processing (1)
  • contracts/access/manager/IAccessManager.sol (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Redirect rules - solidity-contracts
  • GitHub Check: Header rules - solidity-contracts
  • GitHub Check: Pages changed - solidity-contracts
  • GitHub Check: slither
  • GitHub Check: coverage
  • GitHub Check: tests-upgradeable
  • GitHub Check: tests-foundry
  • GitHub Check: tests
  • GitHub Check: halmos
🔇 Additional comments (1)
contracts/access/manager/IAccessManager.sol (1)

37-40: Doc clarification matches behavior.

Thanks for aligning the since description with the effect timestamp emitted by _grantRole; this matches the Delay.withUpdate semantics and existing tests.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@ernestognw ernestognw changed the base branch from master to typo-fixes November 3, 2025 17:23
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Thanks

@ernestognw ernestognw merged commit 9c0046f into OpenZeppelin:typo-fixes Nov 3, 2025
12 of 14 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.

2 participants