feat: upgrade transparent proxy #48
Merged
Conversation
This file contains 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
🔮 Coverage report
|
🌈 Test ResultsNo files changed, compilation skipped
Ran 1 test for test/ChainHelperTest.t.sol:TestChainHelpers
[PASS] test_selectChain_shouldRevert() (gas: 3293)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 400.67µs (117.55µs CPU time)
Ran 6 tests for test/PermissionlessRescuable.t.sol:PermissionlessRescuableTest
[PASS] test_emergencyEtherTransfer() (gas: 59257)
[PASS] test_emergencyEtherTransferInsufficientBalance_shouldRevert() (gas: 45456)
[PASS] test_emergencyTokenTransfer() (gas: 81017)
[PASS] test_emergencyTokenTransferInsufficientBalance_shouldRevert() (gas: 21438)
[PASS] test_emergencyTokenTransfer_withTransferRestriction() (gas: 115750)
[PASS] test_whoShouldReceiveFunds() (gas: 12734)
Suite result: ok. 6 passed; 0 failed; 0 skipped; finished in 1.01ms (516.51µs CPU time)
Ran 5 tests for test/Rescuable.t.sol:RescueTest
[PASS] testEmergencyEtherTransfer() (gas: 57744)
[PASS] testEmergencyEtherTransferWhenNotOwner() (gas: 17666)
[PASS] testEmergencyTokenTransfer() (gas: 231216)
[PASS] testEmergencyTokenTransferWhenNotOwner() (gas: 203504)
[PASS] testToken() (gas: 2392)
Suite result: ok. 5 passed; 0 failed; 0 skipped; finished in 2.13ms (1.76ms CPU time)
Ran 6 tests for test/UpgradeableOwnableWithGuardian.t.sol:TestOfUpgradableOwnableWithGuardian
[PASS] test_initializer() (gas: 18304)
[PASS] test_onlyGuardian() (gas: 11066)
[PASS] test_onlyOwnerOrGuardian() (gas: 13262)
[PASS] test_updateGuardian_eoa(address,address) (runs: 256, μ: 18703, ~: 18703)
[PASS] test_updateGuardian_guardian(address) (runs: 256, μ: 19684, ~: 19684)
[PASS] test_updateGuardian_owner(address) (runs: 256, μ: 19400, ~: 19400)
Suite result: ok. 6 passed; 0 failed; 0 skipped; finished in 44.03ms (42.19ms CPU time)
Ran 3 tests for test/Rescuable721.t.sol:Rescue721Test
[PASS] testFuzzEmergencyTokenTransfer(address) (runs: 256, μ: 79081, ~: 79081)
[PASS] testFuzzEmergencyTokenTransferWhenNotOwner(address,address) (runs: 256, μ: 71859, ~: 71859)
[PASS] testToken() (gas: 2458)
Suite result: ok. 3 passed; 0 failed; 0 skipped; finished in 51.43ms (51.01ms CPU time)
Ran 4 tests for test/TransparentProxyFactory.t.sol:TestTransparentProxyFactory
[PASS] testCreateDeterministic(address,bytes32) (runs: 256, μ: 384826, ~: 384826)
[PASS] testCreateDeterministicProxyAdmin(address,bytes32) (runs: 256, μ: 282376, ~: 282376)
[PASS] testCreateDeterministicWithDeterministicProxy(bytes32,bytes32) (runs: 256, μ: 391377, ~: 391377)
[PASS] testCreateProxyAdmin(address,bytes32) (runs: 256, μ: 276438, ~: 276438)
Suite result: ok. 4 passed; 0 failed; 0 skipped; finished in 107.79ms (218.24ms CPU time)
Ran 6 tests for test/OwnableWithGuardian.t.sol:TestOfOwnableWithGuardian
[PASS] testConstructorLogic() (gas: 18202)
[PASS] testGuardianUpdateNoAccess() (gas: 14902)
[PASS] testGuardianUpdateViaGuardian(address) (runs: 256, μ: 19419, ~: 19419)
[PASS] testGuardianUpdateViaOwner(address) (runs: 256, μ: 19236, ~: 19236)
[PASS] test_onlyGuardianGuard() (gas: 12577)
[PASS] test_onlyGuardianGuard_shouldRevert() (gas: 10566)
Suite result: ok. 6 passed; 0 failed; 0 skipped; finished in 117.20ms (17.20ms CPU time)
Ran 2 tests for test/create3Test.t.sol:Create3FactoryTest
[PASS] testCreate3WithValue(address,address,address) (runs: 256, μ: 280857, ~: 280857)
[PASS] testCreate3WithoutValue(address,address,bytes32) (runs: 256, μ: 283786, ~: 283786)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 84.65ms (105.66ms CPU time)
Ran 8 test suites in 133.67ms (408.63ms CPU time): 33 tests passed, 0 failed, 0 skipped (33 total tests)🌈 Test Results zksyncCompiling 44 files with Solc 0.8.24
Solc 0.8.24 finished in 1.51s
Compiler run successful with warnings:
Warning (2519): This declaration shadows an existing declaration.
--> zksync/test/TransparentProxyFactoryZkSync.t.sol:88:5:
|
88 | address proxyAdmin = factory.createDeterministicProxyAdmin(proxyAdminOwner, proxyAdminSalt);
| ^^^^^^^^^^^^^^^^^^
Note: The shadowed declaration is here:
--> zksync/test/TransparentProxyFactoryZkSync.t.sol:14:3:
|
14 | ProxyAdmin internal proxyAdmin;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning (2519): This declaration shadows an existing declaration.
--> zksync/test/TransparentProxyFactoryZkSync.t.sol:106:5:
|
106 | address proxyAdmin = factory.createDeterministicProxyAdmin(proxyAdminOwner, proxyAdminSalt);
| ^^^^^^^^^^^^^^^^^^
Note: The shadowed declaration is here:
--> zksync/test/TransparentProxyFactoryZkSync.t.sol:14:3:
|
14 | ProxyAdmin internal proxyAdmin;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
No files changed, compilation skipped
Ran 5 tests for zksync/test/TransparentProxyFactoryZkSync.t.sol:TestTransparentProxyFactoryZkSync
[PASS] testCreate() (gas: 2776054)
[PASS] testCreateDeterministic(bytes32) (runs: 256, μ: 3201204, ~: 3201204)
[PASS] testCreateDeterministicProxyAdmin(address,bytes32) (runs: 256, μ: 2017907, ~: 2103572)
[PASS] testCreateDeterministicWithDeterministicProxy(bytes32,bytes32) (runs: 256, μ: 3408342, ~: 3408342)
[PASS] testCreateProxyAdmin(address,bytes32) (runs: 256, μ: 1817962, ~: 1895923)
Suite result: ok. 5 passed; 0 failed; 0 skipped; finished in 9.67s (30.86s CPU time)
Ran 1 test suite in 9.67s (9.67s CPU time): 5 tests passed, 0 failed, 0 skipped (5 total tests) |
⛽ Gas reportCreate3Factory
ProxyAdmin
TransparentProxyFactory
TransparentUpgradeableProxy
MockERC721
Rescuable721
ImplOwnableWithGuardian
|
🔧 Build logsCompiling 85 files with Solc 0.8.28
installing solc version "0.8.28"
Successfully installed solc 0.8.28
Solc 0.8.28 finished in 3.50s
Compiler run successful with warnings:
Warning (5667): Unused function parameter. Remove or comment out the variable name to silence this warning.
--> src/mocks/ERC721.sol:933:23:
|
933 | function tokenURI(uint256 id) public view override returns (string memory) {
| ^^^^^^^^^^
Warning (2018): Function state mutability can be restricted to pure
--> src/mocks/ERC721.sol:923:5:
|
923 | function name() public view override returns (string memory) {
| ^ (Relevant source part starts here and spans across multiple lines).
Warning (2018): Function state mutability can be restricted to pure
--> src/mocks/ERC721.sol:928:5:
|
928 | function symbol() public view override returns (string memory) {
| ^ (Relevant source part starts here and spans across multiple lines).
Warning (2018): Function state mutability can be restricted to pure
--> src/mocks/ERC721.sol:933:5:
|
933 | function tokenURI(uint256 id) public view override returns (string memory) {
| ^ (Relevant source part starts here and spans across multiple lines).
Warning (2018): Function state mutability can be restricted to view
--> test/PermissionlessRescuable.t.sol:63:3:
|
63 | function test_whoShouldReceiveFunds() public {
| ^ (Relevant source part starts here and spans across multiple lines).
Warning (2018): Function state mutability can be restricted to view
--> test/UpgradeableOwnableWithGuardian.t.sol:29:3:
|
29 | function test_initializer() external {
| ^ (Relevant source part starts here and spans across multiple lines).
| Contract | Runtime Size (B) | Initcode Size (B) | Runtime Margin (B) | Initcode Margin (B) |
|-----------------------------|------------------|-------------------|--------------------|---------------------|
| Address | 85 | 135 | 24,491 | 49,017 |
| ChainHelpers | 85 | 135 | 24,491 | 49,017 |
| ChainIds | 85 | 135 | 24,491 | 49,017 |
| Create2Utils | 162 | 212 | 24,414 | 48,940 |
| Create3 | 85 | 135 | 24,491 | 49,017 |
| Create3Factory | 1,094 | 1,122 | 23,482 | 48,030 |
| ERC1967Proxy | 163 | 1,014 | 24,413 | 48,138 |
| ERC1967Utils | 85 | 135 | 24,491 | 49,017 |
| ERC20 | 2,331 | 3,020 | 22,245 | 46,132 |
| EnumerableSet | 85 | 135 | 24,491 | 49,017 |
| ImplOwnableWithGuardian | 1,464 | 1,492 | 23,112 | 47,660 |
| MockContract | 759 | 1,021 | 23,817 | 48,131 |
| MockERC721 | 2,421 | 2,449 | 22,155 | 46,703 |
| MockImpl | 465 | 690 | 24,111 | 48,462 |
| PermissionlessRescuable | 1,908 | 2,081 | 22,668 | 47,071 |
| ProxyAdmin | 1,039 | 1,275 | 23,537 | 47,877 |
| Rescuable | 1,807 | 1,958 | 22,769 | 47,194 |
| Rescuable721 | 2,043 | 2,201 | 22,533 | 46,951 |
| SafeCast | 85 | 135 | 24,491 | 49,017 |
| SafeERC20 | 85 | 135 | 24,491 | 49,017 |
| StorageSlot | 85 | 135 | 24,491 | 49,017 |
| TestNetChainIds | 85 | 135 | 24,491 | 49,017 |
| TransparentProxyFactory | 5,314 | 5,342 | 19,262 | 43,810 |
| TransparentUpgradeableProxy | 1,137 | 2,266 | 23,439 | 46,886 |🔧 Build logs zksyncCompiling 46 files with zksolc and ZKsync solc 0.8.24
zksolc and ZKsync solc 0.8.24 finished in 11.13s
Compiler run successful with warnings:
Warning (2519)
Warning: This declaration shadows an existing declaration.
--> zksync/test/TransparentProxyFactoryZkSync.t.sol:88:5:
|
88 | address proxyAdmin = factory.createDeterministicProxyAdmin(proxyAdminOwner, proxyAdminSalt);
| ^^^^^^^^^^^^^^^^^^
Note: The shadowed declaration is here:
--> zksync/test/TransparentProxyFactoryZkSync.t.sol:14:3:
|
14 | ProxyAdmin internal proxyAdmin;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning (2519)
Warning: This declaration shadows an existing declaration.
--> zksync/test/TransparentProxyFactoryZkSync.t.sol:106:5:
|
106 | address proxyAdmin = factory.createDeterministicProxyAdmin(proxyAdminOwner, proxyAdminSalt);
| ^^^^^^^^^^^^^^^^^^
Note: The shadowed declaration is here:
--> zksync/test/TransparentProxyFactoryZkSync.t.sol:14:3:
|
14 | ProxyAdmin internal proxyAdmin;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning
┌──────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Warning: Your code or one of its dependencies uses the 'extcodesize' instruction, which is │
│ usually needed in the following cases: │
│ 1. To detect whether an address belongs to a smart contract. │
│ 2. To detect whether the deploy code execution has finished. │
│ zkSync Era comes with native account abstraction support (so accounts are smart contracts, │
│ including private-key controlled EOAs), and you should avoid differentiating between contracts │
│ and non-contract addresses. │
└──────────────────────────────────────────────────────────────────────────────────────────────────┘
--> lib/forge-std/src/StdCheats.sol
Warning
┌──────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Warning: Your code or one of its dependencies uses the 'extcodesize' instruction, which is │
│ usually needed in the following cases: │
│ 1. To detect whether an address belongs to a smart contract. │
│ 2. To detect whether the deploy code execution has finished. │
│ zkSync Era comes with native account abstraction support (so accounts are smart contracts, │
│ including private-key controlled EOAs), and you should avoid differentiating between contracts │
│ and non-contract addresses. │
└──────────────────────────────────────────────────────────────────────────────────────────────────┘
--> lib/forge-std/src/StdUtils.sol
Warning
┌──────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Warning: It looks like you are using 'ecrecover' to validate a signature of a user account. │
│ zkSync Era comes with native account abstraction support, therefore it is highly recommended NOT │
│ to rely on the fact that the account has an ECDSA private key attached to it since accounts might│
│ implement other signature schemes. │
│ Read more about Account Abstraction at https://v2-docs.zksync.io/dev/developer-guides/aa.html │
└──────────────────────────────────────────────────────────────────────────────────────────────────┘
--> lib/forge-std/src/mocks/MockERC20.sol
Warning
┌──────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Warning: Your code or one of its dependencies uses the 'extcodesize' instruction, which is │
│ usually needed in the following cases: │
│ 1. To detect whether an address belongs to a smart contract. │
│ 2. To detect whether the deploy code execution has finished. │
│ zkSync Era comes with native account abstraction support (so accounts are smart contracts, │
│ including private-key controlled EOAs), and you should avoid differentiating between contracts │
│ and non-contract addresses. │
└──────────────────────────────────────────────────────────────────────────────────────────────────┘
--> lib/forge-std/src/mocks/MockERC721.sol
| Contract | Runtime Size (B) | Initcode Size (B) | Runtime Margin (B) | Initcode Margin (B) |
|-------------------------------|------------------|-------------------|--------------------|---------------------|
| Address | 224 | 224 | 450,775 | 450,775 |
| Create2UtilsZkSync | 480 | 480 | 450,519 | 450,519 |
| ERC1967Proxy | 4,192 | 4,192 | 446,807 | 446,807 |
| ERC1967Utils | 224 | 224 | 450,775 | 450,775 |
| MockImpl | 2,272 | 2,272 | 448,727 | 448,727 |
| ProxyAdmin | 4,512 | 4,512 | 446,487 | 446,487 |
| StorageSlot | 224 | 224 | 450,775 | 450,775 |
| TransparentProxyFactoryZkSync | 8,032 | 8,032 | 442,967 | 442,967 |
| TransparentUpgradeableProxy | 7,136 | 7,136 | 443,863 | 443,863 | |
kyzia551
reviewed
Nov 17, 2024
| address admin, | ||
| bytes calldata data | ||
| ) external returns (address) { | ||
| function create(address logic, ProxyAdmin admin, bytes calldata data) external returns (address) { |
There was a problem hiding this comment.
why do we use ProxyAdmin here?
There was a problem hiding this comment.
it's a bit debatable, that's why i asked for careful review 😅
My thinking was that if passing a non ProxyAdmin here would potentiall break things. It's simply assumed to pass a proxy admin. While an interface ofc is no perfect enforcement, the intention is to at least highlight what should be passed.
| return proxy; | ||
| } | ||
|
|
||
| /// @inheritdoc ITransparentProxyFactory | ||
| function createProxyAdmin(address adminOwner) external returns (address) { | ||
| address proxyAdmin = address(new ProxyAdmin()); | ||
| IOwnable(proxyAdmin).transferOwnership(adminOwner); | ||
| address proxyAdmin = address(new ProxyAdmin(adminOwner)); |
There was a problem hiding this comment.
and here return it as address, not ProxyAdmin
There was a problem hiding this comment.
Here i simply did not change anything as nothing really changed.
From a user perspective, i usually find it easier to receive an address and cast to the interface i need.
| address adminOwner, | ||
| bytes32 salt | ||
| ) external returns (address) { | ||
| address proxyAdmin = address(new ProxyAdmin{salt: salt}(adminOwner)); |
There was a problem hiding this comment.
same here, did not change as imo doesn't help in this case
kyzia551
reviewed
Nov 17, 2024
There was a problem hiding this comment.
Maybe we should just import from oz directly? As we have 0 diff atm
There was a problem hiding this comment.
This:
import {ITransparentUpgradeableProxy} from './TransparentUpgradeableProxy.sol'; is the diff.
I don't see a way without copying.
kyzia551
reviewed
Nov 17, 2024
There was a problem hiding this comment.
Following the pattern, similiar to ProxyAdmin, maybe we can ditch IOwnable, it's not used anywhere outside I think, and use full Ownable
There was a problem hiding this comment.
kk removed. As we have oz as a dependency, we should probably also remove most things from oz-common, but will defer to another pr.
kyzia551
reviewed
Nov 17, 2024
| * | ||
| * These properties mean that the admin account can only be used for upgrading the proxy, so it's best if it's a | ||
| * dedicated account that is not used for anything else. This will avoid headaches due to sudden errors when trying to | ||
| * call a function from the proxy implementation. For this reason, the proxy deploys an instance of {ProxyAdmin} and |
There was a problem hiding this comment.
good catch, changed
sakulstra
commented
Nov 17, 2024
src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol
Outdated
Show resolved
Hide resolved
|
@kyzia551 just to explain the approach here (as perhaps it was not as self explanatory as i though it was 😅). The "original" version of the contract creates a ProxyAdmin. The rest I did not change, to cause as little changes as possible. |
kyzia551
approved these changes
Nov 18, 2024
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.
The code change here is that the
_adminon TransparentProxy is immutable so things likechangeAdminare no longer possible.The rational is that instead of changing the admin, one should change the owner of the admin.
This way one can omit a storage read which adds up quite a bit if one gas proxies talking to proxies.
This change in behavior also introduces some breaking changes though:
OZ enforces a new ProxyAdmin per Proxy, which for us is a bit suboptimal.
Therefore instead of using their exact code, I had to copy it and change the constructor to accept a ProxyAdmin as input.