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

fix: [L-09] restrict owner change in _beforeTokenTransfer #846

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

skimaharvey
Copy link
Member

What does this PR introduce?

  • Restrict owner change in _beforeTokenTransfer

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.

⛽📊 Gas Benchmark Report

Deployment Costs

Deployed contracts ⛽ Deployment cost
UniversalProfile 3168335 (0 )
KeyManager 3659003 (0 )
LSP1DelegateUP 1637510 (0 )
LSP7Mintable 2407823 (0 )
LSP8Mintable 2832815 (22,546 📈❌)

Runtime Costs

UniversalProfile owned by an 🔑 EOA

🔀 execute scenarios

execute scenarios - UP owned by 🔑 EOA ⛽ Gas Usage
Transfer 1 LYX to an EOA without data 37572 (0 )
Transfer 1 LYX to a UP without data 46265 (0 )
Transfer 1 LYX to an EOA with 256 bytes of data 42245 (12 📈❌)
Transfer 1 LYX to a UP with 256 bytes of data 57198 (12 📈❌)
Transfer 0.1 LYX to 3x EOA without data 70898 (0 )
Transfer 0.1 LYX to 3x UP without data 104489 (0 )
Transfer 0.1 LYX to 3x EOA with 256 bytes of data 84898 (24 📈❌)
Transfer 0.1 LYX to 3x UPs with 256 bytes of data 137245 (-36 📉✅)

🗄️ setData scenarios

setData scenarios - UP owned by 🔑 EOA ⛽ Gas Usage
Set a 20 bytes long value 49921 (0 )
Set a 60 bytes long value 95243 (0 )
Set a 160 bytes long value 164391 (-24 📉✅)
Set a 300 bytes long value 279296 (0 )
Set a 600 bytes long value 486656 (-24 📉✅)
Change the value of a data key already set 32809 (0 )
Remove the value of a data key already set 27283 (0 )
Set 2 data keys of 20 bytes long value 78428 (-12 📉✅)
Set 2 data keys of 100 bytes long value 260592 (-12 📉✅)
Set 3 data keys of 20 bytes long value 105128 (0 )
Change the value of three data keys already set of 20 bytes long value 45428 (0 )
Remove the value of three data keys already set 41325 (0 )

🗄️ Tokens scenarios

Tokens scenarios - UP owned by 🔑 EOA ⛽ Gas Usage
Minting a LSP7Token to a UP (No Delegate) from an EOA 93114 (0 )
Minting a LSP7Token to an EOA from an EOA 59390 (0 )
Transferring an LSP7Token from a UP to another UP (No Delegate) 102367 (0 )
Minting a LSP8Token to a UP (No Delegate) from an EOA 159961 (0 )
Minting a LSP8Token to an EOA from an EOA 126238 (0 )
Transferring an LSP8Token from a UP to another UP (No Delegate) 151147 (64 📈❌)
UniversalProfile owned by a 🔒📄 LSP6KeyManager

🔀 execute scenarios

execute scenarios 👑 main controller 🛃 restricted controller
LYX transfer --> to an EOA 64365 (0 ) 75316 (0 )
LYX transfer --> to a UP 78508 (0 ) 93410 (0 )
LSP7 token transfer --> to an EOA 116872 (0 ) 131622 (0 )
LSP7 token transfer --> to a UP 250518 (0 ) 265268 (0 )
LSP8 NFT transfer --> to an EOA 181045 (64 📈❌) 195795 (64 📈❌)
LSP8 NFT transfer --> to a UP 297893 (64 📈❌) 312643 (64 📈❌)

🗄️ setData scenarios

setData scenarios 👑 main controller 🛃 restricted controller
Update Profile details (LSP3Profile Metadata) 67264 (0 ) 77286 (0 )
Add a new controller with permission to SET_DATA + 3x allowed data keys:
AddressPermissions[]
+ AddressPermissions[index]
+ AddressPermissions:Permissions:<controller>
+ AddressPermissions:AllowedERC725YDataKeys:<controller)
209516 (0 ) 219673 (0 )
Update permissions of previous controller. Allow it now to SUPER_SETDATA 52292 (0 ) 55298 (0 )
Remove a controller:
1. decrease AddressPermissions[] Array length
2. remove the controller address at AddressPermissions[index]
3. set "0x" for the controller permissions under AddressPermissions:Permissions:
78765 (0 ) 90064 (0 )
Write 5x new LSP12 Issued Assets 66959 (0 ) 101586 (0 )
Update 3x data keys (first 3) 125411 (0 ) 159515 (0 )
Update 3x data keys (middle 3) 105499 (0 ) 143669 (0 )
Update 3x data keys (last 3) 125411 (0 ) 169002 (0 )
Set 2 x new data keys + add 3x new controllers 810121 (0 ) 871921 (0 )

