Revert with supplied proposalId for unknown proposals in GovernorStorage

[sashass1315]

The queue/execute/cancel id-only wrappers in GovernorStorage delegated with empty details for unknown proposal IDs, causing GovernorNonexistentProposal to be raised with a computed id that doesn’t match the caller-supplied proposalId. Add an existence check using descriptionHash == 0 and revert with GovernorNonexistentProposal(proposalId) before delegating. This keeps the storage access pattern and aligns revert parameters with the input id for better UX and tooling.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 569bdb2

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]

Walkthrough

The changes add runtime validation checks to the queue, execute, and cancel functions in the Governor storage extension. These checks verify that a proposal exists by examining the descriptionHash field in proposal details. When a proposal is not found (descriptionHash equals 0), the functions revert with a GovernorNonexistentProposal error. The validation is performed before delegating to existing logic and replaces implicit assumptions about proposal existence with explicit guard clauses.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is directly related to the changeset, explaining the issue with empty details and the solution of adding an existence check using descriptionHash == 0.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title clearly summarizes the main change: adding validation to revert with the supplied proposalId for unknown proposals in GovernorStorage, which directly matches the core objective of fixing the id-only wrapper functions.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[Amxx]

If any of these functions is called with a non-existing proposalId, the corresponding structure will be empty. This will correspond to queuing/executing/cancelling an empty proposal. That empty proposal will be hashed to id 99080998683144404393413273667897649021575810352642768606614393633707543911777.

Since that (empty) proposal cannot be proposed (blocked by the targets.length not being 0 check), we get the expected revert GovernorNonexistentProposal. However, the custom error with include the proposalId of the empty proposal and NOT the proposalId that was requested (and that is invalid).

This PR improves the produced error, and might help with debugging.

[Amxx]

I'm not sure we need a changeset here. @ernestognw, would you add one ?

[ernestognw]

I'm not sure we need a changeset here. @ernestognw, would you add one ?

No I don't think it's needed since the only change would be the argument to the GovernorNonexistentProposal error. In any case we already don't recommend relying on custom errors for anything onchain so I don't see how a changeset here would be useful.

[ernestognw]

LGTM, but perhaps there are a couple of things to discuss @Amxx:

1. This change adds a small overhead (maybe a JUMPI and couple of stack operations) for the happy path, are we ok with that?
2. Maybe we'd like to use the require(proposal.descriptionHash != 0, GovernorNonexistentProposal(proposalId)) here?

[Amxx]

1. Gorvernor Storage is already SUPER expensive. The amount of ssload is crazy ... I don't see the added cost is significant.
2. I would have used the require(<bool>, <custom error>) syntax, but it would mean bumping the pragma from 0.8.24 to 0.8.27. I'd rather avoid doing that bump and keep the changes minimal.

[Amxx]

We are talking ~0.2% for queue/execute and keep in mind the execute are barelly doing anything, just calling a mock that emits an event.