refactor!: Optional LSP1 Notification on `revokeOperator(..)` #763

YamenMerhi · 2023-10-26T10:00:11Z

What does this PR introduce?

⚠️ BREAKING CHANGES

♻️ Refactor

Mark increaseAllowance and decreaseAllowance as part of the interface resulting in a change of interfaceId in LSP7
Add boolean parameter to revokeOperator to signal whether to notify or not in LSP7 and LSP8 resulting in interfaceId change.
Mark the notification in increase/decreaseAllowance and authorizeOperator as mandatory that does revert when the receiver revert. These notifications only occurs in case the operator supports LSP1.

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 · 2023-10-26T10:05:21Z

👋 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	2988293 (0 )
KeyManager	3481039 (0 )
LSP1DelegateUP	1587926 (0 )
LSP7Mintable	2339262 (1,699 📈❌)
LSP8Mintable	2439849 (-12,998 📉✅)

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	37537 (0 )
Transfer 1 LYX to a UP without data	36639 (0 )
Transfer 1 LYX to an EOA with 256 bytes of data	42186 (-24 📉✅)
Transfer 1 LYX to a UP with 256 bytes of data	44843 (-24 📉✅)
Transfer 0.1 LYX to 3x EOA without data	70862 (0 )
Transfer 0.1 LYX to 3x UP without data	75680 (0 )
Transfer 0.1 LYX to 3x EOA with 256 bytes of data	84802 (-12 📉✅)
Transfer 0.1 LYX to 3x UPs with 256 bytes of data	100285 (-36 📉✅)

🗄️ `setData` scenarios

`setData` scenarios - UP owned by 🔑 EOA	⛽ Gas Usage
Set a 20 bytes long value	49971 (0 )
Set a 60 bytes long value	95293 (0 )
Set a 160 bytes long value	164453 (-12 📉✅)
Set a 300 bytes long value	279700 (36 📈❌)
Set a 600 bytes long value	484136 (12 📈❌)
Change the value of a data key already set	32859 (0 )
Remove the value of a data key already set	27333 (0 )
Set 2 data keys of 20 bytes long value	78454 (0 )
Set 2 data keys of 100 bytes long value	260618 (0 )
Set 3 data keys of 20 bytes long value	105171 (0 )
Change the value of three data keys already set of 20 bytes long value	45471 (0 )
Remove the value of three data keys already set	41360 (0 )

🗄️ `Tokens` scenarios

`Tokens` scenarios - UP owned by 🔑 EOA	⛽ Gas Usage
Minting a LSP7Token to a UP (No Delegate) from an EOA	92031 (49 📈❌)
Minting a LSP7Token to an EOA from an EOA	59337 (48 📈❌)
Transferring an LSP7Token from a UP to another UP (No Delegate)	100205 (113 📈❌)
Minting a LSP8Token to a UP (No Delegate) from an EOA	158922 (0 )
Minting a LSP8Token to an EOA from an EOA	126229 (0 )
Transferring an LSP8Token from a UP to another UP (No Delegate)	148899 (0 )

UniversalProfile owned by a 🔒📄 LSP6KeyManager

🔀 `execute` scenarios

`execute` scenarios	👑 main controller	🛃 restricted controller
LYX transfer --> to an EOA	66286 (0 )	77079 (0 )
LYX transfer --> to a UP	67568 (0 )	79550 (0 )
LSP7 token transfer --> to an EOA	118198 (134 📈❌)	132889 (134 📈❌)
LSP7 token transfer --> to a UP	252652 (157 📈❌)	267343 (157 📈❌)
LSP8 NFT transfer --> to an EOA	182307 (0 )	196975 (0 )
LSP8 NFT transfer --> to a UP	299985 (0 )	314653 (0 )

🗄️ `setData` scenarios

