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: remove safecast #61

Merged
merged 2 commits into from
Jan 27, 2025
Merged

refactor: remove safecast #61

merged 2 commits into from
Jan 27, 2025

Conversation

sakulstra
Copy link
Contributor

@sakulstra sakulstra commented Jan 22, 2025

The oz version in dependencies is feature equivalent, but more gas optimized & throwing more meaningful errors.

Origin does not rely on this version.

Copy link
Contributor

🌈 Test Results
No files changed, compilation skipped

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.00ms (614.34µs CPU time)

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 3.30ms (48.57µ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.43ms (2.08ms 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.41ms (2.06ms 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 16.25ms (25.00ms 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 70.11ms (69.91ms 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 135.38ms (131.47ms 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 171.87ms (68.16ms 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 171.53ms (239.82ms CPU time)

Ran 9 test suites in 173.23ms (574.28ms 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.41s
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, μ: 463248, ~: 469194)
[PASS] testCreateDeterministicWithDeterministicProxy(bytes32,bytes32) (runs: 256, μ: 685047, ~: 685071)
[PASS] testCreateProxyAdmin(address,bytes32) (runs: 256, μ: 352530, ~: 358030)
Suite result: ok. 5 passed; 0 failed; 0 skipped; finished in 9.93s (32.54s CPU time)

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

Copy link
Contributor

🔧 Build logs
Compiling 86 files with Solc 0.8.27
Solc 0.8.27 finished in 3.17s
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              |
|-----------------------------+------------------+-------------------+--------------------+---------------------|
| RescuableACL                | 1,695            | 1,827             | 22,881             | 47,325              |
|-----------------------------+------------------+-------------------+--------------------+---------------------|
| 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.22s
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

🔮 Coverage report
File Line Coverage Function Coverage Branch Coverage
src/contracts/access-control/OwnableWithGuardian.sol ${\color{orange}88.89\%}$
$16 / 18$
19, 20
${\color{orange}87.5\%}$
$7 / 8$
OwnableWithGuardian.onlyOwnerOrGuardian
${\color{green}100\%}$
$4 / 4$
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/Ownable.sol ${\color{red}76.47\%}$
$13 / 17$
36, 37, 62, 63
${\color{red}71.43\%}$
$5 / 7$
Ownable.onlyOwner, Ownable.renounceOwnership
${\color{red}50\%}$
$2 / 4$
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

Gas report

Create3Factory

  • size: 1122 / 49152
Method min mean median max calls
create(bytes32,bytes) ↑1.9%42275 ↑0.25%210421 ↑0.27%294494 294494 3

TransparentProxyFactory

  • size: 5342 / 49152
Method min mean median max calls
createDeterministic(address,address,bytes,bytes32) 384322 384616 384616 384910 2
createDeterministicProxyAdmin(address,bytes32) 288356 288644 288644 288932 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: 1198 / 49152
Method min mean median max calls
updateGuardian(address) 26169 30021 30519 30519 9

Rescuable721

  • size: 2233 / 49152
Method min mean median max calls
emergency721TokenTransfer(address,address,uint256) 22423 ↓-0.22%40970 ↓-0.22%40970 ↓-0.32%59517 2

ImplOwnableWithGuardian

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

@sakulstra sakulstra merged commit 804c23f into main Jan 27, 2025
1 check passed
@sakulstra sakulstra deleted the refactor/remove-safecast branch January 27, 2025 13:29
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