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

perf: reduce LSP1Delegate deployment cost by 8,678 gas #485

Merged
merged 3 commits into from
Feb 28, 2023

Conversation

CJ42
Copy link
Member

@CJ42 CJ42 commented Feb 25, 2023

What does this PR introduce?

🚀 Perfs

Reduce deployment cost of LSP1DelegateUP by 8.6k gas with the following refactorings:

  • Combine if statements with & when checking if the map key is related to a vaul.
  • use explicit return when calling the Key Manager, instead of assigning the return value of the external call (made to the Key Manager) to the result variable.
  • add local variable isMapValueSet to check if the map value is bytes12(0) or not, to improve readability.

@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

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 55442
transfer LYX to a UP 57044
transfer tokens (LSP7) to an EOA (no data) 101696
transfer tokens (LSP7) to a UP (no data) 261775
transfer a NFT (LSP8) to a EOA (no data) 165572
transfer a NFT (LSP8) to a UP (no data) 289531

🛃 restricted controller

execute scenarios - 🛃 restricted controller ⛽ Gas Usage
transfer some LYXes to an EOA - restricted to 1 x allowed address only (TRANSFERVALUE + 1x AllowedCalls) 62197
transfers some tokens (LSP7) to an EOA - restricted to LSP7 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 114110
transfers some tokens (LSP7) to an other UP - restricted to LSP7 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 274189
transfers a NFT (LSP8) to an EOA - restricted to LSP8 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 177986
transfers a NFT (LSP8) to an other UP - restricted to LSP8 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 301945

🗄️ setData scenarios

👑 unrestricted controller

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

🛃 restricted controller

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

📝 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).

@github-actions
Copy link
Contributor

github-actions bot commented Feb 25, 2023

Changes to gas cost

Generated at commit: 07023fd44ba477a1a50011d7093559681636a8da, compared to commit: d360371559c4be4b009cf34ac2db3c4cbc64d545

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
LSP6ExecuteUnrestrictedController transferTokensToRandomEOA +52 ❌ +0.07%
LSP6ExecuteRestrictedController transferTokensToRandomEOA +52 ❌ +0.07%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
LSP6ExecuteUnrestrictedController 2,425,959 (0) transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
130,018 (+42)
259,407 (+77)
75,773 (+52)
234,554 (+96)
+0.03%
+0.03%
+0.07%
+0.04%
130,018 (+42)
259,407 (+77)
75,773 (+52)
234,554 (+96)
+0.03%
+0.03%
+0.07%
+0.04%
130,018 (+42)
259,407 (+77)
75,773 (+52)
234,554 (+96)
+0.03%
+0.03%
+0.07%
+0.04%
130,018 (+42)
259,407 (+77)
75,773 (+52)
234,554 (+96)
+0.03%
+0.03%
+0.07%
+0.04%
1 (0)
1 (0)
1 (0)
1 (0)
LSP6ExecuteRestrictedController 2,425,959 (0) transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
131,032 (+41)
260,421 (+77)
77,041 (+52)
235,822 (+96)
+0.03%
+0.03%
+0.07%
+0.04%
131,032 (+41)
260,421 (+77)
77,041 (+52)
235,822 (+96)
+0.03%
+0.03%
+0.07%
+0.04%
131,032 (+41)
260,421 (+77)
77,041 (+52)
235,822 (+96)
+0.03%
+0.03%
+0.07%
+0.04%
131,032 (+41)
260,421 (+77)
77,041 (+52)
235,822 (+96)
+0.03%
+0.03%
+0.07%
+0.04%
1 (0)
1 (0)
1 (0)
1 (0)

@skimaharvey
Copy link
Member

Screenshot 2023-02-25 at 19 07 51

since you are returning, do we need to keep in in the stack?

Copy link
Member

@YamenMerhi YamenMerhi left a comment

Choose a reason for hiding this comment

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

Not sure how this refactoring is increasing the gas cost, but we should really stop favoring deployment cost over runtime cost even if it was a big decrease in deployment and a small increase in runtime, because as we know this contract will only be deployed once and will run twice on each token transfer.

LGTM 🚀 Let's merge and keep that in mind for our next PRs!

@CJ42
Copy link
Member Author

CJ42 commented Feb 27, 2023

Screenshot 2023-02-25 at 19 07 51

since you are returning, do we need to keep in in the stack?

We could remove the variable name, but maybe it might be useful for web3.js/ethers.js if the universalReceiver(...) function from LSP1Delegate is called directly via staticcall for instance. The returned object on a dApp would contain the result property.

I have removed them from _whenSending + _whenReceiving, since it's now returned directly and these are internal function.

f58677f

@YamenMerhi @skimaharvey

@CJ42 CJ42 merged commit 0c2a669 into develop Feb 28, 2023
@CJ42 CJ42 deleted the refactor/lsp1-deployment-cost branch February 28, 2023 08:44
JeneaVranceanu added a commit to JeneaVranceanu/lsp-universalprofile-smart-contracts that referenced this pull request Mar 1, 2023
* develop:
  refactor!: change LSP5/6/10 Array length from `uint256` to `uint128` (lukso-network#482)
  ci: temporarily disable downloading Android artifacts on github release (lukso-network#483)
  perf: reduce LSP1Delegate deployment cost by 8,678 gas (lukso-network#485)
  feat!: Add batchCalls function in LSP0 and LSP9 (lukso-network#476)
  test: add tests for edge cases with LSP1 returned values (e.g: `LSP1: asset sent is not registered`, ...) (lukso-network#479)
  refactor!: change `CHANGEPERMISSIONS` to `EDITPERMISSIONS` (lukso-network#481)
  ci: check deployer balance before deploy + verify contracts (lukso-network#480)
  ci: move ci scripts in a `ci/` subfolder (lukso-network#478)
  docs: add extra note about Address lib in CHANGELOG 0.8.1 (lukso-network#477)
  chore: prepare release 0.8.1 (lukso-network#474)
  refactor: replace param with literal LSP9 interfaceId (lukso-network#472)
  refactor: Support Extension for `bytes4(0)` sig in LSP0-LSP9 (lukso-network#473)
  refactor: separate the LSP6 core contract in logic modules (lukso-network#461)
  feat: create `LSP0Utils` library with functions to retrieve LSP1 addresses (lukso-network#466)
  ci: update CI to publish android artifacts + disable temporarily android artifacts build (web3 not working) (lukso-network#469)
  chore: remove Address library (lukso-network#471)
  fix: Add typescript transpilation and hook in package.json exports (lukso-network#470)

# Conflicts:
#	.gitignore
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.

3 participants