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.

Sign up for GitHub

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

Jump to bottom

[Feature]: ERC2981 contract #508

Open

18aaddy wants to merge 7 commits into OpenZeppelin:main from 18aaddy:feat/ERC721RoyaltyExtension

+517 −0

18aaddy commented Jan 23, 2025 •

edited by 0xNeshi

Loading

Towards #358

PR Checklist

Tests
Documentation
Changelog

netlify bot commented Jan 23, 2025 •

edited

Loading

✅ Deploy Preview for contracts-stylus canceled.

Name	Link
🔨 Latest commit	`8a2592a`
🔍 Latest deploy log	https://app.netlify.com/sites/contracts-stylus/deploys/67b6efc8ce794100086e11cf

Author

18aaddy commented Jan 23, 2025

@0xNeshi In the OpenZeppelin Solidity Contracts codebase, ERC-2981 functions were implemented separately under Contracts/Token/Common/ERC2981.sol.
I have implemented the ERC-2981 functions in the ERC Royalty Extension file only as there was no separate file for ERC-2981. Is this fine?

Collaborator

0xNeshi commented Jan 23, 2025

@0xNeshi In the OpenZeppelin Solidity Contracts codebase, ERC-2981 functions were implemented separately under Contracts/Token/Common/ERC2981.sol. I have implemented the ERC-2981 functions in the ERC Royalty Extension file only as there was no separate file for ERC-2981. Is this fine?

Good catch! You can continue working the way you started, I think it will be easy to move the contract file if necessary.
Will get back to you on this

bidzyyys reviewed

View reviewed changes

contracts/src/token/erc721/extensions/royalty.rs Outdated Show resolved Hide resolved

contracts/src/token/erc721/extensions/royalty.rs Outdated Show resolved Hide resolved

18aaddy marked this pull request as ready for review

February 2, 2025 11:35

18aaddy requested review from qalisander and 0xNeshi as code owners

February 2, 2025 11:35

codecov bot commented Feb 3, 2025 •

edited

Loading

Codecov Report

Attention: Patch coverage is 98.77049% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.3%. Comparing base (3792c84) to head (8a2592a).

Files with missing lines	Patch %	Lines
contracts/src/token/common/erc2981.rs	98.7%	3 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
contracts/src/token/common/erc2981.rs	`98.7% <98.7%> (ø)`

0xNeshi reviewed

View reviewed changes

Collaborator

0xNeshi left a comment

Many missing changes/tests/docs. See examples of other extension PRs for inspiration on what needs to be added, e.g. Erc1155Burnable

Author

18aaddy commented Feb 8, 2025

Ok I'll check it out and fix it

Author

18aaddy commented Feb 10, 2025

Many missing changes/tests/docs. See examples of other extension PRs for inspiration on what needs to be added, e.g. Erc1155Burnable

@0xNeshi Should I fix this PR or wait?

Good catch! You can continue working the way you started, I think it will be easy to move the contract file if necessary.
Will get back to you on this

Collaborator

0xNeshi commented Feb 14, 2025 •

edited

Loading

Hey @18aaddy , the team talked about this internally and we'd actually want to have Erc2981 as a separate contract.
You should have two PRs - one for Erc2981 and one after that for Erc721Royalty.
You can repurpose this PR for Erc2981 or you can create a new one, whichever you prefer.

Ping us we you need us to clarify anything or if you need any help!

Author

18aaddy commented Feb 14, 2025

Ok I'll work on it

18aaddy added 7 commits

February 20, 2025 14:27


          feat: added ERC721 Royalty Extension contract

95e9243


          update: added documentation

4d99ebf


          Update: Added docs, fixed review changes, added default royalty tests

78363e5


          Update: Added token royalty tests, added error docs


          update: small, final changes

e794ee8


          fix: fixed failing test

35214b4


          erc2981 contract

8a2592a

18aaddy force-pushed the feat/ERC721RoyaltyExtension branch from 5610195 to 8a2592a Compare

February 20, 2025 09:03

18aaddy changed the title ~~[Feature]: ERC721RoyaltyExtension~~ [Feature]: ERC2981 contract

Author

18aaddy commented Feb 20, 2025

@0xNeshi Can you review? I've made this PR for ERC2981 contract and I'll open a new one for the ERC721Royalty extension.

bidzyyys reviewed

View reviewed changes

Collaborator

bidzyyys left a comment

Hey @18aaddy!
Please adjust docs to rest of our contracts, e.g.:

Start each error with * etc.
Make sure each fn argument is described in Rust docs
Get rid of links like {_setTokenRoyalty} and use proper Rust docs links

Also please:

Remove interface_id from IErc2981 trait definition.

0xNeshi reviewed

View reviewed changes

Collaborator

0xNeshi left a comment

Very good work! Below are mostly nits

contracts/src/token/common/erc2981.rs

+              //! royalty payment information.
+              //!
+              //! Royalty information can be specified globally for all token ids via
+              //! {_setDefaultRoyalty}, and/or individually for specific token ids via

Collaborator

0xNeshi Feb 20, 2025

Make sure to link to actual Rust functions (see other contracts' docs for examples)

contracts/src/token/common/erc2981.rs

+              //! IMPORTANT: ERC-2981 only specifies a way to signal royalty information and
+              //! does not enforce its payment.
+              //!
+              //! See `<https://eips.ethereum.org/EIPS/eip-2981#optional-royalty-payments>` in the ERC.

Collaborator

0xNeshi Feb 20, 2025

Suggested change

      
            //! See `<https://eips.ethereum.org/EIPS/eip-2981#optional-royalty-payments>` in the ERC.
          
            //! See <https://eips.ethereum.org/EIPS/eip-2981#optional-royalty-payments> in the ERC.

I don't think you need backticks

contracts/src/token/common/erc2981.rs

Comment on lines +55 to +59

+                      /// Indicates an error for Invalid Token Royalty. Occurs when
+                      /// fee_numerator > denominator
+                      #[derive(Debug)]
+                      #[allow(missing_docs)]
+                      error ERC2981InvalidTokenRoyalty(uint256 tokenId, uint256 numerator, uint256 denominator);

Collaborator

0xNeshi Feb 20, 2025

We have certain conventions that should be followed:

Use the same error message in the sol! macro that we use in the pub enum Error below it.
Error arguments should be documented (e.g. see error docs for Erc1155)
use Rust's snake_case for argument names even in sol! macro.
nit: fee_numerator doesn't exist; numerator does

contracts/src/token/common/erc2981.rs

Comment on lines +88 to +99

+              /// Struct for Royalty Information for tokens.
+              ///
+              /// # Fields
+              ///
+              /// * `receiver` - The receiver address for royalty
+              /// * `royalty_fraction` - Fraction of royalty for receiver
+              #[storage]
+              #[derive(Erase)]
+              pub struct RoyaltyInfo {
+                  receiver: StorageAddress,
+                  royalty_fraction: StorageU96,
+              }

Collaborator

0xNeshi Feb 20, 2025

Remove # Fields section and move each fields docs above the respective fields

contracts/src/token/common/erc2981.rs

+                  royalty_fraction: StorageU96,
+              }
+              /// State of an ERC2981 contract.

Collaborator

0xNeshi Feb 20, 2025

Suggested change

      
            /// State of an ERC2981 contract.
          
            /// State of an [`Erc2981`] contract.

contracts/src/token/common/erc2981.rs

Comment on lines +173 to +174

		/// Function to change the denominator with which to interpret the fee set
		/// in _setTokenRoyalty and _setDefaultRoyalty as a fraction of the sale

Collaborator

0xNeshi Feb 20, 2025

Suggested change

      
                /// Function to change the denominator with which to interpret the fee set
          
                /// in _setTokenRoyalty and _setDefaultRoyalty as a fraction of the sale
          
                /// Changes the denominator with which to interpret the fee set
          
                /// in _setTokenRoyalty and _setDefaultRoyalty as a fraction of the sale

nit: it's obviously a function. Update all relevant comments the same way.
Also this should link to the actual Rust functions

contracts/src/token/common/erc2981.rs

+                      receiver: Address,
+                      fee_numerator: U96,
+                  ) -> Result<(), Error> {
+                      let denominator: U256 = U256::from(self._fee_denominator());

Collaborator

0xNeshi Feb 20, 2025

You can make the conversion when emitting the error, when actually necessary

contracts/src/token/common/erc2981.rs

+                  ///
+                  /// # Arguments
+                  ///
+                  /// * `&mut self` - Write access to the contract's state.

Collaborator

0xNeshi Feb 20, 2025

Missing token_id

contracts/src/token/common/erc2981.rs

Comment on lines +319 to +320

		const BOB: Address = address!("F4EaCDAbEf3c8f1EdE91b6f2A6840bc2E4DD3526");
		const DAVE: Address = address!("0BB78F7e7132d1651B4Fd884B7624394e92156F1");

Collaborator

0xNeshi Feb 20, 2025

No need to hardcode addresses. In the current motsu version you can list addresses/accounts that you need in the test fn signature

contracts/src/token/common/erc2981.rs

		@@ -0,0 +1,514 @@
		//! Implementation of the NFT Royalty Standard, a standardized way to retrieve

Collaborator

0xNeshi Feb 20, 2025

Missing CHANGELOG entry.
Missing *.adoc files, see Solidity version here.
Missing e2e tests - add at least happy flow for Erc2981::royalty_info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet