fix: allow stateless constructors in validation#1282
Conversation
Walkthrough
ChangesStateless Constructor Validation Bypass
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/contracts/test/ValidationsNatspec.sol (1)
29-36: 💤 Low valueConsider documenting the storage slot.
The storage slot
0x9b779b17422d0df92223018b32b4d1fa46e071723d6817e2486d003becc55f00is used in both this file andValidate.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
📒 Files selected for processing (6)
.changeset/stateless-constructor-validation.mdpackages/core/contracts/test/ValidationsNatspec.solpackages/core/contracts/test/cli/Validate.solpackages/core/src/cli/cli.test.tspackages/core/src/validate.test.tspackages/core/src/validate/run.ts
Problem
@custom:statelesscontracts 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
ReentrancyGuardin Contracts v5.5+.Summary of Changes
@custom:stateless.@openzeppelin/upgrades-core.Related to OpenZeppelin/openzeppelin-foundry-upgrades#116.
Testing
yarn workspace @openzeppelin/upgrades-core test src/validate.test.tsyarn 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.tsSummary by CodeRabbit
Bug Fixes
@custom:statelessannotation can now define constructor logic during upgrade safety validation without triggering validation errors, enabling more flexible contract architecture.Tests