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: [M-01] add notice in Natspec for isValidSignature #787

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

CJ42
Copy link
Member

@CJ42 CJ42 commented Nov 3, 2023

No description provided.

@CJ42 CJ42 changed the title chore: add notice in Natspec for isValidSignature chore: [M-01] add notice in Natspec for isValidSignature Nov 3, 2023
Copy link
Contributor

github-actions bot commented Nov 3, 2023

Changes to gas cost

Generated at commit: d98cd0df62070f7f8099888f91106d49fa0b87b4, compared to commit: 1f92ca530fb56cf2b626b0feae311d6c4737b8a0

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
LSP6ExecuteRestrictedController transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
-210 ✅
-350 ✅
-324 ✅
-323 ✅
-308 ✅
-385 ✅
-0.34%
-0.63%
-0.23%
-0.13%
-0.41%
-0.19%
LSP6ExecuteUnrestrictedController transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
-350 ✅
-350 ✅
-352 ✅
-352 ✅
-337 ✅
-421 ✅
-0.56%
-0.61%
-0.25%
-0.14%
-0.45%
-0.21%
LSP6SetDataRestrictedController execute
givePermissionsToController
-197 ✅
-42 ✅
-0.57%
-0.03%
LSP6SetDataUnrestrictedController execute
givePermissionsToController
-197 ✅
-42 ✅
-0.57%
-0.03%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
LSP6ExecuteRestrictedController 3,046,507 (+2,200) transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
62,198 (-210)
55,389 (-350)
143,044 (-324)
248,665 (-323)
75,009 (-308)
205,805 (-385)
-0.34%
-0.63%
-0.23%
-0.13%
-0.41%
-0.19%
62,198 (-210)
55,389 (-350)
143,044 (-324)
248,665 (-323)
75,009 (-308)
205,805 (-385)
-0.34%
-0.63%
-0.23%
-0.13%
-0.41%
-0.19%
62,198 (-210)
55,389 (-350)
143,044 (-324)
248,665 (-323)
75,009 (-308)
205,805 (-385)
-0.34%
-0.63%
-0.23%
-0.13%
-0.41%
-0.19%
62,198 (-210)
55,389 (-350)
143,044 (-324)
248,665 (-323)
75,009 (-308)
205,805 (-385)
-0.34%
-0.63%
-0.23%
-0.13%
-0.41%
-0.19%
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6ExecuteUnrestrictedController 3,046,507 (+2,200) transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
62,585 (-350)
56,989 (-350)
141,788 (-352)
247,409 (-352)
73,753 (-337)
204,235 (-421)
-0.56%
-0.61%
-0.25%
-0.14%
-0.45%
-0.21%
62,585 (-350)
56,989 (-350)
141,788 (-352)
247,409 (-352)
73,753 (-337)
204,235 (-421)
-0.56%
-0.61%
-0.25%
-0.14%
-0.45%
-0.21%
62,585 (-350)
56,989 (-350)
141,788 (-352)
247,409 (-352)
73,753 (-337)
204,235 (-421)
-0.56%
-0.61%
-0.25%
-0.14%
-0.45%
-0.21%
62,585 (-350)
56,989 (-350)
141,788 (-352)
247,409 (-352)
73,753 (-337)
204,235 (-421)
-0.56%
-0.61%
-0.25%
-0.14%
-0.45%
-0.21%
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6SetDataRestrictedController 3,034,499 (+2,208) execute
givePermissionsToController
restrictControllerToERC725YKeys
30,328 (-352)
122,644 (-42)
141,430 (-43)
-1.15%
-0.03%
-0.03%
34,317 (-197)
122,644 (-42)
141,430 (-43)
-0.57%
-0.03%
-0.03%
34,317 (-197)
122,644 (-42)
141,430 (-43)
-0.57%
-0.03%
-0.03%
38,306 (-43)
122,644 (-42)
141,430 (-43)
-0.11%
-0.03%
-0.03%
2 (0)
1 (0)
1 (0)
LSP6SetDataUnrestrictedController 3,034,499 (+2,208) execute
givePermissionsToController
restrictControllerToERC725YKeys
30,328 (-352)
128,644 (-42)
149,930 (-43)
-1.15%
-0.03%
-0.03%
34,317 (-197)
128,644 (-42)
149,930 (-43)
-0.57%
-0.03%
-0.03%
34,317 (-197)
128,644 (-42)
149,930 (-43)
-0.57%
-0.03%
-0.03%
38,306 (-43)
128,644 (-42)
149,930 (-43)
-0.11%
-0.03%
-0.03%
2 (0)
1 (0)
1 (0)

Copy link
Contributor

github-actions bot commented Nov 3, 2023

