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

chore: [C4] Add first batch of Gas optimization #695

Merged
merged 12 commits into from
Sep 27, 2023
Merged

chore: [C4] Add first batch of Gas optimization #695

merged 12 commits into from
Sep 27, 2023

Conversation

YamenMerhi
Copy link
Member

What does this PR introduce?

  • Add first batch of Gas optimization

PR Checklist

  • Wrote Tests
  • Wrote & Generated Documentation (readme/natspec/dodoc)
  • Ran npm run lint && npm run lint:solidity (solhint)
  • Ran npm run format (prettier)
  • Ran npm run build
  • Ran npm run test

@github-actions
Copy link
Contributor

👋 Hello
⛽ I am the Gas Bot Reporter. I keep track of the gas costs of common interactions using Universal Profiles 🆙 !
📊 Here is a summary of the gas cost with the code introduced by this PR.

⛽📊 See Gas Benchmark report of Using UniversalProfile owned by an EOA

🔀 execute scenarios

execute scenarios - 👑 UP Owner ⛽ Gas Usage
Transfer 1 LYX to an EOA without data 37537
Transfer 1 LYX to a UP without data 36639
Transfer 1 LYX to an EOA with 256 bytes of data 42174
Transfer 1 LYX to a UP with 256 bytes of data 44867
Transfer 0.1 LYX to 3x EOA without data 70862
Transfer 0.1 LYX to 3x UP without data 75680
Transfer 0.1 LYX to 3x EOA with 256 bytes of data 84826
Transfer 0.1 LYX to 3x EOA with 256 bytes of data 100285

🗄️ setData scenarios

setData scenarios - 👑 UP Owner ⛽ Gas Usage
Set a 20 bytes long value 49971
Set a 60 bytes long value 95293
Set a 160 bytes long value 164453
Set a 300 bytes long value 279700
Set a 600 bytes long value 484112
Change the value of a data key already set 32859
Remove the value of a data key already set 27333
Set 2 data keys of 20 bytes long value 78428
Set 2 data keys of 100 bytes long value 260568
Set 3 data keys of 20 bytes long value 105133
Change the value of three data keys already set of 20 bytes long value 45445
Remove the value of three data keys already set 41339

🗄️ Tokens scenarios

Tokens scenarios - 👑 UP Owner ⛽ Gas Usage
Minting a LSP7Token to a UP (No Delegate) from an EOA 91241
Minting a LSP7Token to an EOA from an EOA 59206
Transferring an LSP7Token from a UP to another UP (No Delegate) 98552
Minting a LSP8Token to a UP (No Delegate) from an EOA 158173
Minting a LSP8Token to an EOA from an EOA 126138
Transferring an LSP8Token from a UP to another UP (No Delegate) 147236

📝 Notes

  • The execute and setData scenarios are executed on a fresh UniversalProfile smart contract, deployed as standard contracts (not as proxy behind a base contract implementation).
⛽📊 See Gas Benchmark report of Using UniversalProfile owned by an LSP6KeyManager

This document contains the gas usage for common interactions and scenarios when using UniversalProfile smart contracts.

🔀 execute scenarios

👑 unrestricted controller

execute scenarios - 👑 main controller ⛽ Gas Usage
transfer LYX to an EOA 60449
transfer LYX to a UP 62051
transfer tokens (LSP7) to an EOA (no data) 107077
transfer tokens (LSP7) to a UP (no data) 242592
transfer a NFT (LSP8) to a EOA (no data) 171061
transfer a NFT (LSP8) to a UP (no data) 289802

🛃 restricted controller

execute scenarios - 🛃 restricted controller ⛽ Gas Usage
transfer some LYXes to an EOA - restricted to 1 x allowed address only (TRANSFERVALUE + 1x AllowedCalls) 71641
transfers some tokens (LSP7) to an EOA - restricted to LSP7 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 121728
transfers some tokens (LSP7) to an other UP - restricted to LSP7 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 257243
transfers a NFT (LSP8) to an EOA - restricted to LSP8 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 185700
transfers a NFT (LSP8) to an other UP - restricted to LSP8 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 304441

🗄️ setData scenarios

👑 unrestricted controller

setData scenarios - 👑 main controller ⛽ Gas Usage
updates profile details (LSP3Profile metadata) 136872
give permissions to a controller (AddressPermissions[] + AddressPermissions[index] + AddressPermissions:Permissions:) 133007
restrict a controller to some specific ERC725Y Data Keys 139279
restrict a controller to interact only with 3x specific addresses 161983
remove a controller (its permissions + its address from the AddressPermissions[] array) 67986
write 5x LSP12 Issued Assets 233250

🛃 restricted controller

setData scenarios - 🛃 restricted controller ⛽ Gas Usage
setData(bytes32,bytes) -> updates 1x data key 102773
setData(bytes32[],bytes[]) -> updates 3x data keys (first x3) 161512
setData(bytes32[],bytes[]) -> updates 3x data keys (middle x3) 145666
setData(bytes32[],bytes[]) -> updates 3x data keys (last x3) 170999
setData(bytes32[],bytes[]) -> updates 2x data keys + add 3x new controllers (including setting the array length + indexes under AddressPermissions[index]) 250075

📝 Notes

  • The execute and setData scenarios are executed on a fresh UniversalProfile and LSP6KeyManager smart contracts, deployed as standard contracts (not as proxy behind a base contract implementation).

@YamenMerhi YamenMerhi marked this pull request as draft August 31, 2023 17:18
@github-actions
Copy link
Contributor

github-actions bot commented Aug 31, 2023

Changes to gas cost

