Add ERC6909 #1040

swan-of-bodom · 2024-07-08T20:47:25Z

Adding the new multi-token standard ERC6909 written in Cairo. This token standard will most likely be used by UniswapV4 so might be interesting to have one for Cairo too as discussed with @andrew-fleming

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

Deploy transaction:
0x004297aadb57195520d58b6de3db91c14bb30284080c78b7f3d65407d2d06e79

Mint transaction:
0x008debb59434cc64f432110c118d8ba64725545764a71529c7226d127d46df5a

Thanks!

Removed much of the logic for these extensions from the `ERC6909Component`. Now these are different components. Reason for this was to keep the original code as small as possible as metadata, supply and uri are not part of the EIP, but are just optional extensions

…into erc6909

andrew-fleming · 2024-07-09T17:46:24Z

Thanks for opening this PR @swan-of-bodom! It may take some time to review this since it's quite big, but we'll get to it as soon as we can

swan-of-bodom · 2024-07-09T20:38:03Z

Thanks for opening this PR @swan-of-bodom! It may take some time to review this since it's quite big, but we'll get to it as soon as we can

No worries, thank you!

andrew-fleming

Good start on this! I still have to review tests, but I think it's worth giving feedback now on design choices, implementations, and styling

andrew-fleming · 2024-07-21T06:57:24Z

src/token/erc6909/dual6909.cairo

We include dual dispatchers to provide a means of supporting camelCase entrypoints from the (inconsistent) Cairo v0 convention. Since ERC6909 is a new standard to this lib and it's not backwards compatible with the existing token standards, we don't need to include the dual dispatcher for it

Removed the dual dispatcher in a27fe02.

Thanks!

andrew-fleming · 2024-07-21T07:12:53Z

src/token/erc6909/erc6909.cairo

