Skip to content

fix: allow stateless constructors in validation#1282

Open
ChargingFoxSec wants to merge 1 commit into
OpenZeppelin:masterfrom
ChargingFoxSec:fix/stateless-constructor-validation-116
Open

fix: allow stateless constructors in validation#1282
ChargingFoxSec wants to merge 1 commit into
OpenZeppelin:masterfrom
ChargingFoxSec:fix/stateless-constructor-validation-116

Conversation

@ChargingFoxSec

@ChargingFoxSec ChargingFoxSec commented Jun 20, 2026

Copy link
Copy Markdown

Problem

@custom:stateless contracts can intentionally include constructor logic that does not affect the implementation storage layout, but upgrade safety validation currently reports inherited constructors from these contracts as unsafe.

This surfaced in OpenZeppelin/openzeppelin-foundry-upgrades#116 with contracts such as ReentrancyGuard in Contracts v5.5+.

Summary of Changes

  • Skip constructor validation errors for contracts annotated with @custom:stateless.
  • Add validation coverage for a child contract inheriting a stateless constructor parent.
  • Add CLI validation coverage for the same case.
  • Add a patch changeset for @openzeppelin/upgrades-core.

Related to OpenZeppelin/openzeppelin-foundry-upgrades#116.

Testing

  • yarn workspace @openzeppelin/upgrades-core test src/validate.test.ts
  • yarn workspace @openzeppelin/upgrades-core test src/cli/cli.test.ts --match "validate - single contract with stateless constructor parent - ok"
  • yarn lint:path packages/core/src/validate/run.ts packages/core/src/validate.test.ts packages/core/src/cli/cli.test.ts

Summary by CodeRabbit

  • Bug Fixes

    • Contracts marked with the @custom:stateless annotation can now define constructor logic during upgrade safety validation without triggering validation errors, enabling more flexible contract architecture.
  • Tests

    • Comprehensive test coverage was added for stateless constructor scenarios, including validation across contract inheritance hierarchies and command-line interface validation workflows.

@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

getConstructorErrors in run.ts is updated to import hasAnnotationTag and return immediately with no errors when the contract carries the @stateless annotation tag. Solidity test fixtures (StatelessNonEmptyConstructor, ChildOfStatelessNonEmptyConstructor, StatelessConstructor, HasStatelessConstructorParent) and corresponding unit/CLI test assertions are added, along with a patch-level Changeset entry.

Changes

Stateless Constructor Validation Bypass

Layer / File(s) Summary
Core validation bypass for stateless annotation
packages/core/src/validate/run.ts
Imports hasAnnotationTag and adds an early return in getConstructorErrors when the contract's documentation contains the stateless annotation tag, suppressing all constructor errors for those contracts.
Solidity fixtures and test assertions
packages/core/contracts/test/ValidationsNatspec.sol, packages/core/contracts/test/cli/Validate.sol, packages/core/src/validate.test.ts, packages/core/src/cli/cli.test.ts, .changeset/stateless-constructor-validation.md
Adds StatelessNonEmptyConstructor (inline-assembly sstore constructor) and its child contract to ValidationsNatspec.sol; adds StatelessConstructor (annotated @custom:stateless) and HasStatelessConstructorParent to Validate.sol; extends unit and CLI tests to assert both contracts validate successfully; includes a patch Changeset entry.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • ernestognw

Poem

🐇 Hop, hop! The constructor's free,
When @custom:stateless marks thee.
No errors thrown, no warnings cast,
Inline assembly's safe at last!
This bunny patched the upgrade path—
No sstore shall face validation's wrath. 🌟

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: allowing stateless constructors to pass validation checks, which directly matches the PR's primary objective.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/core/contracts/test/ValidationsNatspec.sol (1)

29-36: 💤 Low value

Consider documenting the storage slot.

The storage slot 0x9b779b17422d0df92223018b32b4d1fa46e071723d6817e2486d003becc55f00 is used in both this file and Validate.sol. Adding a brief comment explaining its purpose (e.g., "test-safe slot that doesn't collide with proxy storage layout") would improve clarity.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/contracts/test/ValidationsNatspec.sol` around lines 29 - 36,
The assembly block in the StatelessNonEmptyConstructor constructor uses a
hardcoded storage slot without explanation. Add a comment above the sstore
operation in the assembly block explaining the purpose of the storage slot
0x9b779b17422d0df92223018b32b4d1fa46e071723d6817e2486d003becc55f00, clarifying
that it is a test-safe slot chosen to avoid collisions with proxy storage
layout. This will improve code clarity since this same storage slot is also used
in Validate.sol.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/core/contracts/test/ValidationsNatspec.sol`:
- Around line 29-36: The assembly block in the StatelessNonEmptyConstructor
constructor uses a hardcoded storage slot without explanation. Add a comment
above the sstore operation in the assembly block explaining the purpose of the
storage slot 0x9b779b17422d0df92223018b32b4d1fa46e071723d6817e2486d003becc55f00,
clarifying that it is a test-safe slot chosen to avoid collisions with proxy
storage layout. This will improve code clarity since this same storage slot is
also used in Validate.sol.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f050d832-a027-4be0-a116-ad2ccac724e3

📥 Commits

Reviewing files that changed from the base of the PR and between 4c07151 and eb03e89.

📒 Files selected for processing (6)
  • .changeset/stateless-constructor-validation.md
  • packages/core/contracts/test/ValidationsNatspec.sol
  • packages/core/contracts/test/cli/Validate.sol
  • packages/core/src/cli/cli.test.ts
  • packages/core/src/validate.test.ts
  • packages/core/src/validate/run.ts

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