Copy link
Contributor

github-actions bot commented Jan 10, 2024

Changes to gas cost

Generated at commit: 2f13e41fe40a943582710e9554ad0a0ac0ddc406, compared to commit: 0433a6b60f0cea17b78d24b063737cd4094c8f02

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
LSP6ExecuteUnrestrictedController transferNFTToRandomEOA
transferNFTToRandomUP
+35,581 ❌
+62,061 ❌
+25.04%
+25.03%
LSP6ExecuteRestrictedController transferNFTToRandomEOA +35,895 ❌ +25.04%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
LSP6ExecuteUnrestrictedController 3,047,114 (0) transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
78,207 (+15,641)
71,116 (+14,223)
177,651 (+35,581)
310,053 (+62,061)
92,600 (+18,520)
224,787 (+19,900)
+25.00%
+25.00%
+25.04%
+25.03%
+25.00%
+9.71%
78,207 (+15,641)
71,116 (+14,223)
177,651 (+35,581)
310,053 (+62,061)
92,600 (+18,520)
224,787 (+19,900)
+25.00%
+25.00%
+25.04%
+25.03%
+25.00%
+9.71%
78,207 (+15,641)
71,116 (+14,223)
177,651 (+35,581)
310,053 (+62,061)
92,600 (+18,520)
224,787 (+19,900)
+25.00%
+25.00%
+25.04%
+25.03%
+25.00%
+9.71%
78,207 (+15,641)
71,116 (+14,223)
177,651 (+35,581)
310,053 (+62,061)
92,600 (+18,520)
224,787 (+19,900)
+25.00%
+25.00%
+25.04%
+25.03%
+25.00%
+9.71%
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6ExecuteRestrictedController 3,047,114 (0) transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
77,723 (+15,544)
69,116 (+13,823)
179,221 (+35,895)
311,623 (+62,375)
94,170 (+18,834)
226,357 (+19,900)
+25.00%
+25.00%
+25.04%
+25.03%
+25.00%
+9.64%
77,723 (+15,544)
69,116 (+13,823)
179,221 (+35,895)
311,623 (+62,375)
94,170 (+18,834)
226,357 (+19,900)
+25.00%
+25.00%
+25.04%
+25.03%
+25.00%
+9.64%
77,723 (+15,544)
69,116 (+13,823)
179,221 (+35,895)
311,623 (+62,375)
94,170 (+18,834)
226,357 (+19,900)
+25.00%
+25.00%
+25.04%
+25.03%
+25.00%
+9.64%
77,723 (+15,544)
69,116 (+13,823)
179,221 (+35,895)
311,623 (+62,375)
94,170 (+18,834)
226,357 (+19,900)
+25.00%
+25.00%
+25.04%
+25.03%
+25.00%
+9.64%
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6SetDataRestrictedController 3,035,099 (0) execute 37,886 (+7,577) +25.00% 38,070 (+3,789) +11.05% 38,070 (+3,789) +11.05% 38,254 (0) 0.00% 2 (0)
LSP6SetDataUnrestrictedController 3,035,099 (0) execute 37,886 (+7,577) +25.00% 38,070 (+3,789) +11.05% 38,070 (+3,789) +11.05% 38,254 (0) 0.00% 2 (0)

@skimaharvey skimaharvey force-pushed the restrict-beforeTokenHook branch 4 times, most recently from 7499671 to 5e9bffd Compare January 10, 2024 15:30
Copy link
Member

@CJ42 CJ42 left a comment

Choose a reason for hiding this comment

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

Added review comments

@skimaharvey skimaharvey force-pushed the restrict-beforeTokenHook branch 2 times, most recently from e974a9e to 35ddbd5 Compare January 11, 2024 09:41
@skimaharvey skimaharvey merged commit 6dbeb53 into develop Jan 11, 2024
43 checks passed
@skimaharvey skimaharvey deleted the restrict-beforeTokenHook branch January 11, 2024 10:50
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