OZ contract upgrade #118
Merged
OZ contract upgrade #118
Conversation
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
kbrizzle
reviewed
Apr 2, 2025
| * @notice ProxyAdmin with 2-step ownership transfer | ||
| * @dev Adds 2-step ownership transfer to the OpenZeppelin ProxyAdmin contract, both for the owner of the ProxyAdmin | ||
| * and to the proxy ownership transfer. | ||
| * @dev Adds 2-step ownership transfer to the OpenZeppelin ProxyAdmin contract for the owner of the ProxyAdmin. |
Contributor
There was a problem hiding this comment.
This is actually a lot cleaner / glad they made admin's immutable in the proxy layer.
test/access/ProxyOwner.t.sol
Outdated
| proxyAdmin.transferOwnership(address(proxyOwner)); | ||
| proxyOwner2 = new ProxyOwner(); | ||
| vm.stopPrank(); | ||
| console.log("owner %s proxyOwner %s admin %s", owner, address(proxyOwner), address(proxyAdmin)); |
Contributor
Author
There was a problem hiding this comment.
removed in 08d3c8d
linter should have caught this; I will configure it outside of this PR
| */ | ||
| function _constructing() private view returns (bool) { | ||
| return !Address.isContract(address(this)); | ||
| return !(address(this).code.length > 0); |
Contributor
There was a problem hiding this comment.
Little nervous that they removed this, I think I read that EOF will break this check also.
How does their version of Initializable handle this initialization now?
Contributor
There was a problem hiding this comment.
Oh nvm looks like they removed it for misuse reasons: OpenZeppelin/openzeppelin-contracts#3945.
src/token/types/Token18.sol
Outdated
| */ | ||
| function approve(Token18 self, address grantee, UFixed18 amount) internal { | ||
| IERC20(Token18.unwrap(self)).safeApprove(grantee, UFixed18.unwrap(amount)); | ||
| IERC20 token = IERC20(Token18.unwrap(self)); |
Contributor
There was a problem hiding this comment.
Yeah, I agree with you that we shouldn't pull this safe logic in.
| // SPDX-License-Identifier: MIT | ||
| // OpenZeppelin Contracts (introduced v4.6.0, adapted from v4.9) (crosschain/CrossChainEnabled.sol) | ||
|
|
||
| pragma solidity ^0.8.20; |
Contributor
There was a problem hiding this comment.
I'm thinking we should follow OZ's lead and remove this cross stuff as well for now.
If we need it again in the future we can either pull it back in or find a more modern way of constructing it.
prateekdefi
reviewed
Apr 3, 2025
Unit Test Coverage ReportCoverage after merging ed/pe-2399 into v3 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
kbrizzle
approved these changes
Apr 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Some notes: