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

feat!: set LSP4TokenType on deployment of LSP7/8 Digital Assets #806

Merged
merged 13 commits into from
Nov 29, 2023

Conversation

CJ42
Copy link
Member

@CJ42 CJ42 commented Nov 23, 2023

What does this PR introduce?

⚠️ BREAKING CHANGES

  • Add an extra parameter to the constructor / initialize(...) functions of LSP4, LSP7 and LSP8 to set LSP4TokenType on deployment.

  • Add an extra parameter to the constructor / initialize(...) functions of LSP8 to set LSP8TokenIdSchema on deployment.

🚀 Feature

  • Allow to specify if a LSP7 / LSP8 Digital Asset represents a Token (0), a NFT (1) or a Collection (2).
  • Allow to specify the schema of the tokenIds of an LSP8 Digital Asset contract.

These schemas can be one of the following:

  NUMBER: 0,
  STRING: 1,
  ADDRESS: 2,
  UNIQUE_ID: 3,
  HASH: 4,
  MIXED_DEFAULT_NUMBER: 100,
  MIXED_DEFAULT_STRING: 101,
  MIXED_DEFAULT_ADDRESS: 102,
  MIXED_DEFAULT_UNIQUE_ID: 103,
  MIXED_DEFAULT_HASH: 104,

♻️ Refactor

See the breaking change description above about LSP4TokenType and LSP8TokenIdSchema set on deployment + make these data keys not editable after the contract has been deployed + initialized.

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

@CJ42
Copy link
Member Author

CJ42 commented Nov 23, 2023

Putting this back in draft, as some things are missing:

  • change the parameter order in LSP8 to have lsp4TokenType_ as 4th param to follow logical order of inheritance ✅ instead of after the tokenIdType_
  • change parameter name of LSP8 to lsp8TokenIdType_ to make the distinction clearer.

As follow:

    constructor(
        string memory name_,
        string memory symbol_,
        address newOwner_,
        uint256 lsp4TokenType_
        uint256 lsp8TokenIdType_
    ) LSP4DigitalAssetMetadata(name_, symbol_, newOwner_, lsp4TokenType_) {

@CJ42 CJ42 marked this pull request as draft November 23, 2023 17:38
@CJ42 CJ42 marked this pull request as ready for review November 24, 2023 10:47
@CJ42 CJ42 force-pushed the feat/lsp4-token-type branch 2 times, most recently from 1ff1de8 to 1085305 Compare November 24, 2023 16:45
Copy link
Contributor

github-actions bot commented Nov 24, 2023

Changes to gas cost

Generated at commit: c4e584d531e9040e2336eb2754407c2525bd75ea, compared to commit: 6594f678c362bf26882023aa1a108bfd8188924d

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
LSP6ExecuteUnrestrictedController transferTokensToRandomEOA
transferTokensToRandomUP
+327 ❌
+851 ❌
+0.44%
+0.42%
LSP6ExecuteRestrictedController transferTokensToRandomEOA +327 ❌ +0.44%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
LSP6ExecuteUnrestrictedController 3,047,114 (0) transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
62,566 (-19)
56,893 (-61)
142,107 (+353)
248,083 (+707)
74,080 (+327)
204,977 (+851)
-0.03%
-0.11%
+0.25%
+0.29%
+0.44%
+0.42%
62,566 (-19)
56,893 (-61)
142,107 (+353)
248,083 (+707)
74,080 (+327)
204,977 (+851)
-0.03%
-0.11%
+0.25%
+0.29%
+0.44%
+0.42%
62,566 (-19)
56,893 (-61)
142,107 (+353)
248,083 (+707)
74,080 (+327)
204,977 (+851)
-0.03%
-0.11%
+0.25%
+0.29%
+0.44%
+0.42%
62,566 (-19)
56,893 (-61)
142,107 (+353)
248,083 (+707)
74,080 (+327)
204,977 (+851)
-0.03%
-0.11%
+0.25%
+0.29%
+0.44%
+0.42%
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6ExecuteRestrictedController 3,047,114 (0) transferLYXToEOA
transferLYXToUP
transferNFTToRandomEOA
transferNFTToRandomUP
transferTokensToRandomEOA
transferTokensToRandomUP
62,179 (-19)
55,293 (-61)
143,363 (+353)
249,339 (+707)
75,336 (+327)
206,547 (+851)
-0.03%
-0.11%
+0.25%
+0.28%
+0.44%
+0.41%
62,179 (-19)
55,293 (-61)
143,363 (+353)
249,339 (+707)
75,336 (+327)
206,547 (+851)
-0.03%
-0.11%
+0.25%
+0.28%
+0.44%
+0.41%
62,179 (-19)
55,293 (-61)
143,363 (+353)
249,339 (+707)
75,336 (+327)
206,547 (+851)
-0.03%
-0.11%
+0.25%
+0.28%
+0.44%
+0.41%
62,179 (-19)
55,293 (-61)
143,363 (+353)
249,339 (+707)
75,336 (+327)
206,547 (+851)
-0.03%
-0.11%
+0.25%
+0.28%
+0.44%
+0.41%
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
LSP6SetDataRestrictedController 3,035,099 (0) execute 30,309 (-19) -0.06% 34,296 (-10) -0.03% 34,296 (-10) -0.03% 38,284 (0) 0.00% 2 (0)
LSP6SetDataUnrestrictedController 3,035,099 (0) execute 30,309 (-19) -0.06% 34,296 (-10) -0.03% 34,296 (-10) -0.03% 38,284 (0) 0.00% 2 (0)

@CJ42 CJ42 force-pushed the feat/lsp4-token-type branch 2 times, most recently from 28ba988 to 88f57c3 Compare November 24, 2023 17:40
@YamenMerhi YamenMerhi merged commit 9ee6558 into develop Nov 29, 2023
25 checks passed
@YamenMerhi YamenMerhi deleted the feat/lsp4-token-type branch November 29, 2023 14:57
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