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.

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

Gas Optimizations For BApps Token Matching #4

Merged
merged 30 commits into from
Jan 31, 2025
Merged

Gas Optimizations For BApps Token Matching #4

merged 30 commits into from
Jan 31, 2025

Conversation

riccardo-ssvlabs
Copy link
Contributor

No description provided.

@riccardo-ssvlabs riccardo-ssvlabs changed the base branch from test/strategy-initial to main January 28, 2025 16:10
@riccardo-ssvlabs riccardo-ssvlabs self-assigned this Jan 29, 2025
@riccardo-ssvlabs riccardo-ssvlabs added the feature New feature or request label Jan 29, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 30, 2025 •
edited
Loading

Changes to gas cost

Generated at commit: 6400abe916480fcf339c1d66f2bd5f3ac93f5bf7, compared to commit: 6bf1a955596f2a9c4e84c0ef187353baaa3ea5e4

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
BasedAppManager addTokensToBApp
optInToBApp
registerBApp		 -19,554 ✅
-23,043 ✅
-57,784 ✅		 -61.27%
-22.31%
-49.62%
Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
BasedAppManager 3,975,997 (+560,896) FEE_EXPIRE_TIME
FEE_TIMELOCK_PERIOD
MAX_PERCENTAGE
OBLIGATION_TIMELOCK_PERIOD
WITHDRAWAL_EXPIRE_TIME
WITHDRAWAL_TIMELOCK_PERIOD
addTokensToBApp
createObligation
createStrategy
delegateBalance
delegations
depositERC20
depositETH
fastUpdateObligation
fastWithdrawERC20
fastWithdrawETH
finalizeFeeUpdate
finalizeUpdateObligation
finalizeWithdrawal
finalizeWithdrawalETH
initialize
maxFeeIncrement
obligationRequests
obligations
optInToBApp
owner
proposeFeeUpdate
proposeUpdateObligation
proposeWithdrawal
proposeWithdrawalETH
proxiableUUID
registerBApp
removeDelegatedBalance
strategies
strategyTokenBalances
totalDelegatedPercentage
updateDelegatedBalance
updateMetadataURI
upgradeToAndCall
usedTokens
withdrawalRequests		 263 (-42)
307 (+67)
316 (+56)
284 (+22)
262 (-22)
306 (+22)
3,121 (-2,617)
3,073 (+159)
425 (-28)
537 (-66)
808 (-33)
805 (+289)
325 (-88)
3,075 (+248)
761 (+289)
682 (+266)
2,817 (-105)
3,014 (-16)
3,096 (+240)
2,972 (+254)
2,875 (-8)
402 (+56)
990 (-87)
888 (+36)
3,409 (+203)
2,431 (+67)
2,711 (+23)
3,065 (+237)
518 (-44)
373 (-44)
386 (-51)
3,341 (-7)
2,688 (-51)
836 (0)
848 (-22)
637 (-11)
581 (+44)
3,166 (-75)
3,207 (-93)
739 (+12)
986 (+22)		 -13.77%
+27.92%
+21.54%
+8.40%
-7.75%
+7.75%
-45.61%
+5.46%
-6.18%
-10.95%
-3.92%
+56.01%
-21.31%
+8.77%
+61.23%
+63.94%
-3.59%
-0.53%
+8.40%
+9.35%
-0.28%
+16.18%
-8.08%
+4.23%
+6.33%
+2.83%
+0.86%
+8.38%
-7.83%
-10.55%
-11.67%
-0.21%
-1.86%
0.00%
-2.53%
-1.70%
+8.19%
-2.31%
-2.82%
+1.65%
+2.28%		 263 (-42)
307 (+67)
316 (+56)
284 (+22)
262 (-22)
306 (+22)
12,362 (-19,554)
53,567 (-10,016)
33,884 (-46)
28,791 (-68)
1,138 (-33)
55,299 (+371)
24,861 (-146)
6,504 (+195)
4,319 (+285)
6,741 (+251)
8,859 (-78)
7,810 (+19)
12,965 (+255)
10,823 (+273)
70,615 (+244)
1,629 (-33)
990 (-87)
888 (+36)
80,258 (-23,043)
2,431 (+67)
15,246 (+10)
38,249 (+4,040)
3,424 (-53)
49,530 (-102)
386 (-51)
58,666 (-57,784)
9,389 (-47)
1,796 (-1)
1,400 (-23)
637 (-11)
5,028 (+41)
4,104 (-52)
7,088 (-150)
741 (+13)
986 (+22)		 -13.77%
+27.92%
+21.54%
+8.40%
-7.75%
+7.75%
-61.27%
-15.75%
-0.14%
-0.24%
-2.82%
+0.68%
-0.58%
+3.09%
+7.06%
+3.87%
-0.87%
+0.24%
+2.01%
+2.59%
+0.35%
-1.99%
-8.08%
+4.23%
-22.31%
+2.83%
+0.07%
+11.81%
-1.52%
-0.21%
-11.67%
-49.62%
-0.50%
-0.06%
-1.62%
-1.70%
+0.82%
-1.25%
-2.07%
+1.79%
+2.28%		 263 (-42)
307 (+67)
316 (+56)
284 (+22)
262 (-22)
306 (+22)
3,429 (-28,487)
55,418 (-10,044)
29,610 (-46)
47,727 (-66)
808 (-33)
65,593 (+371)
24,885 (-146)
7,528 (+2,330)
3,036 (+283)
4,104 (+219)
4,882 (-72)
7,736 (+17)
3,280 (+240)
3,244 (+254)
72,046 (-50)
2,402 (+56)
990 (-87)
888 (+36)
81,427 (-21,503)
2,431 (+67)
5,145 (+17)
54,936 (+5,081)
2,896 (-53)
49,655 (-103)
386 (-51)
51,751 (-42,193)
12,740 (-45)
836 (0)
848 (-22)
637 (-11)
2,921 (+38)
4,104 (-52)
7,088 (-150)
739 (+12)
986 (+22)		 -13.77%
+27.92%
+21.54%
+8.40%
-7.75%
+7.75%
-89.26%
-15.34%
-0.16%
-0.14%
-3.92%
+0.57%
-0.58%
+44.82%
+10.28%
+5.64%
-1.45%
+0.22%
+7.89%
+8.49%
-0.07%
+2.39%
-8.08%
+4.23%
-20.89%
+2.83%
+0.33%
+10.19%
-1.80%
-0.21%
-11.67%
-44.91%
-0.35%
0.00%
-2.53%
-1.70%
+1.32%
-1.25%
-2.07%
+1.65%
+2.28%		 263 (-42)
307 (+67)
316 (+56)
284 (+22)
262 (-22)
306 (+22)
54,682 (-3,413)
55,418 (-10,210)
46,710 (-46)
47,727 (-66)
2,808 (-33)
65,593 (+371)
24,885 (-146)
36,994 (+26,074)
34,124 (+340)
18,076 (+299)
12,893 (-84)
44,640 (-16,958)
42,204 (+299)
26,187 (+310)
72,046 (-50)
2,402 (+56)
990 (-87)
888 (-1,964)
133,075 (-60,157)
2,431 (+67)
32,672 (+14)
54,936 (+5,081)
49,880 (-47)
49,655 (-103)
386 (-51)
149,547 (-383,982)
12,740 (-45)
2,836 (0)
2,848 (-22)
637 (-11)
13,422 (+44)
5,043 (-28)
10,969 (-208)
2,739 (+12)
986 (+22)		 -13.77%
+27.92%
+21.54%
+8.40%
-7.75%
+7.75%
-5.87%
-15.56%
-0.10%
-0.14%
-1.16%
+0.57%
-0.58%
+238.77%
+1.01%
+1.68%
-0.65%
-27.53%
+0.71%
+1.20%
-0.07%
+2.39%
-8.08%
-68.86%
-31.13%
+2.83%
+0.04%
+10.19%
-0.09%
-0.21%
-11.67%
-71.97%
-0.35%
0.00%
-0.77%
-1.70%
+0.33%
-0.55%
-1.86%
+0.44%
+2.28%		 512 (0)
514 (0)
15,995 (-491)
775 (+1)
257 (0)
771 (0)
12 (+8)
274 (+7)
23,818 (-988)
2,062 (0)
1,549 (0)
1,307 (0)
1,035 (0)
520 (-250)
272 (0)
4 (0)
515 (0)
520 (+1)
4 (0)
769 (0)
165 (+28)
417 (+28)
521 (0)
6,209 (-493)
5,679 (-249)
1 (0)
1,283 (0)
779 (+4)
262 (0)
770 (0)
1 (0)
5,473 (-227)
3 (0)
12,404 (-496)
4,670 (-4)
1,033 (0)
5 (0)
4 (0)
2 (0)
5,946 (-494)
1,028 (0)

