Update erc721 docs #763

andrew-fleming · 2023-10-01T15:20:55Z

Supersedes #699 and fixes #563.

* Change the spelling of "StarkNet" to "Starknet" (OpenZeppelin#557) * fix: link (OpenZeppelin#545) * change StarkNet to Starknet --------- Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com> * add Cargo and Makefile * add deps in cairo_project, create lib * add presets * add base lib * add tests * remove old cairo lib and interface * remove unused import * fix vars * change external funcs to snake case * remove unused import * add tests for externals * add bool assertions * remove preset mods * add IERC20 trait * clean up test * remove assertion * simplify max_u256 * clarify error msg * clean up python project. ready for rust/cairo1 * add erc165 + tests * kickstart account module * re-structure project * re-structure project * re-structure project * update makefile * re-structure project * update to felt252, add use ContractAddress * formatting * remove colons from storage, remove _mint from initializer * add constructor test * add comments * fix format * check error msg in tests * set max_u256 as func * update cairo * update cargo * revert doc commit * remove __init__ * refix link --------- Co-authored-by: kongtaoxing <69096526+kongtaoxing@users.noreply.github.com> Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com> Co-authored-by: Martín Triay <martriay@gmail.com>

* update cairo * fix path * add security mod * add initializable * add tests * Update src/openzeppelin/security/initializable.cairo Co-authored-by: Martín Triay <martriay@gmail.com> * remove underscore * add internal macros --------- Co-authored-by: Martín Triay <martriay@gmail.com>

…ppelin#598) * use zeroable for address * cleanup import

* fix path * add security mod * add pausable * add mock * add tests * add security mod * update cargo * update cairo * Apply suggestions from code review Co-authored-by: Martín Triay <martriay@gmail.com> * remove underscore from _paused * remove unused imports * add test assertion for is_paused * move mocks inside tests crate * remove underscore * add blank line --------- Co-authored-by: Martín Triay <martriay@gmail.com>

This reverts commit e86bcd2.

* Apply suggestions from code review Co-authored-by: Andrew Fleming <fleming-andrew@protonmail.com> Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> * apply review suggestions * remove unused import * add internal macro * add deregister --------- Co-authored-by: Andrew Fleming <fleming-andrew@protonmail.com> Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>

* add utils and constants * simplify role val * add context comments for interface ids

martriay

this needs to be updated to use components, but other than that it looks good!

docs/modules/ROOT/pages/erc721.adoc

martriay · 2023-11-10T07:06:29Z

docs/modules/ROOT/pages/erc721.adoc

+trait IERC721ABI {
+    // IERC721


now that we have components and presets, I wonder if we should begin with this chunk of interface. It's kindof big and with lots of information (non standard, camel support). maybe i'd focus in the component. what do you think?

I agree that it is kind of big with a lot going on. I'm not sure what you're suggesting though. Do you mean removing the Interface section entirely? Or removing the individual interface subsections and just keep the ERC721ABI?

i'm not sure i understood the second option, did you mean IERC721? i was suggesting to remove the interface section, although it may require a bit more of thought. My impression is that it's not that important now we have components because it's not a single module with all of it anymore, now you can select the impls.

If anything, it is closer to the preset, which is no yet documented and probably not the main focus of the section either. The focus should be the component and design/implementation details.

i'm not sure i understood the second option, did you mean IERC721?

Yeah, that section, IERC721Metadata, and ISRC5. I removed those section already since I left that response

i was suggesting to remove the interface section, although it may require a bit more of thought. My impression is that it's not that important now we have components because it's not a single module with all of it anymore, now you can select the impls.

If anything, it is closer to the preset, which is no yet documented and probably not the main focus of the section either. The focus should be the component and design/implementation details.

Agreed^

Maybe it'd make sense to follow a structure like this in the doc:

ERC721Component

Implementations

ERC721Impl

ERC721MetadataImpl
...

ERC721 compatibility

Usage
...

We can either head the impls with the interface snippet or just provide a link to the interface in the API...or just remove the Interface/Implementations section entirely, idk

Right now we have this inconsistent structures:

Upgrades

explainer

usage

Account

interface

Access

usage

interface

ERC20 and ERC721

interface

compatibility

usage

Security

usage

Introspection

interface

usage

I think we should have

explainer

usage

compatibility

and leave interface for API, of have it between explainer and usage. Either way we can merge this and fix it all together later in #829.

docs/modules/ROOT/pages/erc721.adoc

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

ericnordelo

Looking good Andrew! Left a couple of small comments but I think it looks ready.

docs/modules/ROOT/pages/erc721.adoc

ericnordelo · 2023-11-22T12:07:24Z

docs/modules/ROOT/pages/erc721.adoc

+    component!(path: SRC5Component, storage: src5, event: SRC5Event);
+    component!(path: ERC721Component, storage: erc721, event: ERC721Event);


I think we should swap the order (this applies to the impls below as well). This is compliant with the account preset too.

ericnordelo · 2023-11-22T12:09:12Z

docs/modules/ROOT/pages/erc721.adoc

+    component!(path: SRC5Component, storage: src5, event: SRC5Event);
+    component!(path: ERC721ReceiverComponent, storage: erc721_receiver, event: ERC721ReceiverEvent);


Same comment regarding swapping

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

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

ericnordelo

LGTM!

src/token/erc721/interface.cairo

docs/modules/ROOT/pages/erc721.adoc

martriay · 2023-11-24T06:18:19Z

docs/modules/ROOT/pages/erc721.adoc

+trait IERC721ABI {
+    // IERC721


i'm not sure i understood the second option, did you mean IERC721? i was suggesting to remove the interface section, although it may require a bit more of thought. My impression is that it's not that important now we have components because it's not a single module with all of it anymore, now you can select the impls.

If anything, it is closer to the preset, which is no yet documented and probably not the main focus of the section either. The focus should be the component and design/implementation details.

martriay

Looks good! Left minor comments.

I'm having troubles compiling the snippet from the usage section. Most probably an error on my end, but couldn't yet pinpoint it. Can you confirm it works?

I'm using scarb 2.3.1 and I'm using main branch

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

docs/modules/ROOT/pages/erc721.adoc

Co-authored-by: Martín Triay <martriay@gmail.com>

andrew-fleming · 2023-11-25T05:51:46Z

I'm having troubles compiling the snippet from the usage section. Most probably an error on my end, but couldn't yet pinpoint it. Can you confirm it works?

Weird. Yeah, I copied the .toml you posted and I'm also using Scarb 2.3.1 and it compiles on my end. I don't have the syntax highlighting though. Maybe try just pasting the code in MyToken/src/lib.cairo

martriay

It works now, I had to delete my Scarb.lock.

andrew-fleming and others added 30 commits February 18, 2023 11:48

remove unused import

1cb1252

update cairo

f0330cd

add tests for externals

abc737a

add bool assertions

4ca2620

update cairo

8dd8ed3

remove preset mods

091a29c

add IERC20 trait

1c866f2

update cairo

3405063

clean up test

8fe3c82

remove assertion

ad928bc

update cairo

d883eb5

simplify max_u256

543501b

clarify error msg

2ecba91

add erc165 + tests

6c3978b

kickstart account module

60365c8

clean up python project. ready for rust/cairo1

be1e9df

rebase to cairo-1

c20fd02

re-structure project

bce2506

re-structure project

35ede09

re-structure project

e55889a

update makefile

9d5079c

bump submodule

ae89fa5

Use zeroable::zero() instead of contract_address_const::<0>() (OpenZe…

f48da2c

…ppelin#598) * use zeroable for address * cleanup import

add utils and constants

e86bcd2

Revert "add utils and constants"

42a160f

This reverts commit e86bcd2.

Migrate constants (OpenZeppelin#611)

b2624ee

* add utils and constants * simplify role val * add context comments for interface ids

andrew-fleming added 4 commits November 9, 2023 03:19

fix title

a048b8a

Merge branch 'component-erc721' into add-erc721-docs

d4424e4

fix conflicts

73d5693

update usage section, fix code examples

eb9ecf8

martriay reviewed Nov 10, 2023

View reviewed changes

andrew-fleming added 15 commits November 10, 2023 10:38

update receiver section

2d2ca00

fix conflicts

78cd8d6

remove the constructor

03b5d5a

change to component, remove src5

0ddf9fa

clean up metadata section

6397666

remove src5 from interface section

5599636

fix symbol link

42f7b8f

fix safe descriptions

3fe29ff

fix camel fn order

e7d7500

fix typos

6bfc301

simplify interface section

d1146c9

fix impl order in code blocks

2e45ee0

fix usage sentence

94734d9

add erc20 camel impl to examples

033df18

fix usage section

51f6d77

ericnordelo reviewed Nov 22, 2023

View reviewed changes

andrew-fleming and others added 2 commits November 22, 2023 10:20

Apply suggestions from code review

563e177

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

reorder components in code examples

b6e512c

ericnordelo approved these changes Nov 23, 2023

View reviewed changes

martriay reviewed Nov 24, 2023

View reviewed changes

martriay approved these changes Nov 24, 2023

View reviewed changes

docs/modules/ROOT/pages/api/erc721.adoc Outdated Show resolved Hide resolved

docs/modules/ROOT/pages/erc721.adoc Show resolved Hide resolved

Apply suggestions from code review

054caee

Co-authored-by: Martín Triay <martriay@gmail.com>

martriay approved these changes Nov 26, 2023

View reviewed changes

martriay merged commit 50ed6b5 into OpenZeppelin:main Nov 26, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update erc721 docs #763

Update erc721 docs #763

andrew-fleming commented Oct 1, 2023

martriay left a comment

martriay Nov 10, 2023

andrew-fleming Nov 12, 2023

martriay Nov 24, 2023 •

edited

andrew-fleming Nov 24, 2023

martriay Nov 26, 2023 •

edited

martriay Nov 26, 2023 •

edited

ericnordelo left a comment

ericnordelo Nov 22, 2023

ericnordelo Nov 22, 2023

ericnordelo left a comment

martriay Nov 24, 2023 •

edited

martriay left a comment •

edited

andrew-fleming commented Nov 25, 2023 •

edited

martriay left a comment

		component!(path: SRC5Component, storage: src5, event: SRC5Event);
		component!(path: ERC721Component, storage: erc721, event: ERC721Event);

		component!(path: SRC5Component, storage: src5, event: SRC5Event);
		component!(path: ERC721ReceiverComponent, storage: erc721_receiver, event: ERC721ReceiverEvent);

Update erc721 docs #763

Update erc721 docs #763

Conversation

andrew-fleming commented Oct 1, 2023

martriay left a comment

Choose a reason for hiding this comment

martriay Nov 10, 2023

Choose a reason for hiding this comment

andrew-fleming Nov 12, 2023

Choose a reason for hiding this comment

martriay Nov 24, 2023 • edited

Choose a reason for hiding this comment

andrew-fleming Nov 24, 2023

Choose a reason for hiding this comment

martriay Nov 26, 2023 • edited

Choose a reason for hiding this comment

martriay Nov 26, 2023 • edited

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

ericnordelo Nov 22, 2023

Choose a reason for hiding this comment

ericnordelo Nov 22, 2023

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

martriay Nov 24, 2023 • edited

Choose a reason for hiding this comment

martriay left a comment • edited

Choose a reason for hiding this comment

andrew-fleming commented Nov 25, 2023 • edited

martriay left a comment

Choose a reason for hiding this comment

martriay Nov 24, 2023 •

edited

martriay Nov 26, 2023 •

edited

martriay Nov 26, 2023 •

edited

martriay Nov 24, 2023 •

edited

martriay left a comment •

edited

andrew-fleming commented Nov 25, 2023 •

edited