Skip to content

Extend type converter to avoid duplication of file in different projects #49

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.

Sign up for GitHub

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

Jump to bottom
Open
toninorair wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Extend type converter to avoid duplication of file in different projects #49

toninorair wants to merge 1 commit into from
+30 −10

Conversation

@toninorair
Copy link
Collaborator

@toninorair toninorair commented Dec 13, 2025 •
edited
Loading

Extend the type converter to eliminate copy-paste in the portal-v2 project.
Additionally, review whether most of its methods are still needed, given the previously introduced UintMath.

@github-actions
Copy link

github-actions bot commented Dec 13, 2025

Changes to gas cost

Generated at commit: 2ebffbc154a692276277c1ce852747fc2ff9af40, compared to commit: e54273e79cbd8a38b2d2b41ca4123f4e671fcc07

🧾 Summary (20% most significant diffs)

Contract Method Avg (+/-) %
ERC20ExtendedHandler approve
burn
transfer
transferFrom		 -1,103 ✅
+568 ❌
+9,849 ❌
+7,812 ❌		 -2.10%
+1.12%
+15.49%
+12.26%
Proxy fallback
initialize		 +3,075 ❌
-11,030 ✅		 +7.89%
-6.54%
ERC20ExtendedHarness approve
transfer
transferFrom		 -556 ✅
+1,506 ❌
+355 ❌		 -1.19%
+4.79%
+1.04%
Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
ERC20ExtendedHandler 1,370,312 (0) approve
burn
mint
transfer
transferFrom		 32,689 (-204)
48,291 (0)
679 (0)
780 (0)
65,809 (+64,923)		 -0.62%
0.00%
0.00%
0.00%
+7327.65%		 51,400 (-1,103)
51,091 (+568)
73,671 (-1,186)
73,449 (+9,849)
71,541 (+7,812)		 -2.10%
+1.12%
-1.58%
+15.49%
+12.26%		 52,877 (+12)
50,619 (0)
69,251 (0)
68,796 (+5,000)
69,256 (+5,012)		 +0.02%
0.00%
0.00%
+7.84%
+7.80%		 53,189 (0)
59,631 (+8,816)
103,691 (0)
140,649 (+76,657)
143,130 (+78,678)		 0.00%
+17.35%
0.00%
+119.79%
+122.07%		 1,245 (0)
1,331 (0)
1,287 (0)
1,251 (0)
1,311 (0)
Proxy 107,489 (0) approve
burn
fallback
initialize
mint
permit
transfer
transferFrom		 31,494 (-216)
29,338 (0)
5,296 (+81)
148,698 (-19,972)
34,109 (0)
29,832 (-24)
29,339 (0)
30,262 (0)		 -0.68%
0.00%
+1.55%
-11.84%
0.00%
-0.08%
0.00%
0.00%		 51,668 (-20)
34,613 (+4)
42,060 (+3,075)
157,640 (-11,030)
73,660 (-215)
71,136 (+1)
41,109 (+54)
41,378 (-32)		 -0.04%
+0.01%
+7.89%
-6.54%
-0.29%
+0.00%
+0.13%
-0.08%		 51,766 (+12)
31,795 (+163)
10,686 (-1,810)
148,698 (-19,972)
73,993 (-12)
86,791 (+14)
41,018 (0)
35,904 (+108)		 +0.02%
+0.52%
-14.48%
-11.84%
-0.02%
+0.02%
0.00%
+0.30%		 51,994 (0)
40,115 (+24)
120,934 (-27,764)
168,670 (0)
74,293 (0)
87,723 (0)
52,697 (0)
58,776 (+24)		 0.00%
+0.06%
-18.67%
0.00%
0.00%
0.00%
0.00%
+0.04%		 1,029 (0)
513 (0)
60 (-7,817)
67 (+37)
3,598 (+1,542)
1,542 (0)
514 (0)
772 (0)
ERC20ExtendedHarness 2,825,346 (0) approve
burn
mint
permit
receiveWithAuthorization(address,address,uint256,uint256,uint256,bytes32,bytes)
receiveWithAuthorization(address,address,uint256,uint256,uint256,bytes32,bytes32,bytes32)
receiveWithAuthorization(address,address,uint256,uint256,uint256,bytes32,uint8,bytes32,bytes32)
transfer
transferFrom
transferWithAuthorization(address,address,uint256,uint256,uint256,bytes32,bytes)
transferWithAuthorization(address,address,uint256,uint256,uint256,bytes32,bytes32,bytes32)
transferWithAuthorization(address,address,uint256,uint256,uint256,bytes32,uint8,bytes32,bytes32)		 26,648 (-204)
24,510 (-12)
29,060 (0)
24,971 (-24)
63,920 (0)
62,964 (0)
32,009 (0)
24,511 (-12)
25,373 (-12)
63,774 (0)
62,950 (0)
31,918 (0)		 -0.76%
-0.05%
0.00%
-0.10%
0.00%
0.00%
0.00%
-0.05%
-0.05%
0.00%
0.00%
0.00%		 46,079 (-556)
29,684 (+304)
64,781 (-369)
62,663 (-1)
81,843 (-76)
80,887 (-76)
78,639 (-72)
32,971 (+1,506)
34,428 (+355)
81,690 (-74)
80,866 (-74)
78,683 (-70)		 -1.19%
+1.03%
-0.57%
-0.00%
-0.09%
-0.09%
-0.09%
+4.79%
+1.04%
-0.09%
-0.09%
-0.09%		 46,884 (+24)
29,310 (0)
69,124 (+12)
77,595 (+22)
82,116 (0)
81,160 (0)
81,422 (-8)
29,589 (0)
32,789 (+204)
81,962 (+8)
81,138 (+8)
81,290 (+4)		 +0.05%
0.00%
+0.02%
+0.03%
0.00%
0.00%
-0.01%
0.00%
+0.63%
+0.01%
+0.01%
+0.00%		 47,148 (0)
35,294 (+24)
69,472 (0)
78,539 (0)
82,500 (0)
81,544 (0)
81,814 (0)
52,673 (+4,800)
55,189 (+1,328)
82,334 (0)
81,510 (0)
81,670 (0)		 0.00%
+0.07%
0.00%
0.00%
0.00%
0.00%
0.00%
+10.03%
+2.47%
0.00%
0.00%
0.00%		 2,393 (+119)
1,844 (0)
4,881 (0)
1,542 (0)
257 (0)
257 (0)
271 (0)
1,764 (+1)
2,083 (+7)
257 (0)
257 (0)
270 (0)
ContractHelperHarness 430,744 (0) getContractFrom 1,910 (0) 0.00% 2,099 (-17) -0.80% 2,145 (0) 0.00% 2,192 (0) 0.00% 269 (0)
ERC20ExtendedUpgradeableHarness 3,306,517 (0) approve
burn
mint
permit
receiveWithAuthorization(address,address,uint256,uint256,uint256,bytes32,bytes)
receiveWithAuthorization(address,address,uint256,uint256,uint256,bytes32,bytes32,bytes32)
receiveWithAuthorization(address,address,uint256,uint256,uint256,bytes32,uint8,bytes32,bytes32)
transferFrom
transferWithAuthorization(address,address,uint256,uint256,uint256,bytes32,bytes)
transferWithAuthorization(address,address,uint256,uint256,uint256,bytes32,bytes32,bytes32)
transferWithAuthorization(address,address,uint256,uint256,uint256,bytes32,uint8,bytes32,bytes32)		 5,348 (0)
3,176 (0)
7,750 (0)
1,989 (0)
44,300 (0)
43,873 (0)
12,671 (0)
3,720 (0)
44,235 (0)
43,831 (0)
12,574 (0)		 0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%		 25,131 (-39)
8,196 (-33)
47,162 (-100)
42,870 (-52)
66,637 (-88)
66,210 (-88)
63,557 (-84)
16,050 (-99)
66,591 (-89)
66,187 (-89)
63,615 (-85)		 -0.15%
-0.40%
-0.21%
-0.12%
-0.13%
-0.13%
-0.13%
-0.61%
-0.13%
-0.13%
-0.13%		 25,248 (0)
7,772 (0)
47,550 (0)
58,544 (0)
67,000 (0)
66,573 (0)
66,661 (-20)
8,876 (0)
66,955 (0)
66,551 (0)
66,529 (-10)		 0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
-0.03%
0.00%
0.00%
0.00%
-0.02%		 25,248 (0)
13,372 (0)
47,550 (0)
58,914 (0)
67,000 (0)
66,573 (0)
66,681 (0)
36,456 (0)
66,955 (0)
66,551 (0)
66,549 (0)		 0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%		 1,029 (0)
513 (0)
3,598 (0)
1,542 (0)
257 (0)
257 (0)
271 (0)
772 (0)
257 (0)
257 (0)
270 (0)
Bytes32StringHarness 405,806 (0) toBytes32
toString		 1,765 (0)
1,141 (0)		 0.00%
0.00%		 1,818 (+1)
9,990 (-60)		 +0.06%
-0.60%		 1,846 (0)
11,865 (-410)		 0.00%
-3.34%		 1,846 (0)
14,265 (0)		 0.00%
0.00%		 292 (0)
292 (0)
TransferHelperHarness 801,322 (0) safeApprove
safeTransfer
safeTransferExactFrom
safeTransferFrom		 27,462 (0)
29,871 (+24)
39,733 (0)
35,975 (0)		 0.00%
+0.08%
0.00%
0.00%		 38,544 (-55)
42,505 (+3)
78,136 (+1)
49,133 (+3)		 -0.14%
+0.01%
+0.00%
+0.01%		 28,576 (-108)
31,140 (0)
78,230 (0)
36,783 (0)		 -0.38%
0.00%
0.00%
0.00%		 50,342 (0)
55,477 (0)
91,216 (0)
62,121 (0)		 0.00%
0.00%
0.00%
0.00%		 1,024 (0)
1,024 (0)
512 (0)
1,024 (0)
SignatureCheckerHarness 985,574 (0) isValidSignature
validateECDSASignature(address,bytes32,bytes32,bytes32)		 5,743 (0)
1,641 (0)		 0.00%
0.00%		 6,203 (+1)
5,492 (+1)		 +0.02%
+0.02%		 6,170 (0)
5,509 (0)		 0.00%
0.00%		 10,448 (0)
5,520 (0)		 0.00%
0.00%		 532 (0)
518 (0)
FeeOnTransferERC20 778,506 (-12)
StandardERC20 662,482 (-12)

