Skip to content
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

refactor: use oz ownable opposed to custom copy #60

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

sakulstra
Copy link
Contributor

@sakulstra sakulstra commented Jan 20, 2025

Currently using this contract is a big pain, because it uses a different context, so there's always conflicts.
Using the default Ownable fixes this.

Origin maintains its own copy of Ownable and is not affected by that change.

Copy link
Contributor

github-actions bot commented Jan 20, 2025

🔧 Build logs
Compiling 86 files with Solc 0.8.27
Solc 0.8.27 finished in 3.14s
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                | 654              | 952               | 23,922             | 48,200              |
|-----------------------------+------------------+-------------------+--------------------+---------------------|
| 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              |
|-----------------------------+------------------+-------------------+--------------------+---------------------|
| RescuableACL                | 1,695            | 1,827             | 22,881             | 47,325              |
|-----------------------------+------------------+-------------------+--------------------+---------------------|
| 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 zksync
Compiling 46 files with zksolc and solc 0.8.24
zksolc and solc 0.8.24 finished in 9.29s
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;
   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


╭-------------------------------+------------------+-------------------+--------------------+---------------------╮
| 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,000            | 4,000             | 446,999            | 446,999             |
|-------------------------------+------------------+-------------------+--------------------+---------------------|
| ERC1967Utils                  | 224              | 224               | 450,775            | 450,775             |
|-------------------------------+------------------+-------------------+--------------------+---------------------|
| MockImpl                      | 2,208            | 2,208             | 448,791            | 448,791             |
|-------------------------------+------------------+-------------------+--------------------+---------------------|
| ProxyAdmin                    | 4,384            | 4,384             | 446,615            | 446,615             |
|-------------------------------+------------------+-------------------+--------------------+---------------------|
| StorageSlot                   | 224              | 224               | 450,775            | 450,775             |
|-------------------------------+------------------+-------------------+--------------------+---------------------|
| TransparentProxyFactoryZkSync | 7,712            | 7,712             | 443,287            | 443,287             |
|-------------------------------+------------------+-------------------+--------------------+---------------------|
| TransparentUpgradeableProxy   | 6,752            | 6,752             | 444,247            | 444,247             |
╰-------------------------------+------------------+-------------------+--------------------+---------------------╯

Copy link
Contributor

github-actions bot commented Jan 20, 2025

