feat: add ERC2981 component #1043

ptisserand · 2024-07-09T08:27:28Z

This PR add ERC2981 component.

ERC2981 component implements the ERC2981 standard interface:

pub const IERC2981_ID: felt252 = 0x2d3414e45a8700c29f119a54b9f11dca0e29e06ddcb214018fc37340e165ed6;

#[starknet::interface]
pub trait IERC2981<TState> {
    fn royalty_info(self: @TState, token_id: u256, sale_price: u256) -> (ContractAddress, u256);
}

The following internal functions are implemented:

fn initializer(ref self: TState, default_receiver: ContractAddress, default_royalty_fraction: u256);
fn _fee_denominator(self: @TState) -> u256;
fn _default_royalty(self: @TState) -> (ContractAddress, u256, u256);
fn _set_default_royalty(ref self: TState, receiver: ContractAddress,fee_numerator: u256,); 
fn _set_token_royalty(ref self: TState, token_id: u256, receiver: ContractAddress, fee_numerator: u256); 
fn _token_royalty(self: @TState, token_id: u256) -> (ContractAddress, u256, u256);
fn _reset_token_royalty(ref self: TState, token_id: u256);

PR Checklist

Tests
Documentation
Added entry to CHANGELOG.md
Tried the feature on a public network

ericnordelo

Thanks again for taking the time @ptisserand. Left some comments. The main suggestion is to be as consistent as possible with the implementation we have in Solidity.

ericnordelo · 2024-07-10T11:02:33Z

src/token.cairo

@@ -1,3 +1,4 @@
 pub mod erc1155;
 pub mod erc20;
+pub mod erc2981;


Since this is not a different token but an extension to both ERC721 and ERC1155, it makes sense for it to be placed under a common module instead.

I have moved the implementation to src/token/common/erc2981

ericnordelo · 2024-07-10T11:03:09Z

src/token/erc2981.cairo

+pub use erc2981::ERC2981Component;
+pub use fees::FeesImpl;
+pub use fees::FeesRatio;
+pub use fees::FeesRatioDefault;
+pub use interface::IERC2981Dispatcher;
+pub use interface::IERC2981DispatcherTrait;
+pub use interface::IERC2981SetupDispatcher;
+pub use interface::IERC2981SetupDispatcherTrait;


These imports can be consolidated into three lines.

ericnordelo · 2024-07-10T11:05:23Z

src/token/erc2981/erc2981.cairo

@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: MIT


While the idea is good, we try to be as consistent as possible with modules that we have already implemented in Solidity, and we have an implementation here.

Do you mean that I should removed my FeesRatio structure and use _feeDenominator in storage as in solidity contract ?

The solidity implementation doesn't have _feeDenominator in storage, but as part of the bytecode as a function. I mean to be as consistent as possible, only diverging when the benefit is obvious or there is a feature inconsistency.

Thanks for your explanation, I have updated source code to be more consistent with solidity implementation.

ericnordelo · 2024-07-10T11:07:28Z

src/token/erc2981/interface.cairo

+
+
+#[starknet::interface]
+pub trait IERC2981Setup<TState> {


This should be provided as internal functions, but since this interface is not part of the standard, we shouldn't provide an external implementation.

I have removed this trait and added internal functions matching the ones in Solidity implementation

ptisserand · 2024-07-17T18:31:31Z

branch rebased on main

ptisserand · 2024-07-18T08:21:21Z

Hi @ericnordelo , are you agree that ERC2981 API documentation should be in docs/modules/ROOT/pages/api/erc2981.adoc ?
Is there a tool to generate documentation from cairo source code ?

src/token/common/erc2981/erc2981.cairo

src/tests/token/erc2981/test_erc2981.cairo

immrsd · 2024-07-29T07:30:50Z

src/tests/token/erc2981/test_erc2981.cairo

+
+fn RECEIVER() -> ContractAddress {
+    contract_address_const::<'RECEIVER'>()
+}


The constants module already declares similar constants, you can employ them instead

src/token/common/erc2981/erc2981.cairo

src/tests/token/erc2981/test_erc2981.cairo

ericnordelo

Hey @ptisserand, the PR is looking good and we have left a few comments. We have two big migrations ongoing (as you can see here), and that's why this won't make it into the next release, since those should be merged into it first. With this said, we will prioritize it and get it merged right after that release, and it will be included in the next one as the main priority.

src/tests/mocks/erc2981_mocks.cairo

src/token/common/erc2981/erc2981.cairo

immrsd · 2024-08-04T07:26:05Z

src/token/common/erc2981/erc2981.cairo

+                .default_royalty_info
+                .write(RoyaltyInfo { receiver, royalty_fraction: fee_numerator })
+        }
+


This implementation lacks _deleteDefaultRoyalty function, which is present in the Solidity implementation. Imo, it makes sense to add it for consistency

Could you please confirm if the following behavior is expected after _deleteDefaultRoyalty is called ?:

Default Royalty is 0% and receiver address is address(0)

Yes, it is the expected one

src/token/common/erc2981/interface.cairo

ericnordelo · 2024-08-12T18:03:48Z

Thanks @ptisserand. Closing this in favor of #1091.

ericnordelo requested changes Jul 10, 2024

View reviewed changes

ptisserand marked this pull request as ready for review July 15, 2024 14:20

ptisserand added 6 commits July 17, 2024 11:19

feat: add ERC2981 component

8430cd0

struct events are public

9ce0dd9

move erc2981 to common directory

d448665

erc2981: be more consistent with Solidity implementation

1196dda

erc2981: add test cases for ERC2981Component

944e9f1

erc2981: LegacyMap is obsolete with scarb 2.7.0-rc

b9c8c9c

ptisserand force-pushed the feature/erc2981-royalty branch from 0cb7fc4 to b9c8c9c Compare July 17, 2024 18:30

immrsd requested changes Jul 29, 2024

View reviewed changes

ericnordelo reviewed Jul 29, 2024

View reviewed changes

src/tests/mocks/erc2981_mocks.cairo Outdated Show resolved Hide resolved

ptisserand added 2 commits July 29, 2024 16:46

erc2981: code style & add some edge case tests.

6f57a4b

erc2981: add missing test for interface registration.

4cb6f9f

immrsd requested changes Aug 4, 2024

View reviewed changes

erc2981: code formatting

c1ff61b

ericnordelo closed this Aug 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ERC2981 component #1043

feat: add ERC2981 component #1043

ptisserand commented Jul 9, 2024 •

edited

Loading

ericnordelo left a comment

ericnordelo Jul 10, 2024

ptisserand Jul 15, 2024

ericnordelo Jul 10, 2024

ericnordelo Jul 10, 2024

ptisserand Jul 11, 2024

ericnordelo Jul 12, 2024

ptisserand Jul 15, 2024

ericnordelo Jul 10, 2024

ptisserand Jul 15, 2024

ptisserand commented Jul 17, 2024

ptisserand commented Jul 18, 2024

immrsd Jul 29, 2024

ericnordelo left a comment

immrsd Aug 4, 2024

ptisserand Aug 5, 2024

immrsd Aug 12, 2024

ericnordelo commented Aug 12, 2024

feat: add ERC2981 component #1043

feat: add ERC2981 component #1043

Conversation

ptisserand commented Jul 9, 2024 • edited Loading

PR Checklist

ericnordelo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptisserand commented Jul 17, 2024

ptisserand commented Jul 18, 2024

Choose a reason for hiding this comment

ericnordelo 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

ericnordelo commented Aug 12, 2024

ptisserand commented Jul 9, 2024 •

edited

Loading