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!: set LSP8 TokenId Type on deployment / initialization #712

Merged
merged 9 commits into from
Sep 27, 2023

Conversation

CJ42
Copy link
Member

@CJ42 CJ42 commented Sep 12, 2023

What does this PR introduce?

⚠️ BREAKING CHANGES

  • Add parameter on constructor / initialize to set the LSP8 Token Id type when the contract is deployed (for standard contracts) or initialized (for proxy contracts).

  • Restrict the internal _setData(bytes32,bytes) function to disallow to edit the data key LSP8TokenIdType after the contract has been deployed or initialized.

🚀 Feature

Add Typescript and Solidity constants for the different types of LSP8 Token ID Types

Value Type Description
0 uint256 each NFT is represented with a unique number.This number is an incrementing count, where each minted token is assigned the next number.
1 string each NFT is represented using a unique name (as a short utf8 encoded string, no more than 32 characters long)
2 bytes32 each NFT is represented using a 32 bytes unique identifier.
3 bytes32 each NFT is represented using a 32 bytes hash digest.
4 address each NFT is represented as its own ERC725Y smart contract.

♻️ Refactor

Add extra parameter of uint256 tokenIdType in constructor and initialize functions of LSP8 Core, presets and extensions.

Add custom error named LSP8TokenIdType not editable to disallow setting the data key LSP8TokenIdType.

🧪 Tests

Add tests to ensure data key / value pair for LSP8TokenIdType is set when deploying / initializing a LSP8 contract.

📄 Documentation

Update auto-generated Natspec docs for LSP8 contracts.

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 Sep 12, 2023

@CJ42 CJ42 force-pushed the DEV-3725 branch 5 times, most recently from e7cbffa to 7e349f8 Compare September 19, 2023 11:06
@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2023

Changes to gas cost

Generated at commit: 115b2b8fd00560f7ff1a247bc4e08152396a5617, compared to commit: b8eca3c5696acf85239130ef67edec9e8c134bfa

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
LSP6ExecuteRestrictedController lsp20VerifyCall
transferLYXToEOA
transferLYXToUP
transferTokensToRandomEOA
-214 ✅
-147 ✅
-147 ✅
-157 ✅
-1.26%
-0.26%
-0.47%
-0.21%
LSP6ExecuteUnrestrictedController lsp20VerifyCall
transferLYXToEOA
transferLYXToUP
transferTokensToRandomEOA
-214 ✅
-147 ✅
-147 ✅
-157 ✅
-1.26%
-0.26%
-0.44%
-0.22%
LSP6SetDataRestrictedController execute
givePermissionsToController
restrictControllerToERC725YKeys
-160 ✅
+192 ❌
+192 ❌
-0.56%
+0.16%
+0.14%
LSP6SetDataUnrestrictedController execute
givePermissionsToController
-160 ✅
+192 ❌
-0.56%
+0.15%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
LSP6ExecuteRestrictedController 2,862,894 (-12,797) lsp20VerifyCall
transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
14,619 (-277)
56,018 (-147)
31,349 (-147)
130,724 (-45)
239,224 (-90)
73,535 (-157)
208,947 (-212)
-1.86%
-0.26%
-0.47%
-0.03%
-0.04%
-0.21%
-0.10%
16,818 (-214)
56,018 (-147)
31,349 (-147)
130,724 (-45)
239,224 (-90)
73,535 (-157)
208,947 (-212)
-1.26%
-0.26%
-0.47%
-0.03%
-0.04%
-0.21%
-0.10%
17,551 (-194)
56,018 (-147)
31,349 (-147)
130,724 (-45)
239,224 (-90)
73,535 (-157)
208,947 (-212)
-1.09%
-0.26%
-0.47%
-0.03%
-0.04%
-0.21%
-0.10%
17,551 (-194)
56,018 (-147)
31,349 (-147)
130,724 (-45)
239,224 (-90)
73,535 (-157)
208,947 (-212)
-1.09%
-0.26%
-0.47%
-0.03%
-0.04%
-0.21%
-0.10%
8 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6ExecuteUnrestrictedController 2,862,894 (-12,797) lsp20VerifyCall
transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
14,619 (-277)
56,648 (-147)
33,349 (-147)
129,472 (-46)
237,973 (-90)
71,971 (-157)
207,383 (-212)
-1.86%
-0.26%
-0.44%
-0.04%
-0.04%
-0.22%
-0.10%
16,818 (-214)
56,648 (-147)
33,349 (-147)
129,472 (-46)
237,973 (-90)
71,971 (-157)
207,383 (-212)
-1.26%
-0.26%
-0.44%
-0.04%
-0.04%
-0.22%
-0.10%
17,551 (-194)
56,648 (-147)
33,349 (-147)
129,472 (-46)
237,973 (-90)
71,971 (-157)
207,383 (-212)
-1.09%
-0.26%
-0.44%
-0.04%
-0.04%
-0.22%
-0.10%
17,551 (-194)
56,648 (-147)
33,349 (-147)
129,472 (-46)
237,973 (-90)
71,971 (-157)
207,383 (-212)
-1.09%
-0.26%
-0.44%
-0.04%
-0.04%
-0.22%
-0.10%
8 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6SetDataRestrictedController 2,847,670 (-12,798) execute
givePermissionsToController
restrictControllerToERC725YKeys
18,343 (-147)
120,826 (+192)
139,076 (+192)
-0.80%
+0.16%
+0.14%
28,375 (-160)
120,826 (+192)
139,076 (+192)
-0.56%
+0.16%
+0.14%
28,375 (-160)
120,826 (+192)
139,076 (+192)
-0.56%
+0.16%
+0.14%
38,408 (-173)
120,826 (+192)
139,076 (+192)
-0.45%
+0.16%
+0.14%
2 (0)
1 (0)
1 (0)
LSP6SetDataUnrestrictedController 2,847,670 (-12,798) execute
givePermissionsToController
restrictControllerToERC725YKeys
18,343 (-147)
126,826 (+192)
147,576 (+192)
-0.80%
+0.15%
+0.13%
28,375 (-160)
126,826 (+192)
147,576 (+192)
-0.56%
+0.15%
+0.13%
28,375 (-160)
126,826 (+192)
147,576 (+192)
-0.56%
+0.15%
+0.13%
38,408 (-173)
126,826 (+192)
147,576 (+192)
-0.45%
+0.15%
+0.13%
2 (0)
1 (0)
1 (0)

@CJ42 CJ42 force-pushed the DEV-3725 branch 2 times, most recently from 671539f to 39d8985 Compare September 26, 2023 08:36
@CJ42 CJ42 force-pushed the DEV-3725 branch 2 times, most recently from 8e6c582 to a80e619 Compare September 26, 2023 09:24
Copy link
Member

@b00ste b00ste left a comment

Choose a reason for hiding this comment

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

Everything besides the comments looks good

@CJ42 CJ42 merged commit 67cb333 into develop Sep 27, 2023
26 checks passed
@CJ42 CJ42 deleted the DEV-3725 branch September 27, 2023 12:06
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