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!: replace tuple value for LSP5/10 from bytes8 -> uint128 #486

Merged
merged 2 commits into from
Mar 6, 2023

Conversation

CJ42
Copy link
Member

@CJ42 CJ42 commented Feb 28, 2023

What does this PR introduce?

⚠️ BREAKING CHANGE

  • Change the value stored under the LSP5/10 tuples from bytes4,bytes8) to --> (bytes4,uint128)

  • add checks to test when the value under the LSP5ReceivedAssets[] and LSP10Vaults[] array is the max possible index and introduce custom error to handle this case.

This is to prevent a Panic(uint256) error type with error code 0x11 due to arithmetic overflow with max(uint128) + 1

This PR follows the changes introduced in the LSP5 and LSP10 specs.
See this PR in the LIP repository for reference: lukso-network/LIPs#173

@CJ42 CJ42 added the help wanted Extra attention is needed label Feb 28, 2023
@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) 101718
transfer tokens (LSP7) to a UP (no data) 239674
transfer a NFT (LSP8) to a EOA (no data) 165594
transfer a NFT (LSP8) to a UP (no data) 287320

🛃 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) 114132
transfers some tokens (LSP7) to an other UP - restricted to LSP7 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 252088
transfers a NFT (LSP8) to an EOA - restricted to LSP8 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 178008
transfers a NFT (LSP8) to an other UP - restricted to LSP8 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data) 299734

🗄️ setData scenarios

👑 unrestricted controller

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

🛃 restricted controller

setData scenarios - 🛃 restricted controller ⛽ Gas Usage
setData(bytes32,bytes) -> updates 1x data key 101339
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 28, 2023

Changes to gas cost

Generated at commit: 73dfc7348722ed9182e71c891fcef220c7be861a, compared to commit: 6bcfd4d60d66838f055f8100f6b66bafc9b61a61

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
LSP6ExecuteUnrestrictedController transferNFTToRandomEOA +5 ❌ +0.00%
LSP6ExecuteRestrictedController transferNFTToRandomEOA +5 ❌ +0.00%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
LSP6ExecuteUnrestrictedController 2,439,174 (0) transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomUP
129,873 (+5)
241,544 (+7)
212,407 (+3)
+0.00%
+0.00%
+0.00%
129,873 (+5)
241,544 (+7)
212,407 (+3)
+0.00%
+0.00%
+0.00%
129,873 (+5)
241,544 (+7)
212,407 (+3)
+0.00%
+0.00%
+0.00%
129,873 (+5)
241,544 (+7)
212,407 (+3)
+0.00%
+0.00%
+0.00%
1 (0)
1 (0)
1 (0)
LSP6ExecuteRestrictedController 2,439,174 (0) transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomUP
130,888 (+5)
242,559 (+7)
213,675 (+3)
+0.00%
+0.00%
+0.00%
130,888 (+5)
242,559 (+7)
213,675 (+3)
+0.00%
+0.00%
+0.00%
130,888 (+5)
242,559 (+7)
213,675 (+3)
+0.00%
+0.00%
+0.00%
130,888 (+5)
242,559 (+7)
213,675 (+3)
+0.00%
+0.00%
+0.00%
1 (0)
1 (0)
1 (0)

@CJ42 CJ42 added help wanted Extra attention is needed and removed help wanted Extra attention is needed labels Mar 1, 2023
@CJ42
Copy link
Member Author

CJ42 commented Mar 1, 2023

not forget to remove the custom errors that are not used anymore from the constants.ts

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.

LGTM

@CJ42 CJ42 merged commit 83a05db into develop Mar 6, 2023
@CJ42 CJ42 deleted the refactor/lsp5-10-tuples branch March 6, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Development

Successfully merging this pull request may close these issues.

3 participants