-
Notifications
You must be signed in to change notification settings - Fork 12.3k
Update transpilation process #5941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughThis change updates documentation and tooling: CHANGELOG.md notes that Initializable and UUPSUpgradeable are no longer transpiled into @openzeppelin/contracts-upgradeable and removes two previously listed breaking-change entries. UUPSUpgradeable.sol gains a NatSpec tag "@Custom:stateless" with no code or API changes. package.json bumps devDependency "@openzeppelin/upgrade-safe-transpiler" from ^0.3.32 to ^0.4.0. scripts/upgradeable/transpile.sh refines exclusion patterns to target only contracts/proxy/**/Proxy.sol and contracts/proxy/beacon/UpgradeableBeacon.sol, and updates an explanatory comment. No exported/public declarations are altered. Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CHANGELOG.md (1)
9-9: Clarify migration with an import example.The note is good; add a short snippet to reduce user friction.
- - `Initializable` and `UUPSUpgradeable` are no longer transpiled, and are now absent from the `@openzeppelin/contracts-upgradeable` package. Imports must be updated to get these files from `@openzeppelin/contracts` instead. + - `Initializable` and `UUPSUpgradeable` are no longer transpiled, and are now absent from the `@openzeppelin/contracts-upgradeable` package. Import them from `@openzeppelin/contracts` instead. Example: + + ```diff + - import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; + - import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; + + import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; + + import { UUPSUpgradeable } from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; + ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)contracts/proxy/utils/UUPSUpgradeable.sol(1 hunks)package.json(1 hunks)scripts/upgradeable/transpile.sh(2 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). (4)
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: tests
- GitHub Check: tests-foundry
🔇 Additional comments (4)
contracts/proxy/utils/UUPSUpgradeable.sol (1)
18-19: NatSpec “stateless” tag added — LGTM.No functional change; consistent with documentation patterns. Ensure docs toolchain surfaces custom tags.
If using solidity-docgen, confirm @Custom:stateless is preserved in generated docs with current config.
scripts/upgradeable/transpile.sh (2)
26-26: Comment tweak OK.The updated description matches the narrower -x globs.
31-44: Add CI guard: fail if transpiler output contains Initializable or UUPSUpgradeableRepo search found only originals at contracts/proxy/utils/Initializable.sol and contracts/proxy/utils/UUPSUpgradeable.sol and no contracts-upgradeable outputs — add a CI job that runs the transpiler and fails if any generated files named Initializable.sol or UUPSUpgradeable.sol appear (e.g., check the transpile output dir).
package.json (1)
65-65: Transpiler 0.4.0 not published — confirm dependency and CLI flags.
npm shows @openzeppelin/upgrade-safe-transpiler latest published is 0.3.33; no 0.4.0 release exists to verify -i/-x/-N/-q behavior.
Either change package.json to a published version (e.g., "^0.3.33") or provide the private 0.4.0 package.json/CHANGELOG or repo/PR so the CLI flags can be checked.
File: package.json (line 65)
8be00ac to
258e110
Compare
415b8e7 to
d17b86c
Compare
|
Note: the storage layout breaking in |
|
In regards to ERC721Holder and ERC1155Holder, we can also keep them stateless and provide a concrete instruction to use the ones from |
The rational was to minimize the breaking change, and delay that for later. One important thing to note is that Do we want to remove them now ? |
|
Ah I see that this PR is about stop transpiling (or porting) UUPSUpgradeable and Initializable over to upgradeable contracts and not necessarily about being strict on all the contracts we should transpile. I would consider adding the |
Use transpiler v0.4.1
@openzeppelin/contracts/proxy/utils/Initializable.solinstead of a relative (local) import.