ERC2981 (Royalty Info) for Cairo #413

immrsd · 2024-11-27T13:13:54Z

Add support for ERC2981 (RoyaltyInfo) to ERC721 and ERC1155
Support uint Cairo types in input fields
Support constants in trait implementations

…aits

ericnordelo

The changes look great @immrsd. We should think of a way to group imports together, but that's out of the scope of this PR.

andrew-fleming

Nice work, @immrsd! Overall, the PR looks good to me. I left a few questions and comments

andrew-fleming · 2024-11-27T22:05:53Z

packages/ui/src/cairo/inject-hyperlinks.ts

-    libraryPathSegments.splice(-1, 1, componentMappings[lastItem]);
+    libraryPathSegments.splice(-1, 1, componentMappings[lastItem] as string);


Are there cases where the the value won't be a string (if it's not undefined)?

Is this cast needed? The inferred type should already be string, and these mappings are defined on line 27 above.

It's needed (at least in the Typescript version I'm using). There's a check componentMappings[lastItem] !== undefined, but since the dictionary is mutable, the compiler can't guarantee that on the next line an item at the lastItem will still exist

Another way could be to use a non-null assertion instead: componentMappings[lastItem]!

andrew-fleming · 2024-11-27T22:40:48Z

packages/ui/src/cairo/ERC1155Controls.svelte

+<RoyaltyInfoSection bind:opts={opts.royaltyInfo} errors={errors} />
+


The window is getting pretty crowded with all the features. Maybe we should consider doing something like having a dropdown menu that opens when the box is checked for votes and royalty? Outside of this PR's scope though

Agree, we should consider that

On the other hand, the Cairo control panels are still more concise that the Solidity ones and if we decide to go with that approach it may make sense to also apply it to the Solidity version

packages/core-cairo/src/utils/convert-strings.ts

packages/core-cairo/src/set-royalty-info.ts

packages/core-cairo/src/utils/convert-strings.ts

ericglau

Thanks @immrsd, this looks great! Just a few comments.

packages/ui/src/cairo/RoyaltyInfoSection.svelte

ericglau · 2024-11-28T20:45:49Z

packages/ui/src/cairo/inject-hyperlinks.ts

-    libraryPathSegments.splice(-1, 1, componentMappings[lastItem]);
+    libraryPathSegments.splice(-1, 1, componentMappings[lastItem] as string);


Is this cast needed? The inferred type should already be string, and these mappings are defined on line 27 above.

packages/core-cairo/src/set-royalty-info.ts

packages/core-cairo/src/contract.ts

ericglau · 2024-11-28T21:09:03Z

packages/core-cairo/src/set-royalty-info.ts

+export const enabledDefaults: RoyaltyInfoOptions = {
+  enabled: true,
+  defaultRoyaltyFraction: '10',
+  feeDenominator: DEFAULT_FEE_DENOMINATOR.toString()
+};


It looks like this is only used in generate/erc{1155,721}.ts, which has generator functions for testing different possible combinations. In this case:

rename or add a comment to make it more clear that these values are just for testing

generate/erc{1155,721}.ts should use an array of the different combinations to test with

For example, define something like the following:

Suggested change

export const enabledDefaults: RoyaltyInfoOptions = {

enabled: true,

defaultRoyaltyFraction: '10',

feeDenominator: DEFAULT_FEE_DENOMINATOR.toString()

};

export const testRoyaltyInfoOptions = [

{

enabled: false,

defaultRoyaltyFraction: '10', // these should be ignored since enabled is false

feeDenominator: DEFAULT_FEE_DENOMINATOR.toString(),

},

{

enabled: true,

defaultRoyaltyFraction: '100',

feeDenominator: DEFAULT_FEE_DENOMINATOR.toString(),

},

{

enabled: true,

defaultRoyaltyFraction: '10000',

feeDenominator: '100000',

];

I agree this is the desired approach, so I've added 3 different options. The only problem is that now instead of 800 generated contracts we have 2000. That shouldn't be a problem, but as a result Scarb compiler required more than 100 GB of RAM at some point during compilation. I've barely managed to compile all the contracts and it took more than 30 minutes

Scarb compiler is not yet well-optimized and this behaviour shouldn't be normal. We should look into it, it seems that some compilation steps that should be O(1) are in fact O(n). Although, I reckon it shouldn't be in the scope of this PR

ericglau · 2024-11-28T21:10:27Z

packages/core-cairo/src/generate/erc1155.ts

  upgradeable: upgradeableOptions,
+  royaltyInfo: [royaltyInfoEnabled],


From my other comment, this should be an array of the different combinations that we want to test using the generator function.

ericglau · 2024-11-28T21:10:41Z

packages/core-cairo/src/generate/erc721.ts

@@ -17,6 +18,7 @@ const blueprint = {
  appVersion: ['v1'],
  pausable: booleans,
  mintable: booleans,
+  royaltyInfo: [royaltyInfoEnabled],


From my other comment, this should be an array of the different combinations that we want to test using the generator function.

packages/core-cairo/src/set-royalty-info.ts

ericglau

It would be good to add some tests in packages/core-cairo/src/erc721.test.ts and packages/core-cairo/src/erc1155.test.ts so that their snapshots are included.

Can you also bump the version in packages/core-cairo/package.json?

The rest LGTM, thanks!

immrsd added 11 commits October 1, 2024 15:22

Add functionality for declaring constants within traits

d4f6afb

Specify return types for printing functions, support constants for tr…

42bca98

…aits

Merge master

6af508f

Support uint Cairo types

5df51a2

Add support for ERC2981 extension

5e3cf95

Add RoyaltyInfo for ERC1155

168c862

Add RoyaltyInfo for ERC721

3d72bd9

Introduce RoyaltyInfoSection with UI controls

6155fea

Enable errors proxying to ERC1155 controls

6b37bd9

Add RoyaltyInfo section to ERC1155 controls

0f52a2c

Add RoyaltyInfo section to ERC721 controls

6fee79c

immrsd requested review from ericnordelo, ericglau and andrew-fleming November 27, 2024 13:13

immrsd self-assigned this Nov 27, 2024

immrsd added 5 commits November 27, 2024 14:17

Update changelog

b02d19d

Fix unnecessary brackets for Cairo constants import

9219dfd

Make Access control required for RoyaltyInfo

83d8017

Update snapshot for Cairo custom contract tests

8b4037c

Apply minor fixes

f424d78

ericnordelo approved these changes Nov 27, 2024

View reviewed changes

andrew-fleming reviewed Nov 27, 2024

View reviewed changes

ericglau reviewed Nov 28, 2024

View reviewed changes

immrsd added 7 commits November 29, 2024 08:22

Add missing semicolons

310b23f

Rename setRoyaltyInfoIfNeeded to setRoyaltyInfo

d1b80ce

Sanitize ERC20 premint value

d5b9830

Add explicit function return types

30b858d

Introduce DEFAULT_ACCESS_CONTROL constant

81a51f2

Remove number check for negative

81fc2eb

Fix broken link

1609800

Add multiple Royalty info options for generate-based-testing

826ba89

ericglau approved these changes Nov 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ERC2981 (Royalty Info) for Cairo #413

ERC2981 (Royalty Info) for Cairo #413

immrsd commented Nov 27, 2024 •

edited

Loading

ericnordelo left a comment

andrew-fleming left a comment

andrew-fleming Nov 27, 2024

ericglau Nov 28, 2024

immrsd Nov 29, 2024

ericglau Nov 29, 2024

andrew-fleming Nov 27, 2024

immrsd Nov 29, 2024

ericglau left a comment

ericglau Nov 28, 2024

ericglau Nov 28, 2024

immrsd Nov 29, 2024

ericglau Nov 28, 2024

ericglau Nov 28, 2024

ericglau left a comment

		libraryPathSegments.splice(-1, 1, componentMappings[lastItem]);
		libraryPathSegments.splice(-1, 1, componentMappings[lastItem] as string);

		<RoyaltyInfoSection bind:opts={opts.royaltyInfo} errors={errors} />

-export const enabledDefaults: RoyaltyInfoOptions = {
-  enabled: true,
-  defaultRoyaltyFraction: '10',
-  feeDenominator: DEFAULT_FEE_DENOMINATOR.toString()
-};
+export const testRoyaltyInfoOptions = [
+  {
+    enabled: false,
+    defaultRoyaltyFraction: '10', // these should be ignored since enabled is false
+    feeDenominator: DEFAULT_FEE_DENOMINATOR.toString(),
+  },
+  {
+    enabled: true,
+    defaultRoyaltyFraction: '100',
+    feeDenominator: DEFAULT_FEE_DENOMINATOR.toString(),
+  },
+  {
+    enabled: true,
+    defaultRoyaltyFraction: '10000',
+    feeDenominator: '100000',
+];

		upgradeable: upgradeableOptions,
		royaltyInfo: [royaltyInfoEnabled],

ERC2981 (Royalty Info) for Cairo #413

Are you sure you want to change the base?

ERC2981 (Royalty Info) for Cairo #413

Conversation

immrsd commented Nov 27, 2024 • edited Loading

ericnordelo left a comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericglau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericglau left a comment

Choose a reason for hiding this comment

immrsd commented Nov 27, 2024 •

edited

Loading