`setData` scenarios	👑 main controller	🛃 restricted controller
Update Profile details (LSP3Profile Metadata)	68906 (0 )	78905 (0 )
Add a new controller with permission to `SET_DATA` + 3x allowed data keys: `AddressPermissions[]` + `AddressPermissions[index]` + `AddressPermissions:Permissions:<controller>` + `AddressPermissions:AllowedERC725YDataKeys:<controller`)	212305 (0 )	222370 (0 )
Update permissions of previous controller. Allow it now to `SUPER_SETDATA`	53793 (0 )	56776 (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:	80716 (0 )	91940 (0 )
Write 5x new LSP12 Issued Assets	68459 (0 )	103063 (0 )
Update 3x data keys (first 3)	127747 (0 )	161828 (0 )
Update 3x data keys (middle 3)	107835 (0 )	145982 (0 )
Update 3x data keys (last 3)	127747 (0 )	171315 (0 )
Set 2 x new data keys + add 3x new controllers	816194 (0 )	877741 (0 )

github-actions · 2023-10-26T10:05:36Z

Changes to gas cost

Generated at commit: 38f4c3d5b3ab8535bb34334286e4d0cae3cef7f1, compared to commit: e2ec65aeba2aca8351d9a9e71e08e34a004ae483

🧾 Summary (10% most significant diffs)

Contract	Method	Avg (+/-)	%
LSP6ExecuteRestrictedController	transferTokensToRandomEOA	+36 ❌	+0.05%
LSP6ExecuteUnrestrictedController	transferTokensToRandomEOA	+35 ❌	+0.05%

Full diff report 👇

Contract	Deployment Cost (+/-)	Method	Min (+/-)	%	Avg (+/-)	%	Median (+/-)	%	Max (+/-)	%	# Calls (+/-)
LSP6ExecuteRestrictedController	2,905,943 (0)	transferTokensToRandomEOA transferTokensToRandomUP	74,680 (+36) 206,915 (+66)	+0.05% +0.03%	74,680 (+36) 206,915 (+66)	+0.05% +0.03%	74,680 (+36) 206,915 (+66)	+0.05% +0.03%	74,680 (+36) 206,915 (+66)	+0.05% +0.03%	1 (0) 1 (0)
LSP6ExecuteUnrestrictedController	2,905,943 (0)	transferTokensToRandomEOA transferTokensToRandomUP	73,428 (+35) 205,351 (+66)	+0.05% +0.03%	73,428 (+35) 205,351 (+66)	+0.05% +0.03%	73,428 (+35) 205,351 (+66)	+0.05% +0.03%	73,428 (+35) 205,351 (+66)	+0.05% +0.03%	1 (0) 1 (0)

…#763)" This reverts commit 63c1a0f.

skimaharvey

LGTM

CJ42 · 2023-10-27T12:41:20Z

contracts/LSP7DigitalAsset/ILSP7DigitalAsset.sol

     * @param operatorNotificationData The data to notify the operator about via LSP1.
     */
    event RevokedOperator(
        address indexed operator,
        address indexed tokenOwner,
+        bool notified,


should we index this?

CJ42 · 2023-10-27T12:42:35Z

contracts/LSP7DigitalAsset/ILSP7DigitalAsset.sol

@@ -123,6 +126,7 @@ interface ILSP7DigitalAsset is IERC165, IERC725Y {
     * on behalf of the token owner (the caller of the function `msg.sender`). See also {authorizedAmountFor}.
     *
     * @param operator The address to revoke as an operator.
+     * @param notify Boolean indicating whether to notify the operator or not


Missing the period at the end.

Suggested change

* @param notify Boolean indicating whether to notify the operator or not

* @param notify Boolean indicating whether to notify the operator or not.

CJ42 · 2023-10-27T13:39:36Z

contracts/LSP7DigitalAsset/LSP7DigitalAssetCore.sol

-            } catch {
-                return;
-            }
+            ILSP1UniversalReceiver(operator).universalReceiver(


It looks like the two functions _notifyTokenOperator and _notifyTokenSender are redundant, as they contain the same logic. The only part that changes is the typeId passed inside it.

lsp-smart-contracts/contracts/LSP7DigitalAsset/LSP7DigitalAssetCore.sol

Lines 624 to 639 in 63c1a0f

function _notifyTokenSender(

address from,

bytes memory lsp1Data

) internal virtual {

if (

ERC165Checker.supportsERC165InterfaceUnchecked(

from,

_INTERFACEID_LSP1

)

) {

ILSP1UniversalReceiver(from).universalReceiver(

_TYPEID_LSP7_TOKENSSENDER,

lsp1Data

);

}

}

lsp-smart-contracts/contracts/LSP7DigitalAsset/LSP7DigitalAssetCore.sol

Lines 599 to 614 in 63c1a0f

function _notifyTokenOperator(

address operator,

bytes memory lsp1Data

) internal virtual {

if (

ERC165Checker.supportsERC165InterfaceUnchecked(

operator,

_INTERFACEID_LSP1

)

) {

ILSP1UniversalReceiver(operator).universalReceiver(

_TYPEID_LSP7_TOKENOPERATOR,

lsp1Data

);

}

}

We could use the library function tryNotifyUniversalReceiver maybe? This will improve code re-use and consistency across the codebase.

lsp-smart-contracts/contracts/LSP1UniversalReceiver/LSP1Utils.sol

Lines 36 to 49 in 63c1a0f

function tryNotifyUniversalReceiver(

address lsp1Implementation,

bytes32 typeId,

bytes memory data

) internal {

if (

ERC165Checker.supportsERC165InterfaceUnchecked(

lsp1Implementation,

_INTERFACEID_LSP1

)

) {

ILSP1(lsp1Implementation).universalReceiver(typeId, data);

}

}

As we use this library function to notify for transfer ownership and renounce ownership on LSP0, LSP14 and LSP9.

See here: https://github.com/search?q=repo%3Alukso-network%2Flsp-smart-contracts%20tryNotifyUniversalReceiver&type=code

CJ42 · 2023-10-27T13:41:46Z

contracts/LSP8IdentifiableDigitalAsset/ILSP8IdentifiableDigitalAsset.sol

     * @param operatorNotificationData The data to notify the operator about via LSP1.
     */
    event RevokedOperator(
        address indexed operator,
        address indexed tokenOwner,
        bytes32 indexed tokenId,
+        bool notified,


same question, should we index this?

CJ42 · 2023-10-27T13:42:33Z

contracts/LSP8IdentifiableDigitalAsset/LSP8IdentifiableDigitalAssetCore.sol

+            bytes memory lsp1Data = abi.encode(
+                msg.sender,
+                tokenId,
+                false, // unauthorized


What does the comment // unauthorized mean here? I am not sure to understand.

…#763)" (#765) This reverts commit 63c1a0f.

YamenMerhi added 4 commits October 26, 2023 12:45

refactor!: make notification optional in revokeOperator

6d09f32

test: adjust the tests accordingly

524c6d1

docs: generate docs

23aa0f9

chore: add override keyword

88e2b03

YamenMerhi added 6 commits October 26, 2023 14:20

chore: fix lsp7compatibleERC20

ed1f032

refactor: include authorization status bool in lsp1 data in LSP8

0e57ed6

refactor!: allow increase allowance only when allowance > 0

e187479

test: adjust the tests accoridngly

76202f9

docs: generate docs

68df393

docs: generate docs

206d588

b00ste approved these changes Oct 27, 2023

View reviewed changes

CJ42 merged commit 63c1a0f into develop Oct 27, 2023
26 checks passed

CJ42 deleted the authorizeOperatorNotification branch October 27, 2023 13:24

CJ42 added a commit that referenced this pull request Oct 27, 2023

Revert "refactor!: Optional LSP1 Notification on revokeOperator(..) (…

d09a33c

…#763)" This reverts commit 63c1a0f.

CJ42 mentioned this pull request Oct 27, 2023

Revert "refactor!: Optional LSP1 Notification on revokeOperator(..)" #765

Merged

CJ42 restored the authorizeOperatorNotification branch October 27, 2023 13:26

skimaharvey approved these changes Oct 27, 2023

View reviewed changes

CJ42 reviewed Oct 27, 2023

View reviewed changes

CJ42 added a commit that referenced this pull request Oct 27, 2023

Revert "refactor!: Optional LSP1 Notification on revokeOperator(..) (…

b2bc093

…#763)" (#765) This reverts commit 63c1a0f.

github-actions bot mentioned this pull request Oct 30, 2023

chore(main): release 0.12.0 #772

Merged

This was referenced Nov 30, 2023

chore(main): release lsp-smart-contracts 0.13.0 #816

Merged

chore(main): release lsp-smart-contracts 0.13.0 #819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: Optional LSP1 Notification on `revokeOperator(..)` #763

refactor!: Optional LSP1 Notification on `revokeOperator(..)` #763

YamenMerhi commented Oct 26, 2023

github-actions bot commented Oct 26, 2023

🔀 `execute` scenarios

🗄️ `setData` scenarios

🗄️ `Tokens` scenarios

🔀 `execute` scenarios

🗄️ `setData` scenarios

github-actions bot commented Oct 26, 2023 •

edited

Loading

skimaharvey left a comment

CJ42 Oct 27, 2023

CJ42 Oct 27, 2023

CJ42 Oct 27, 2023

CJ42 Oct 27, 2023

CJ42 Oct 27, 2023

	* @param notify Boolean indicating whether to notify the operator or not
	* @param notify Boolean indicating whether to notify the operator or not.

	function _notifyTokenSender(
	address from,
	bytes memory lsp1Data
	) internal virtual {
	if (
	ERC165Checker.supportsERC165InterfaceUnchecked(
	from,
	_INTERFACEID_LSP1
	)
	) {
	ILSP1UniversalReceiver(from).universalReceiver(
	_TYPEID_LSP7_TOKENSSENDER,
	lsp1Data
	);
	}
	}

	function _notifyTokenOperator(
	address operator,
	bytes memory lsp1Data
	) internal virtual {
	if (
	ERC165Checker.supportsERC165InterfaceUnchecked(
	operator,
	_INTERFACEID_LSP1
	)
	) {
	ILSP1UniversalReceiver(operator).universalReceiver(
	_TYPEID_LSP7_TOKENOPERATOR,
	lsp1Data
	);
	}
	}

	function tryNotifyUniversalReceiver(
	address lsp1Implementation,
	bytes32 typeId,
	bytes memory data
	) internal {
	if (
	ERC165Checker.supportsERC165InterfaceUnchecked(
	lsp1Implementation,
	_INTERFACEID_LSP1
	)
	) {
	ILSP1(lsp1Implementation).universalReceiver(typeId, data);
	}
	}

refactor!: Optional LSP1 Notification on revokeOperator(..) #763

refactor!: Optional LSP1 Notification on revokeOperator(..) #763

Conversation

YamenMerhi commented Oct 26, 2023

What does this PR introduce?

⚠️ BREAKING CHANGES

♻️ Refactor

PR Checklist

github-actions bot commented Oct 26, 2023

⛽📊 Gas Benchmark Report

Deployment Costs

Runtime Costs

🔀 execute scenarios

🗄️ setData scenarios

🗄️ Tokens scenarios

🔀 execute scenarios

🗄️ setData scenarios

github-actions bot commented Oct 26, 2023 • edited Loading

Changes to gas cost

🧾 Summary (10% most significant diffs)

skimaharvey left a comment

Choose a reason for hiding this comment

CJ42 Oct 27, 2023

Choose a reason for hiding this comment

CJ42 Oct 27, 2023

Choose a reason for hiding this comment

CJ42 Oct 27, 2023

Choose a reason for hiding this comment

CJ42 Oct 27, 2023

Choose a reason for hiding this comment

CJ42 Oct 27, 2023

Choose a reason for hiding this comment

refactor!: Optional LSP1 Notification on `revokeOperator(..)` #763

refactor!: Optional LSP1 Notification on `revokeOperator(..)` #763

🔀 `execute` scenarios

🗄️ `setData` scenarios

🗄️ `Tokens` scenarios

🔀 `execute` scenarios

🗄️ `setData` scenarios

github-actions bot commented Oct 26, 2023 •

edited

Loading