feat: Make OTCBuffer Upgradeable (DEV-1158)#232
Merged
lucas-manuel merged 8 commits intodevfrom Jan 29, 2026
Merged
Conversation
Summary by OctaneNew ContractsNo new contracts were added. Updated Contracts
🔗 Commit Hash: a5736a2 |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughOTCBuffer is migrated to an upgradeable contract using OpenZeppelin's UUPS pattern. The contract now inherits from upgradeable base classes, replaces constructor-based initialization with an initializer function, and wraps deployment in ERC1967Proxy. Tests are updated to accommodate the new initialization flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
supercontracts
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/OTCBuffer.sol`:
- Around line 25-34: The initialize function allows passing a zero admin; add a
validation to prevent granting roles to address(0) by checking admin is non-zero
(e.g., require(admin != address(0), "OTCBuffer/invalid-admin")) before calling
_grantRole(DEFAULT_ADMIN_ROLE, admin) in function initialize to ensure a valid
admin is set.
- Around line 34-36: OTCBuffer is UUPS-upgradeable but lacks a storage gap; add
a reserved storage array (e.g., uint256[50] private __gap) to the OTCBuffer
contract to preserve future storage layout compatibility. Place the __gap
declaration as a private state variable inside the OTCBuffer contract (near
other state declarations, alongside functions like _authorizeUpgrade) so future
upgrades can add variables without corrupting storage.
In `@test/unit/OTCBuffer.t.sol`:
- Around line 51-67: Add tests covering UUPS upgrade authorization and
re-initialization protection: (1) add a test that calling initialize again on
the already-initialized proxy (call OTCBuffer.initialize on buffer) reverts (use
vm.expectRevert before calling buffer.initialize(admin, almProxy)); (2) add a
test that a non-admin cannot perform upgrades by calling
buffer.upgradeToAndCall(...) and expecting a revert encoded with
AccessControlUnauthorizedAccount(address, DEFAULT_ADMIN_ROLE); (3) add a test
where admin (vm.prank(admin)) calls buffer.upgradeToAndCall(newImpl, "") to
ensure successful upgrade (verify by checking implementation slot or
post-upgrade behavior). Ensure tests reference buffer, OTCBuffer.initialize,
_authorizeUpgrade, and upgradeToAndCall.
src/OTCBuffer.sol
Outdated
Comment on lines
25
to
34
| function initialize(address admin, address _almProxy) external initializer { | ||
| require(_almProxy != address(0), "OTCBuffer/invalid-alm-proxy"); | ||
|
|
||
| __AccessControlEnumerable_init(); | ||
| __UUPSUpgradeable_init(); | ||
|
|
||
| _grantRole(DEFAULT_ADMIN_ROLE, admin); | ||
|
|
||
| almProxy = _almProxy; | ||
| } |
There was a problem hiding this comment.
Consider validating admin address is non-zero.
The _almProxy parameter is validated against address(0), but admin is not. Granting DEFAULT_ADMIN_ROLE to address(0) would result in an unusable contract since no one could perform admin actions or upgrades.
🛡️ Proposed fix to add validation
function initialize(address admin, address _almProxy) external initializer {
+ require(admin != address(0), "OTCBuffer/invalid-admin");
require(_almProxy != address(0), "OTCBuffer/invalid-alm-proxy");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function initialize(address admin, address _almProxy) external initializer { | |
| require(_almProxy != address(0), "OTCBuffer/invalid-alm-proxy"); | |
| __AccessControlEnumerable_init(); | |
| __UUPSUpgradeable_init(); | |
| _grantRole(DEFAULT_ADMIN_ROLE, admin); | |
| almProxy = _almProxy; | |
| } | |
| function initialize(address admin, address _almProxy) external initializer { | |
| require(admin != address(0), "OTCBuffer/invalid-admin"); | |
| require(_almProxy != address(0), "OTCBuffer/invalid-alm-proxy"); | |
| __AccessControlEnumerable_init(); | |
| __UUPSUpgradeable_init(); | |
| _grantRole(DEFAULT_ADMIN_ROLE, admin); | |
| almProxy = _almProxy; | |
| } |
🤖 Prompt for AI Agents
In `@src/OTCBuffer.sol` around lines 25 - 34, The initialize function allows
passing a zero admin; add a validation to prevent granting roles to address(0)
by checking admin is non-zero (e.g., require(admin != address(0),
"OTCBuffer/invalid-admin")) before calling _grantRole(DEFAULT_ADMIN_ROLE, admin)
in function initialize to ensure a valid admin is set.
Overview
🔗 Commit Hash: a5736a2 |
lucas-manuel
requested changes
Jan 29, 2026
lucas-manuel
added
Status: Revisions Needed
and removed
Status: Ready for Review
labels
supercontracts
added
Status: Ready for Review
and removed
Status: Revisions Needed
labels
lucas-manuel
requested changes
Jan 29, 2026
lucas-manuel
added
Status: Revisions Needed
and removed
Status: Ready for Review
labels
lucas-manuel
requested changes
Jan 29, 2026
supercontracts
added
Status: Ready for Review
and removed
Status: Revisions Needed
labels
lucas-manuel
requested changes
Jan 29, 2026
src/OTCBuffer.sol
Outdated
| 0xa8ff6143ce9b2840ac86932ea3c593f97be7f3f4c76899ca3e385ef6d4c71f00; | ||
|
|
||
| function _getBufferStorage() internal pure returns (BufferStorage storage $) { | ||
| // slither-disable-next-line assembly |
| ) | ||
| ); | ||
|
|
||
| assertEq(newBuffer.hasRole(DEFAULT_ADMIN_ROLE, newAdmin), true); |
Contributor
There was a problem hiding this comment.
Need to check almProxy address too
deluca-mike
requested changes
Jan 29, 2026
deluca-mike
previously approved these changes
Jan 29, 2026
lucas-manuel
requested changes
Jan 29, 2026
lucas-manuel
added
Status: Revisions Needed
and removed
Status: Ready for Review
labels
supercontracts
added
Status: Ready for Review
and removed
Status: Revisions Needed
labels
lucas-manuel
approved these changes
Jan 29, 2026
|
Coverage after merging dev-1158-otcbuffer-upgradable into dev will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.