Generated at commit: 244d821c8d29a9e0f81a924202f8815f154dbc89, compared to commit: b8eca3c5696acf85239130ef67edec9e8c134bfa

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
LSP6ExecuteRestrictedController lsp20VerifyCall
transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
-200 ✅
-138 ✅
-138 ✅
-335 ✅
-479 ✅
-148 ✅
-328 ✅
-1.17%
-0.25%
-0.44%
-0.26%
-0.20%
-0.20%
-0.16%
LSP6ExecuteUnrestrictedController lsp20VerifyCall
transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
-200 ✅
-138 ✅
-138 ✅
-335 ✅
-479 ✅
-148 ✅
-328 ✅
-1.17%
-0.24%
-0.41%
-0.26%
-0.20%
-0.21%
-0.16%
LSP6SetDataRestrictedController execute
givePermissionsToController
restrictControllerToERC725YKeys
-148 ✅
+180 ❌
+180 ❌
-0.52%
+0.15%
+0.13%
LSP6SetDataUnrestrictedController execute
givePermissionsToController
-148 ✅
+180 ❌
-0.52%
+0.14%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
LSP6ExecuteRestrictedController 2,862,294 (-13,397) lsp20VerifyCall
transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
14,633 (-263)
56,027 (-138)
31,358 (-138)
130,434 (-335)
238,835 (-479)
73,544 (-148)
208,831 (-328)
-1.77%
-0.25%
-0.44%
-0.26%
-0.20%
-0.20%
-0.16%
16,832 (-200)
56,027 (-138)
31,358 (-138)
130,434 (-335)
238,835 (-479)
73,544 (-148)
208,831 (-328)
-1.17%
-0.25%
-0.44%
-0.26%
-0.20%
-0.20%
-0.16%
17,565 (-180)
56,027 (-138)
31,358 (-138)
130,434 (-335)
238,835 (-479)
73,544 (-148)
208,831 (-328)
-1.01%
-0.25%
-0.44%
-0.26%
-0.20%
-0.20%
-0.16%
17,565 (-180)
56,027 (-138)
31,358 (-138)
130,434 (-335)
238,835 (-479)
73,544 (-148)
208,831 (-328)
-1.01%
-0.25%
-0.44%
-0.26%
-0.20%
-0.20%
-0.16%
8 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6ExecuteUnrestrictedController 2,862,294 (-13,397) lsp20VerifyCall
transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
14,633 (-263)
56,657 (-138)
33,358 (-138)
129,183 (-335)
237,584 (-479)
71,980 (-148)
207,267 (-328)
-1.77%
-0.24%
-0.41%
-0.26%
-0.20%
-0.21%
-0.16%
16,832 (-200)
56,657 (-138)
33,358 (-138)
129,183 (-335)
237,584 (-479)
71,980 (-148)
207,267 (-328)
-1.17%
-0.24%
-0.41%
-0.26%
-0.20%
-0.21%
-0.16%
17,565 (-180)
56,657 (-138)
33,358 (-138)
129,183 (-335)
237,584 (-479)
71,980 (-148)
207,267 (-328)
-1.01%
-0.24%
-0.41%
-0.26%
-0.20%
-0.21%
-0.16%
17,565 (-180)
56,657 (-138)
33,358 (-138)
129,183 (-335)
237,584 (-479)
71,980 (-148)
207,267 (-328)
-1.01%
-0.24%
-0.41%
-0.26%
-0.20%
-0.21%
-0.16%
8 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6SetDataRestrictedController 2,847,070 (-13,398) execute
givePermissionsToController
restrictControllerToERC725YKeys
18,352 (-138)
120,814 (+180)
139,064 (+180)
-0.75%
+0.15%
+0.13%
28,387 (-148)
120,814 (+180)
139,064 (+180)
-0.52%
+0.15%
+0.13%
28,387 (-148)
120,814 (+180)
139,064 (+180)
-0.52%
+0.15%
+0.13%
38,422 (-159)
120,814 (+180)
139,064 (+180)
-0.41%
+0.15%
+0.13%
2 (0)
1 (0)
1 (0)
LSP6SetDataUnrestrictedController 2,847,070 (-13,398) execute
givePermissionsToController
restrictControllerToERC725YKeys
18,352 (-138)
126,814 (+180)
147,564 (+180)
-0.75%
+0.14%
+0.12%
28,387 (-148)
126,814 (+180)
147,564 (+180)
-0.52%
+0.14%
+0.12%
28,387 (-148)
126,814 (+180)
147,564 (+180)
-0.52%
+0.14%
+0.12%
38,422 (-159)
126,814 (+180)
147,564 (+180)
-0.41%
+0.14%
+0.12%
2 (0)
1 (0)
1 (0)

@YamenMerhi YamenMerhi marked this pull request as ready for review September 26, 2023 07:42
@@ -329,37 +331,43 @@ abstract contract LSP6KeyManagerCore is
isSetData = true;
}

address target_ = _target;
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of using postfix underscore (except constructor arguments). Would suggest a different variable name in camelCase to maintain Solidity style guide

Suggested change
address target_ = _target;
address targetContract = _target;

@@ -396,14 +404,20 @@ abstract contract LSP6KeyManagerCore is
isSetData = true;
}

bool isReentrantCall = _nonReentrantBefore(isSetData, msg.sender);
address target_ = _target;
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above

contracts/LSP6KeyManager/LSP6KeyManagerCore.sol Outdated Show resolved Hide resolved
@CJ42
Copy link
Member

CJ42 commented Sep 27, 2023

Amazing! 🚀

@CJ42 CJ42 merged commit f1686d5 into develop Sep 27, 2023
26 checks passed
@CJ42 CJ42 deleted the c4/Gas branch September 27, 2023 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants