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!: remove LSP7 & LSP8 compatible contracts #845

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Conversation

b00ste
Copy link
Member

@b00ste b00ste commented Jan 10, 2024

What does this PR introduce?

⚠️ BREAKING CHANGES

♻️ Refactor

Remove the LSP7 & LSP8 compatible contracts and tests, fix the build and the ci.

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

@Hugoo
Copy link
Contributor

Hugoo commented Jan 10, 2024

Copy link
Contributor

github-actions bot commented Jan 10, 2024

Changes to gas cost

Generated at commit: 0c65e4cc23ea7bcc6cb72f831d0987d279dfde7f, 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)

@CJ42
Copy link
Member

CJ42 commented Jan 12, 2024

For context, it was decided to remove and deprecate the Compatible Token contracts LSP7CompatibleERC20 and LSP8CompatibleERC721 for the following reasons:

  • Compatible token contracts are problematic as they bind 2 specifications (LSP7 + ERC20, LSP8 + ERC721) that can sometimes be opposite to each other.

  • High gas cost for transactions: because of the multiple storage writes (on ERC721 for operators approval for all), notifications hooks (onERC721Received + LSP1 hooks) and events emitted from both standards (e.g: Transfer from LSP7 + ERC20), this creates a very high gas cost for any transfer or operator approval transaction. The highest being safeTransferFrom from LSP8CompatibleERC721 which incurs all of these points defined previously.

  • These contracts go against the vision of LUKSO to encourage LSP7 + LSP8 standards on the LUKSO blockchain, compared to use these old ERC20 + ERC721 standards that carry issues and limitations that we hope being addressed by both LSP7 + LSP8

  • security reasons: it is very easy for a developer to just assume the compatible extensions work exactly like the ERC20 and ERC721 they are familiar with, which could create security problems (eg. not caring about the universal receiver callback).

Therefore, deprecating them feels like a good idea, the compatible extensions feel like forcing LSP7 and LSP8 to do things that they weren't meant to do in the first place.

@CJ42 CJ42 merged commit 78b3cf0 into develop Jan 12, 2024
43 checks passed
@CJ42 CJ42 deleted the DEV-9534 branch January 12, 2024 08:46
@richtera richtera mentioned this pull request Mar 6, 2024
6 tasks
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