MatheusFranco99
MatheusFranco99 reviewed Jan 30, 2025
View reviewed changes
src/interfaces/ICore.sol
uint32 sharedRiskLevel;
/// @notice Represents a SharedRiskLevel
struct SharedRiskLevel {
/// @dev The shared risk level
Copy link
Contributor

@MatheusFranco99 MatheusFranco99 Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

When the encoding for beta is decided, we could add a comment here explaining it. Perhaps, we should reach a decision now.

Mathematically, the logarithmic encoding ($x_{real} = a + b \log{(1 + x_{uint32})}$) may be the best approach as it's used when smaller numbers require more accuracy than large ones (our case I think). But it may generate more confusion since it's more complex.

If so, we could stick to the linear encoding ($x_{real} = \frac{x_{uint32}}{s}$). It's simpler and well-known. Perhaps, even simpler: $x_{real} = \frac{x_{uint32}}{10^d}$.

The type max value is $2^{32}-1 = 4294967295$.
So, I think $d$ between 4 and 6 have very good precision and maximum values (more than necessary for our case). I would stick to $d = 6$.

What do you think?
@riccardo-ssvlabs @mtabasco @GalRogozinski

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Closing as discussed

@MatheusFranco99 MatheusFranco99 mentioned this pull request Jan 30, 2025
Merged
@mtabasco
Copy link
Contributor

Create a private function that unifies timelock checks like:

if (block.timestamp < requestTime + WITHDRAWAL_TIMELOCK_PERIOD) revert ICore.WithdrawalTimelockNotElapsed();
        if (block.timestamp > requestTime + WITHDRAWAL_TIMELOCK_PERIOD + WITHDRAWAL_EXPIRE_TIME) {

mtabasco
mtabasco requested changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@mtabasco mtabasco left a comment

Choose a reason for hiding this comment

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

Great work @riccardo-ssvlabs ! Such great improvements in this iteration. Please check the comments. thanks!

src/interfaces/ICore.sol Outdated
@@ -19,7 +25,7 @@ interface ICore {
/// @dev The fee in percentage
uint32 fee;
/// @dev The proposed fee
uint32 feeProposed; // TODO: here is transparent, but could be moved outside in a separate mapping?
uint32 feeProposed;
/// @dev The proposed fee expiration time
uint256 feeUpdateTime;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reduce the type for timestamp-related variables? uint32 or playing safe with uint64

Copy link
Contributor

Choose a reason for hiding this comment

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

On this, particularly with ObligationRequest struct we can pack the data into 1 storage slot if we reduce the type of requestTime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i prefer to use uint256 because block.timestamp returns uint256 and that will avoid unnecessary typecasting

@mtabasco mtabasco mentioned this pull request Jan 30, 2025
Open
@riccardo-ssvlabs riccardo-ssvlabs merged commit e05d732 into main Jan 31, 2025
2 of 3 checks passed
@riccardo-ssvlabs riccardo-ssvlabs deleted the feat/gas-optimization-p0 branch February 3, 2025 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtabasco mtabasco mtabasco approved these changes

@MatheusFranco99 MatheusFranco99 MatheusFranco99 approved these changes

@liorrutenberg liorrutenberg Awaiting requested review from liorrutenberg

Assignees

@riccardo-ssvlabs riccardo-ssvlabs

Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@riccardo-ssvlabs @mtabasco @MatheusFranco99