👋 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 3157537 (0 )
KeyManager 3658355 (0 )
LSP1DelegateUP 1636034 (0 )
LSP7Mintable 2350310 (0 )
LSP8Mintable 2443708 (0 )

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 37560 (0 )
Transfer 1 LYX to a UP without data 46253 (0 )
Transfer 1 LYX to an EOA with 256 bytes of data 42221 (-12 📉✅)
Transfer 1 LYX to a UP with 256 bytes of data 57186 (24 📈❌)
Transfer 0.1 LYX to 3x EOA without data 70862 (0 )
Transfer 0.1 LYX to 3x UP without data 104453 (0 )
Transfer 0.1 LYX to 3x EOA with 256 bytes of data 84814 (-24 📉✅)
Transfer 0.1 LYX to 3x UPs with 256 bytes of data 137245 (0 )

🗄️ setData scenarios

setData scenarios - UP owned by 🔑 EOA ⛽ Gas Usage
Set a 20 bytes long value 49973 (0 )
Set a 60 bytes long value 95295 (0 )
Set a 160 bytes long value 164443 (-12 📉✅)
Set a 300 bytes long value 279690 (0 )
Set a 600 bytes long value 484126 (-12 📉✅)
Change the value of a data key already set 32861 (0 )
Remove the value of a data key already set 27335 (0 )
Set 2 data keys of 20 bytes long value 78456 (12 📈❌)
Set 2 data keys of 100 bytes long value 260620 (0 )
Set 3 data keys of 20 bytes long value 105174 (12 📈❌)
Change the value of three data keys already set of 20 bytes long value 45474 (12 📈❌)
Remove the value of three data keys already set 41362 (0 )

🗄️ Tokens scenarios

Tokens scenarios - UP owned by 🔑 EOA ⛽ Gas Usage
Minting a LSP7Token to a UP (No Delegate) from an EOA 92724 (0 )
Minting a LSP7Token to an EOA from an EOA 59377 (0 )
Transferring an LSP7Token from a UP to another UP (No Delegate) 101577 (0 )
Minting a LSP8Token to a UP (No Delegate) from an EOA 159593 (0 )
Minting a LSP8Token to an EOA from an EOA 126247 (0 )
Transferring an LSP8Token from a UP to another UP (No Delegate) 150271 (0 )
UniversalProfile owned by a 🔒📄 LSP6KeyManager

🔀 execute scenarios

execute scenarios 👑 main controller 🛃 restricted controller
LYX transfer --> to an EOA 64337 (0 ) 75288 (0 )
LYX transfer --> to a UP 78488 (0 ) 93385 (0 )
LSP7 token transfer --> to an EOA 116373 (0 ) 131123 (0 )
LSP7 token transfer --> to a UP 249647 (0 ) 264397 (0 )
LSP8 NFT transfer --> to an EOA 180460 (0 ) 195210 (0 )
LSP8 NFT transfer --> to a UP 296936 (0 ) 311686 (0 )

🗄️ setData scenarios

setData scenarios 👑 main controller 🛃 restricted controller
Update Profile details (LSP3Profile Metadata) 67338 (0 ) 77360 (0 )
Add a new controller with permission to SET_DATA + 3x allowed data keys:
AddressPermissions[]
+ AddressPermissions[index]
+ AddressPermissions:Permissions:<controller>
+ AddressPermissions:AllowedERC725YDataKeys:<controller)
209614 (0 ) 219771 (0 )
Update permissions of previous controller. Allow it now to SUPER_SETDATA 52366 (0 ) 55372 (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:
78844 (0 ) 90142 (0 )
Write 5x new LSP12 Issued Assets 67033 (0 ) 101660 (0 )
Update 3x data keys (first 3) 125479 (0 ) 159583 (0 )
Update 3x data keys (middle 3) 105567 (0 ) 143737 (0 )
Update 3x data keys (last 3) 125479 (0 ) 169070 (0 )
Set 2 x new data keys + add 3x new controllers 810459 (0 ) 872259 (0 )

Copy link
Member

@JeneaVranceanu JeneaVranceanu left a comment

Choose a reason for hiding this comment

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

More docs! Good! 👍🏻

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

CJ42 commented Nov 6, 2023

Thanks @JeneaVranceanu
Comments resolved ✅

@JeneaVranceanu
Copy link
Member

JeneaVranceanu commented Nov 6, 2023

@CJ42 Nice!
I'm in the process of going through other PRs as well.
Maybe I will have more input on what's open.

@CJ42 CJ42 merged commit c1574a8 into develop Nov 6, 2023
25 checks passed
@CJ42 CJ42 deleted the docs/isValidSignature branch November 6, 2023 09:38
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