+        fn supports_interface(
+            self: @ComponentState<TContractState>, interface_id: felt252
+        ) -> bool {
+            interface_id == interface::IERC6909_ID || interface_id == ISRC5_ID


Shouldn't this component have SRC5 as a dependency? If a contract implemented this and access control, for example, supports_interface would clash when exposing them. SRC5 stores a contract's supported interfaces in storage, so I think the IERC6909 should be set in SRC5's storage during construction (in the initializer)

Definitely, you are right, implementing this with access control would clash. Will set it in storage

andrew-fleming · 2024-07-21T07:14:00Z

src/token/erc6909/erc6909.cairo

+    #[embeddable_as(ERC6909CamelOnlyImpl)]
+    impl ERC6909CamelOnly<
+        TContractState, +HasComponent<TContractState>, +ERC6909HooksTrait<TContractState>
+    > of interface::IERC6909CamelOnly<ComponentState<TContractState>> {


We can remove these. See the comment in the dual dispatchers module

Removed in a27fe02

andrew-fleming · 2024-07-21T07:21:15Z

src/token/erc6909/erc6909.cairo

+    /// @notice The event emitted when a transfer occurs.
+    /// @param caller The caller of the transfer.
+    /// @param sender The address of the sender.
+    /// @param receiver The address of the receiver.
+    /// @param id The id of the token.
+    /// @param amount The amount of the token.


The code is well-documented! Though, we don't follow natspec. We use a rust-like style. Would you mind updating the documentation to be consistent with our style (see our other components)?

I updated the comments to match as close as possible to the other components. 03e577e

andrew-fleming · 2024-07-21T07:31:34Z

src/token/erc6909/erc6909.cairo

+        /// @dev Thrown when owner balance for id is insufficient.
+        pub const INSUFFICIENT_BALANCE: felt252 = 'ERC6909: insufficient balance';
+        /// @dev Thrown when spender allowance for id is insufficient.
+        pub const INSUFFICIENT_ALLOWANCE: felt252 = 'ERC6909: insufficient allowance';
+        /// @dev Thrown when transferring from the zero address
+        pub const TRANSFER_FROM_ZERO: felt252 = 'ERC6909: transfer from 0';
+        /// @dev Thrown when transferring to the zero address
+        pub const TRANSFER_TO_ZERO: felt252 = 'ERC6909: transfer to 0';
+        /// @dev Thrown when minting to the zero address
+        pub const MINT_TO_ZERO: felt252 = 'ERC6909: mint to 0';
+        /// @dev Thrown when burning from the zero address
+        pub const BURN_FROM_ZERO: felt252 = 'ERC6909: burn from 0';
+        /// @dev Thrown when approving from the zero address
+        pub const APPROVE_FROM_ZERO: felt252 = 'ERC6909: approve from 0';
+        /// @dev Thrown when approving to the zero address


Again, I know this is directly from the reference in the EIP, but we can omit these comments. The const name conveys enough IMO. (We should also be using panics instead of thrown in cairoland)

Same as above, updated comments on 03e577e

andrew-fleming · 2024-07-25T22:03:42Z

docs/modules/ROOT/pages/api/erc6909.adoc

+* xref:#ERC6909Component-_set_operator[`++_set_operator(self, owner, spender, approved)++`]
+* xref:#ERC6909Component-_spend_allowance[`++_spend_allowance(self, sender, spender, id, amount)++`]
+* xref:#ERC6909Component-_approve[`++_approve(self, owner, spender, id, amount)++`]
+* xref:#ERC6909Component-_transfer[`++_approve(self, caller, sender, receiver, id, amount)++`]


Suggested change

* xref:#ERC6909Component-_transfer[`++_approve(self, caller, sender, receiver, id, amount)++`]

* xref:#ERC6909Component-_transfer[`++_transfer(self, caller, sender, receiver, id, amount)++`]

andrew-fleming · 2024-07-25T22:18:26Z

docs/modules/ROOT/pages/api/erc6909.adoc

+
+[.contract-item]
+[[ERC6909Component-Approval]]
+==== `[.contract-item-name]#++Approval++#++(owner: ContractAddress, spender: ContractAddress, value: u256)++` [.item-kind]#event#


Suggested change

==== `[.contract-item-name]#++Approval++#++(owner: ContractAddress, spender: ContractAddress, value: u256)++` [.item-kind]#event#

==== `[.contract-item-name]#++Approval++#++(owner: ContractAddress, spender: ContractAddress, id: u256, amount: u256)++` [.item-kind]#event#

andrew-fleming · 2024-07-25T22:23:55Z

docs/modules/ROOT/pages/api/erc6909.adoc

+
+NOTE: Implementing xref:#ERC6909Component[ERC6909Component] is a requirement for this component to be implemented.
+
+This extension allows to set the contract URI (ideally) in the constructor via `initializer(uri: ByteArray)`.


Suggested change

This extension allows to set the contract URI (ideally) in the constructor via `initializer(uri: ByteArray)`.

This extension allows contracts to set the contract URI in the constructor via `initializer`.

And this may include setting the base_uri depending on the design discussion

andrew-fleming · 2024-07-25T22:28:09Z

docs/modules/ROOT/pages/api/erc6909.adoc

+
+NOTE: Implementing xref:#ERC6909Component[ERC6909Component] is a requirement for this component to be implemented.
+
+WARNING: To individual token metadata, this extension requires that the


Not sure what you mean by "To individual token metadata"

andrew-fleming · 2024-07-25T22:36:44Z

docs/modules/ROOT/pages/api/erc6909.adoc

We're missing the ERC6909TokenSupply section and the actual API for ERC6909Metadata

immrsd

Thanks for your efforts! I've left several suggestions. Please note that to approve the implementation we'll have to wait till the EIP-6909 is finalized, since atm it's still in the draft state

immrsd · 2024-08-04T07:35:51Z

src/token/erc6909/erc6909.cairo

+// SPDX-License-Identifier: MIT
+// OpenZeppelin Contracts for Cairo v0.14.0 (token/erc6909/erc6909.cairo)
+
+use core::starknet::{ContractAddress};


Suggested change

use core::starknet::{ContractAddress};

use starknet::ContractAddress;

immrsd · 2024-08-04T07:36:40Z

src/token/erc6909/erc6909.cairo

+    use starknet::ContractAddress;
+    use starknet::get_caller_address;


Suggested change

use starknet::ContractAddress;

use starknet::get_caller_address;

use starknet::{ContractAddress, get_caller_address};

Updated in 56570aa

immrsd · 2024-08-04T07:54:04Z

src/token/erc6909/erc6909.cairo

+
+            let zero_address = Zero::zero();
+
+            if (sender != zero_address) {


Suggested change

if (sender != zero_address) {

if (sender.is_non_zero()) {

Fixed in 40538f6

immrsd · 2024-08-04T07:54:15Z

src/token/erc6909/erc6909.cairo

+                self.ERC6909_balances.write((sender, id), sender_balance - amount);
+            }
+
+            if (receiver != zero_address) {


Suggested change

if (receiver != zero_address) {

if (receiver.is_non_zero()) {

Fixed in 40538f6

immrsd · 2024-08-04T07:56:11Z

src/token/erc6909/erc6909.cairo

+        /// Does not update the allowance value in case of infinite allowance or if spender is operator.
+        fn _spend_allowance(
+            ref self: ComponentState<TContractState>,
+            sender: ContractAddress,


Should this parameter be named owner as in ERC20?

Renamed it to owner in 40538f6 to match ERC20!

immrsd · 2024-08-04T08:00:39Z

src/token/erc6909/erc6909.cairo

+            caller: ContractAddress, // For the `Transfer` event
+            sender: ContractAddress, // from
+            receiver: ContractAddress, // to


+1
I think getting caller by calling get_caller_address in the function makes total sense and can reduce the amount of parameters

src/token/erc6909/extensions/erc6909_content_uri.cairo

immrsd · 2024-08-04T08:03:43Z

src/token/erc6909/extensions/erc6909_metadata.cairo

+            let zero_address = Zero::zero();
+
+            // In case of new ID mints update the token metadata
+            if (sender == zero_address) {


Suggested change

if (sender == zero_address) {

if (sender.is_zero()) {

Fixed in 40538f6

immrsd · 2024-08-04T08:29:29Z

src/token/erc6909/extensions/erc6909_token_supply.cairo

+            let zero_address = Zero::zero();
+
+            // In case of mints we increase the total supply of this token ID
+            if (sender == zero_address) {


Suggested change

if (sender == zero_address) {

if (sender.is_zero()) {

Fixed in 40538f6.

immrsd · 2024-08-04T08:29:40Z

src/token/erc6909/extensions/erc6909_token_supply.cairo

+            }
+
+            // In case of burns we decrease the total supply of this token ID
+            if (receiver == zero_address) {


Suggested change

if (receiver == zero_address) {

if (receiver.is_zero()) {

Fixed in 40538f6 thank you!

swan-of-bodom · 2024-08-13T11:51:37Z

Thanks for your efforts! I've left several suggestions. Please note that to approve the implementation we'll have to wait till the EIP-6909 is finalized, since atm it's still in the draft state

Thanks, happy to contribute to this great library :)

swan-of-bodom added 28 commits June 21, 2024 11:14

erc6909 cairo contracts and tests

f973569

add docs and changelog

7712199

edit /docs/

bd82b63

docs

0a3b145

Merge remote-tracking branch 'upstream/main' into erc6909

b0135f2

update docs

49d4369

update token

79363c7

update token tests

7047486

add token_supply mocoks

3b8b627

Merge branch 'main' of https://github.com/OpenZeppelin/cairo-contracts …

2d7a385

…into erc6909

refactor dual mocks and tests

f5fa462

add token supply tests

641f33f

add token supply and content uri tests

fa621c0

Merge remote-tracking branch 'upstream/main' into erc6909

3c7738a

simplify mock

7802429

update docs

b2fecad

add metadata hook

e30797a

refactor metadata and add comments and docs

5094c66

update documentation

b3a9b62

erc6909 api

d653416

make internalimpl mocks consistent

d265875

update erc6909 docs

ca6c739

resolve docs supports_interface

21a28c6

comments on docs

fbae8e2

remove trailing space

c9ccaa7

lint and typos

3a5e4c0

make imports consistent

6918d90

andrew-fleming reviewed Jul 26, 2024

View reviewed changes

immrsd requested changes Aug 4, 2024

View reviewed changes

swan-of-bodom added 5 commits August 9, 2024 12:51

remove dual dispatchers and update comments

a27fe02

update comments

03e577e

bump to v0.15.0

ba9ff8a

remove tests

543e1a6

add new Map and paths

aa8e8c6

swan-of-bodom added 3 commits August 13, 2024 14:20

use is_non_zero and is_zero and simplify update

40538f6

comments and join imports

56570aa

update comments

54d6f05

	* xref:#ERC6909Component-_transfer[`++_approve(self, caller, sender, receiver, id, amount)++`]
	* xref:#ERC6909Component-_transfer[`++_transfer(self, caller, sender, receiver, id, amount)++`]

	==== `[.contract-item-name]#++Approval++#++(owner: ContractAddress, spender: ContractAddress, value: u256)++` [.item-kind]#event#
	==== `[.contract-item-name]#++Approval++#++(owner: ContractAddress, spender: ContractAddress, id: u256, amount: u256)++` [.item-kind]#event#


		NOTE: Implementing xref:#ERC6909Component[ERC6909Component] is a requirement for this component to be implemented.

		This extension allows to set the contract URI (ideally) in the constructor via `initializer(uri: ByteArray)`.

	This extension allows to set the contract URI (ideally) in the constructor via `initializer(uri: ByteArray)`.
	This extension allows contracts to set the contract URI in the constructor via `initializer`.


		NOTE: Implementing xref:#ERC6909Component[ERC6909Component] is a requirement for this component to be implemented.

		WARNING: To individual token metadata, this extension requires that the

	use core::starknet::{ContractAddress};
	use starknet::ContractAddress;

		use starknet::ContractAddress;
		use starknet::get_caller_address;

	use starknet::ContractAddress;
	use starknet::get_caller_address;
	use starknet::{ContractAddress, get_caller_address};


		let zero_address = Zero::zero();

		if (sender != zero_address) {

	if (receiver != zero_address) {
	if (receiver.is_non_zero()) {

Add ERC6909 #1040

Are you sure you want to change the base?

Add ERC6909 #1040

Conversation

swan-of-bodom commented Jul 8, 2024

andrew-fleming commented Jul 9, 2024

swan-of-bodom commented Jul 9, 2024

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

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

immrsd 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

swan-of-bodom Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swan-of-bodom Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swan-of-bodom Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swan-of-bodom commented Aug 13, 2024

swan-of-bodom Aug 13, 2024 •

edited

Loading

swan-of-bodom Aug 13, 2024 •

edited

Loading

swan-of-bodom Aug 13, 2024 •

edited

Loading