@github-actions
Copy link

github-actions bot commented Dec 13, 2025

LCOV of commit 9ad62c4 during Forge Coverage #154

Summary coverage rate:
  lines......: 92.5% (433 of 468 lines)
  functions..: 92.2% (141 of 153 functions)
  branches...: no data found

Files changed coverage rate:
                                     |Lines       |Functions  |Branches    
  Filename                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================
  src/libs/TypeConverter.sol         |53.8%     13| 0.0%     5|    -      0

PierrickGT
PierrickGT requested changes Dec 15, 2025
View reviewed changes
src/libs/TypeConverter.sol
/// @notice Converts a uint256 to uint16, reverting if the value overflows.
/// @param value The uint256 value to convert.
/// @return The uint16 representation of the value.
function toUint16(uint256 value) internal pure returns (uint16) {
Copy link
Member

@PierrickGT PierrickGT Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add unit tests for these new functions.

Copy link
Member

@PierrickGT PierrickGT Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice it before but these functions are similar to the ones we have in UintMath.

common/src/libs/UIntMath.sol

Line 40 in e54273e

function safe16(uint256 n) internal pure returns (uint16) {

Copy link

@denalimarsh denalimarsh Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PierrickGT PierrickGT PierrickGT requested changes

@denalimarsh denalimarsh denalimarsh left review comments

@Oighty Oighty Awaiting requested review from Oighty

@0xIryna 0xIryna Awaiting requested review from 0xIryna

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@toninorair @PierrickGT @denalimarsh