🌈 Test Results
No 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 398.61µs (112.17µ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 954.44µs (550.36µ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.42ms (2.07ms CPU time)

Ran 5 tests for test/RescuableACL.t.sol:RescueACLTest
[PASS] testEmergencyEtherTransfer() (gas: 57746)
[PASS] testEmergencyEtherTransferWhenNotOwner() (gas: 17659)
[PASS] testEmergencyTokenTransfer() (gas: 231235)
[PASS] testEmergencyTokenTransferWhenNotOwner() (gas: 203516)
[PASS] testToken() (gas: 2392)
Suite result: ok. 5 passed; 0 failed; 0 skipped; finished in 2.35ms (2.01ms 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 33.17ms (57.63ms CPU time)

Ran 6 tests for test/OwnableWithGuardian.t.sol:TestOfOwnableWithGuardian
[PASS] testConstructorLogic() (gas: 18202)
[PASS] testGuardianUpdateNoAccess() (gas: 15325)
[PASS] testGuardianUpdateViaGuardian(address) (runs: 256, μ: 19431, ~: 19431)
[PASS] testGuardianUpdateViaOwner(address) (runs: 256, μ: 19245, ~: 19245)
[PASS] test_onlyGuardianGuard() (gas: 12577)
[PASS] test_onlyGuardianGuard_shouldRevert() (gas: 10965)
Suite result: ok. 6 passed; 0 failed; 0 skipped; finished in 46.53ms (46.01ms 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 33.23ms (33.02ms 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 141.94ms (306.48ms CPU time)

Ran 2 tests for test/create3Test.t.sol:Create3FactoryTest
[PASS] testCreate3WithValue(address,address,address) (runs: 256, μ: 257570, ~: 257570)
[PASS] testCreate3WithoutValue(address,address,bytes32) (runs: 256, μ: 260210, ~: 260210)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 105.78ms (105.57ms CPU time)

Ran 9 test suites in 171.90ms (366.78ms CPU time): 38 tests passed, 0 failed, 0 skipped (38 total tests)
🌈 Test Results zksync
Compiling 44 files with Solc 0.8.24
Solc 0.8.24 finished in 1.45s
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: 342384)
[PASS] testCreateDeterministic(bytes32) (runs: 256, μ: 574418, ~: 574418)
[PASS] testCreateDeterministicProxyAdmin(address,bytes32) (runs: 256, μ: 463994, ~: 469194)
[PASS] testCreateDeterministicWithDeterministicProxy(bytes32,bytes32) (runs: 256, μ: 685055, ~: 685071)
[PASS] testCreateProxyAdmin(address,bytes32) (runs: 256, μ: 352432, ~: 358030)
Suite result: ok. 5 passed; 0 failed; 0 skipped; finished in 10.03s (32.97s CPU time)

Ran 1 test suite in 10.03s (10.03s CPU time): 5 tests passed, 0 failed, 0 skipped (5 total tests)

Copy link
Contributor

github-actions bot commented Jan 20, 2025

🔮 Coverage report
File Line Coverage Function Coverage Branch Coverage
src/contracts/access-control/OwnableWithGuardian.sol $^{↑0.66\%}{\color{orange}89.47\%}$
$17 / 19$
19, 20
${\color{orange}87.5\%}$
$7 / 8$
OwnableWithGuardian.onlyOwnerOrGuardian
${\color{green}100\%}$
$2 / 2$
src/contracts/access-control/UpgradeableOwnableWithGuardian.sol ${\color{green}100\%}$
$23 / 23$
${\color{green}100\%}$
$9 / 9$
${\color{green}100\%}$
$2 / 2$
src/contracts/create3/Create3.sol ${\color{orange}89.47\%}$
$17 / 19$
62, 66
${\color{red}80\%}$
$4 / 5$
Create3.create3
${\color{red}33.33\%}$
$1 / 3$
src/contracts/create3/Create3Factory.sol ${\color{green}100\%}$
$6 / 6$
${\color{green}100\%}$
$2 / 2$
${\color{green}100\%}$
$0 / 0$
src/contracts/oz-common/SafeERC20.sol ${\color{red}16.67\%}$
$5 / 30$
45, 46, 53, 54, 55 and 20 more
${\color{red}25\%}$
$2 / 8$
SafeERC20.safeTransferFrom, SafeERC20.safeIncreaseAllowance, SafeERC20.safeDecreaseAllowance, SafeERC20.forceApprove, SafeERC20.safePermit and 1 more
${\color{red}0\%}$
$0 / 4$
src/contracts/transparent-proxy/Initializable.sol ${\color{red}66.67\%}$
$16 / 24$
123, 124, 128, 129, 131 and 3 more
${\color{red}60\%}$
$3 / 5$
Initializable.reinitializer, Initializable.onlyInitializing
${\color{red}45.45\%}$
$5 / 11$
src/contracts/transparent-proxy/Proxy.sol ${\color{red}0\%}$
$0 / 16$
28, 33, 37, 40, 44 and 11 more
${\color{red}0\%}$
$0 / 5$
Proxy._delegate, Proxy._fallback, Proxy.fallback, Proxy.receive, Proxy._beforeFallback
${\color{green}100\%}$
$0 / 0$
src/contracts/transparent-proxy/ProxyAdmin.sol ${\color{red}0\%}$
$0 / 2$
38, 43
${\color{red}0\%}$
$0 / 1$
ProxyAdmin.upgradeAndCall
${\color{green}100\%}$
$0 / 0$
src/contracts/transparent-proxy/TransparentProxyFactory.sol ${\color{green}100\%}$
$3 / 3$
${\color{green}100\%}$
$1 / 1$
${\color{green}100\%}$
$0 / 0$
src/contracts/transparent-proxy/TransparentProxyFactoryBase.sol ${\color{red}63.64\%}$
$14 / 22$
18, 19, 21, 22, 26 and 3 more
${\color{red}66.67\%}$
$4 / 6$
TransparentProxyFactoryBase.create, TransparentProxyFactoryBase.createProxyAdmin
${\color{green}100\%}$
$0 / 0$
src/contracts/transparent-proxy/TransparentUpgradeableProxy.sol ${\color{red}57.14\%}$
$8 / 14$
104, 105, 107, 121, 122 and 1 more
${\color{red}75\%}$
$3 / 4$
TransparentUpgradeableProxy._dispatchUpgradeToAndCall
${\color{red}0\%}$
$0 / 4$
src/contracts/utils/PermissionlessRescuable.sol ${\color{green}100\%}$
$4 / 4$
${\color{green}100\%}$
$2 / 2$
${\color{green}100\%}$
$0 / 0$
src/contracts/utils/Rescuable.sol ${\color{green}100\%}$
$7 / 7$
${\color{green}100\%}$
$3 / 3$
${\color{green}100\%}$
$1 / 1$
src/contracts/utils/Rescuable721.sol ${\color{green}100\%}$
$3 / 3$
${\color{green}100\%}$
$1 / 1$
${\color{green}100\%}$
$0 / 0$
src/contracts/utils/RescuableACL.sol ${\color{green}100\%}$
$6 / 6$
${\color{green}100\%}$
$3 / 3$
${\color{green}100\%}$
$0 / 0$
src/contracts/utils/RescuableBase.sol ${\color{green}100\%}$
$10 / 10$
${\color{green}100\%}$
$2 / 2$
${\color{green}100\%}$
$1 / 1$

Copy link
Contributor

github-actions bot commented Jan 20, 2025

Gas report

Create3Factory

  • size: 1122 / 49152
Method min mean median max calls
create(bytes32,bytes) ↓-2.6%40423 ↓-7.9%193416 ↓-8.2%269661 ↓-8.3%270165 3

TransparentProxyFactory

  • size: 5342 / 49152
Method min mean median max calls
createDeterministic(address,address,bytes,bytes32) 384898 384904 384904 384910 2
createDeterministicProxyAdmin(address,bytes32) 288356 ↓-0.1%288362 ↓-0.1%288362 ↓-0.2%288368 2

MockERC721

  • size: 2449 / 49152
Method min mean median max calls
mint(address,uint256) ↑0.3%68318 ↑0.15%68318 ↑0.15%68318 68318 2
transferFrom(address,address,uint256) ↑0.38%54097 ↑0.38%54097 ↑0.38%54097 ↑0.38%54097 1

ImplOwnableWithGuardian

  • size: ↓-1.3%1183 / 49152
Method min mean median max calls
mock_onlyGuardian() 23394 23424 23424 ↓-0.18%23455 2
updateGuardian(address) 26144 ↑0.1%30029 30531 30531 9

Rescuable721

  • size: 2233 / 49152
Method min mean median max calls
emergency721TokenTransfer(address,address,uint256) 22423 ↓-0.23%40964 ↓-0.23%40964 ↓-0.34%59505 2

ImplOwnableWithGuardian

  • size: 1492 / 49152
Method min mean median max calls
updateGuardian(address) 26343 ↑0.23%29204 30532 ↑0.67%30739 3

MockContract

  • size: ↓-6.4%1016 / 49152

@sakulstra sakulstra changed the title feat: use oz refactor: use oz ownable opposed to custom copy Jan 22, 2025
@sakulstra sakulstra merged commit c686d34 into main Jan 27, 2025
1 check passed
@sakulstra sakulstra deleted the fix/upgrade-withguardian branch January 27, 2025 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants