feat: erc165 support interface #281

qalisander · 2024-09-11T11:50:01Z

Adds erc165 standard altogether with a proc macro, that lets to compute interface id in a simple way.

Resolves #33

PR Checklist

Tests
Documentation

…face

netlify · 2024-09-11T11:50:17Z

✅ Deploy Preview for contracts-stylus canceled.

Name	Link
🔨 Latest commit	`7d27027`
🔍 Latest deploy log	https://app.netlify.com/sites/contracts-stylus/deploys/66f1a96284edb00008522be9

qalisander · 2024-09-11T11:51:28Z

examples/erc20/src/lib.rs

+        interface_id: FixedBytes<4>,
+    ) -> Result<bool, Vec<u8>> {
+        let interface_id = u32::from_be_bytes(*interface_id);
+        let supported = interface_id == <Erc20 as IErc20>::INTERFACE_ID;


It also should check for IErc165 interface id

I think this can be done relatively straight forward with something like this(not sure if FixedBytes<4> can be converted to [u8; 4] and the return type can be simplified, but I did it for the simplicity of the example):

pub trait ERC165 { const INTERFACE_ID: [u8; 4] = [0x01, 0xff, 0xff, 0xff]; fn supports_erc165_interface(&self, interface_id: [u8; 4]) -> bool { interface_id == Self::INTERFACE_ID } }

And then this contract can do:

impl ERC165 for Erc20Example {} impl Erc20Example { fn supports_interface(&self, interface_id: [u8; 4]) -> bool { interface_id == <Erc20 as IErc20>::INTERFACE_ID || ERC165::supports_erc165_interface(self, interface_id) } }

I'm sure there are better ways to achieve the same, but something like this should work

codecov · 2024-09-11T11:53:02Z

Codecov Report

Attention: Patch coverage is 73.19588% with 26 lines in your changes missing coverage. Please review.

Project coverage is 78.7%. Comparing base (8fb2758) to head (7d27027).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
contracts/src/token/erc20/extensions/metadata.rs	20.0%	4 Missing ⚠️
contracts/src/token/erc20/mod.rs	20.0%	4 Missing ⚠️
...ontracts/src/token/erc721/extensions/enumerable.rs	20.0%	4 Missing ⚠️
contracts/src/token/erc721/extensions/metadata.rs	0.0%	4 Missing ⚠️
contracts/src/token/erc721/mod.rs	20.0%	4 Missing ⚠️
contracts-proc/src/interface_id.rs	95.5%	3 Missing ⚠️
contracts/src/utils/introspection/erc165.rs	0.0%	3 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
contracts-proc/src/lib.rs	`100.0% <100.0%> (ø)`
contracts-proc/src/interface_id.rs	`95.5% <95.5%> (ø)`
contracts/src/utils/introspection/erc165.rs	`0.0% <0.0%> (ø)`
contracts/src/token/erc20/extensions/metadata.rs	`3.7% <20.0%> (+3.7%)`	⬆️
contracts/src/token/erc20/mod.rs	`83.4% <20.0%> (-0.7%)`	⬇️
...ontracts/src/token/erc721/extensions/enumerable.rs	`86.7% <20.0%> (-1.2%)`	⬇️
contracts/src/token/erc721/extensions/metadata.rs	`0.0% <0.0%> (ø)`
contracts/src/token/erc721/mod.rs	`88.2% <20.0%> (-0.3%)`	⬇️

ggonzalez94

This is looking good! I like the approach of making this a procedural macro. I still need to go through the macro implementation, but already left a couple comments

ggonzalez94 · 2024-09-18T02:57:33Z

examples/erc20/src/lib.rs

+        interface_id: FixedBytes<4>,
+    ) -> Result<bool, Vec<u8>> {
+        let interface_id = u32::from_be_bytes(*interface_id);
+        let supported = interface_id == <Erc20 as IErc20>::INTERFACE_ID;


I think this can be done relatively straight forward with something like this(not sure if FixedBytes<4> can be converted to [u8; 4] and the return type can be simplified, but I did it for the simplicity of the example):

pub trait ERC165 { const INTERFACE_ID: [u8; 4] = [0x01, 0xff, 0xff, 0xff]; fn supports_erc165_interface(&self, interface_id: [u8; 4]) -> bool { interface_id == Self::INTERFACE_ID } }

And then this contract can do:

impl ERC165 for Erc20Example {} impl Erc20Example { fn supports_interface(&self, interface_id: [u8; 4]) -> bool { interface_id == <Erc20 as IErc20>::INTERFACE_ID || ERC165::supports_erc165_interface(self, interface_id) } }

ggonzalez94 · 2024-09-18T03:12:49Z

examples/erc20/src/lib.rs

+        interface_id: FixedBytes<4>,
+    ) -> Result<bool, Vec<u8>> {
+        let interface_id = u32::from_be_bytes(*interface_id);
+        let supported = interface_id == <Erc20 as IErc20>::INTERFACE_ID;


I'm sure there are better ways to achieve the same, but something like this should work

contracts-proc/src/interface.rs

bidzyyys

Great job @qalisander!
I am not a fan of introducing contracts-proc lib only for a single macro.
I am thinking if is there any way to achieve the same only with default trait impls like:

pub trait IInterface {
    const INTERFACE_ID: FixedBytes<4>;
}
pub trait IErc165: IInterface { 
    const ERC165_INTERFACE_ID: FixedBytes<4> = fixed_bytes!("01ffffff"); 
    
    fn supports_interface(&self, interface_id: FixedBytes<4>) -> bool { 
        interface_id == Self::ERC165_INTERFACE_ID || interface_id == Self::INTERFACE_ID
    } 
}

Could you try this out? Or if you see any blockers let me know before spending time on it.

contracts-proc/src/interface.rs

contracts-proc/src/interface_id.rs

qalisander · 2024-09-19T20:15:13Z

contracts-proc/src/interface_id.rs

+                func.attrs.push(attr);
+                continue;
+            };
+            if *ident == "selector" {


Here we retrieve all attributes of type: #[selector(name = "actualSelectorName") in case actual selector's name should be different from rust function's name. Erc721 is best example of it.

…face

qalisander · 2024-09-20T13:47:18Z

One additional advantage of having procedural macro for interface ids, instead of hard-coding them is: Whenever we will accidentally change/refactor contract's trait or stylus-sdk will introduce update that will slightly change selector or interface id computation. It will be caught by our unit and integration tests.

bidzyyys

Looks good overall, left comments that bothers me.

contracts-proc/Cargo.toml

contracts/Cargo.toml

bidzyyys · 2024-09-20T23:39:00Z

contracts-proc/Cargo.toml

@@ -0,0 +1,22 @@
+[package]
+name = "openzeppelin-stylus-proc"
+description = "Procedural macros for OpenZeppelin Stylus contracts"


Suggested change

description = "Procedural macros for OpenZeppelin Stylus contracts"

description = "Procedural macros for OpenZeppelin Stylus smart contracts library."

Do you think we should also change description of contracts from OpenZeppelin Contracts for Stylus to OpenZeppelin smart contracts library for Stylus? Just seeking for consistency among projects.
cc @ggonzalez94

I like the original one slightly more, since it is more concise. But I don't have a strong opinion tbh. Agreed with your comment @qalisander that we should keep both consistent.

examples/erc20/src/lib.rs

bidzyyys · 2024-09-21T00:44:51Z

examples/erc721/src/lib.rs

+
+    pub fn supports_interface(interface_id: FixedBytes<4>) -> bool {
+        Erc721::supports_interface(interface_id)
+            || Enumerable::supports_interface(interface_id)


Why Enumerable?

Basically it is implemented for Erc721Enumerable in solidity

bidzyyys · 2024-09-21T00:47:36Z

contracts/src/token/erc721/extensions/metadata.rs

+mod tests {
+    // use crate::token::erc721::extensions::{Erc721Metadata, IErc721Metadata};
+
+    // TODO: IErc721Metadata should be refactored to have same api as solidity


You implemented metadata extension different from solidity. It has base_uri function instead of token_uri. And now it exposes baseUri() function into abi that is not like that in solidity. If you have ideas how to refactor it right now we can do. Otherwise make sense to do it later and create an issue for that
cc @ggonzalez94

bidzyyys · 2024-09-21T00:47:49Z

contracts/src/token/erc721/mod.rs

@@ -549,6 +555,13 @@ impl IErc721 for Erc721 {
    }
 }

+impl IErc165 for Erc721 {


I am not sure if we want to have it implemented always.

It is implemented in solidity for Erc721

We will have to decide on an approach for all the standards in our library, so we can use this thread to do so(I've also opened an internal chat to get our Solidity team opinion). I'm leaning more on not adding it by default and letting the user opt in when they want.
That being said when the standard defines that ERC165 should be part of the contract we should do it. That's the case with ERC721

@qalisander after discussing internally and doing some research it seems ERC165 is not as widely used that is worth adding it to all our contract. What I suggest we do:

Add it automatically to only the standards that require it as part of the ERC(e.g ERC721)

Add a section in our docs that explains how to add it to your own contract. This could be under the API reference for ERC165 or similar.

What do you think?

I think that would look a bit inconsistent. I even wondering do we need ERC165 that much just for ERC721 .. Also I suspect, it is not widely used mostly, because it is not implemented for the contracts like erc20.

Consider that rust is different from solidity. And with current implementation just to make support_interface function visible, library user should import openzeppelin_stylus::utils::introspection::erc165::IErc165 module in the scope (otherwise it won't be even compiled). And then reexport it within #[public] macro. So we are not "automatically" implementing the standard as I see, but making it simple to implement for users. At least what do you mean by "automatically" practically?

That's a great point. I need to add a guide about erc165 to antora docs. But seems for me the ERC165 guide (for erc20) would be more simple if we would implement IErc165 trait for ERC20 also.

And one more point. There is an "orphan rule" in rust that disallows to user implement foreign trait (like IErc165 in our case) for a foreign contract type (like Erc20). In this case the IErc165 trait seems even less practical for users if they would try to implement it for Erc20. May be it would even make sense to remove it.

I think that would look a bit inconsistent.

I don't think it is inconsistent if we explicitly say: We will only add it to standards that require it and give the user the option to add it to any contract(s) they think is worth it.

I even wondering do we need ERC165 that much just for ERC721

It wouldn't be for ERC721 only, since the implementation would be available for anyone wanting to add this to contracts they would like to specify which interfaces they support.

And then reexport it within #[public] macro. So we are not "automatically" implementing the standard as I see, but making it simple to implement for users.

What do you mean here? We are adding it to the #[public] impl block of contracts. That means it would automatically be part of the inherited contract api right?

What do you mean here? We are adding it to the #[public] impl block of contracts. That means it would automatically be part of the inherited contract api right?

We are doing so inside examples. User also can opt out to export it to the abi, even if trait IErc165 is implemented in our library. That is the difference from solidity, where as I see Erc721 will always have supportInterface(...) abi public

Ok... after discussing this today I agree it makes sense to add it to implementations of main interfaces and it doesn't come at any cost to users that don't want it(since it is not part of the public abi and they still need to explicitely opt in). Only potential downside is a bit of gas overhead when inheriting from multiple contracts.

contracts/src/utils/introspection/erc165.rs

bidzyyys · 2024-09-21T00:49:25Z

contracts/src/token/erc20/mod.rs

@@ -286,6 +290,13 @@ impl IErc20 for Erc20 {
    }
 }

+impl IErc165 for Erc20 {


I am not sure if we want to have it implemented always.

Actually OZ Erc20 doesn't have Erc165 support, compare to Erc721. I think the original issue is here was closed due to "complexity". Some other implementations of Erc20 standard (not from open zeppelin) have it.
Also seeking around github by actual Erc20 interface id 0x36372b07, I could find projects that rely on the Erc20 with interface support.
In fact at this pr we are not forcing users to expose supportsInterface(bytes4). But they have an option to do so.
@ggonzalez94 what do you think?

#281 (comment)
^^

bidzyyys · 2024-09-21T00:49:38Z

contracts/src/token/erc20/extensions/metadata.rs

@@ -76,3 +81,22 @@ impl IErc20Metadata for Erc20Metadata {
        DEFAULT_DECIMALS
    }
 }
+
+impl IErc165 for Erc20Metadata {


I am not sure if we want to have it implemented always.

ggonzalez94

Great job here! I just did a second pass and is looking good.
I left a few comments/suggestions and tomorrow I'll go through the open questions

ggonzalez94 · 2024-09-21T22:50:06Z

contracts-proc/Cargo.toml

@@ -0,0 +1,22 @@
+[package]
+name = "openzeppelin-stylus-proc"
+description = "Procedural macros for OpenZeppelin Stylus contracts"


I like the original one slightly more, since it is more concise. But I don't have a strong opinion tbh. Agreed with your comment @qalisander that we should keep both consistent.

contracts-proc/src/interface_id.rs

contracts-proc/src/lib.rs

contracts-proc/src/interface_id.rs

ggonzalez94

@qalisander this looks ready to go IMO. The only thing left to do is documentation, but you can decide if you want to include it as part of this PR or create a new one for that

Adds erc165 standard altogether with a proc macro, that lets to compute interface id in a simple way.  Resolves #33 #### PR Checklist - [x] Tests - [x] Documentation --------- Co-authored-by: Daniel Bigos <[email protected]> (cherry picked from commit 9b04143)

qalisander added 3 commits September 11, 2024 15:26

add erc165

9a3200a

Merge remote-tracking branch 'origin/main' into erc165-supports-inter…

5811bf2

…face

++

94ed516

qalisander commented Sep 11, 2024

View reviewed changes

++

252264f

ggonzalez94 reviewed Sep 18, 2024

View reviewed changes

bidzyyys reviewed Sep 18, 2024

View reviewed changes

contracts-proc/src/interface.rs Outdated Show resolved Hide resolved

qalisander added 5 commits September 19, 2024 23:44

refactor

de539d3

++

0b433b7

++

402e05e

++

0615b0f

++

9229b32

qalisander commented Sep 19, 2024

View reviewed changes

contracts-proc/src/interface_id.rs Show resolved Hide resolved

qalisander commented Sep 19, 2024

View reviewed changes

qalisander added 2 commits September 20, 2024 00:22

++

bec9e4c

Merge remote-tracking branch 'origin/main' into erc165-supports-inter…

e385668

…face

qalisander marked this pull request as ready for review September 19, 2024 20:22

qalisander requested review from ggonzalez94 and bidzyyys September 19, 2024 20:23

qalisander added 3 commits September 20, 2024 16:46

++

4b6eea2

++

4e0f887

++

80af3b0

qalisander force-pushed the erc165-supports-interface branch from 5bfba36 to 80af3b0 Compare September 20, 2024 13:05

++

7be160e

bidzyyys added 2 commits September 21, 2024 01:28

Merge branch 'main' into erc165-supports-interface

7e6a88e

ref: cleanup in workspace

c83e84b

bidzyyys reviewed Sep 21, 2024

View reviewed changes

++

96989da

ggonzalez94 reviewed Sep 22, 2024

View reviewed changes

qalisander and others added 3 commits September 22, 2024 14:48

refactor macro

4cf685e

++

eb48b39

Merge branch 'main' into erc165-supports-interface

7d27027

ggonzalez94 approved these changes Sep 26, 2024

View reviewed changes

qalisander merged commit 9b04143 into main Sep 27, 2024
22 checks passed

bidzyyys deleted the erc165-supports-interface branch October 15, 2024 15:33

	description = "Procedural macros for OpenZeppelin Stylus contracts"
	description = "Procedural macros for OpenZeppelin Stylus smart contracts library."

feat: erc165 support interface #281

feat: erc165 support interface #281

Conversation

qalisander commented Sep 11, 2024 • edited Loading

PR Checklist

netlify bot commented Sep 11, 2024 • edited Loading

✅ Deploy Preview for contracts-stylus canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

ggonzalez94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bidzyyys left a comment • edited Loading

Choose a reason for hiding this comment

qalisander Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

qalisander commented Sep 20, 2024 • edited Loading

bidzyyys 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

qalisander Sep 21, 2024 • edited Loading

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

qalisander Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

qalisander Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

ggonzalez94 Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

qalisander Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qalisander Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggonzalez94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggonzalez94 left a comment

Choose a reason for hiding this comment

qalisander commented Sep 11, 2024 •

edited

Loading

netlify bot commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

bidzyyys left a comment •

edited

Loading

qalisander Sep 19, 2024 •

edited

Loading

qalisander commented Sep 20, 2024 •

edited

Loading

qalisander Sep 21, 2024 •

edited

Loading

qalisander Sep 25, 2024 •

edited

Loading

qalisander Sep 25, 2024 •

edited

Loading

ggonzalez94 Sep 25, 2024 •

edited

Loading

qalisander Sep 25, 2024 •

edited

Loading

qalisander Sep 21, 2024 •

edited

Loading