Refactor directory structure by andrew-fleming · Pull Request #350 · OpenZeppelin/cairo-contracts

andrew-fleming · 2022-06-01T16:19:17Z

Resolves #335.

This PR proposes to separate libraries and preset contracts in separate directories. For library-only directories (like security), this PR includes the library directory here as well i.e.
from openzeppelin.security.library.Initializable import Initializable.

This PR also proposes to remove underscores from library and preset contract names meaning ERC20_Mintable_Burnable.cairo is now ERC20MintableBurnable.cairo.

…g/cairo-contracts into refactor-structure

JulissaDantes

I think it looks good, should we also update the contributing guidelines of our CONTRIBUTING file?

…ts#350" This reverts commit 3b60db7.

martriay

Left a few comments on the CONTRIBUTING section to orient the refactor design towards the latest proposal.

CONTRIBUTING.md

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

andrew-fleming · 2022-07-15T02:28:23Z

Some things to note in this latest iteration:

the security directory includes subdirectories for each library i.e.
- openzeppelin/security/initializable/library.cairo
- openzeppelin/security/pausable/library.cairo
the ERC721Enumerable library follows @martriay's suggestion => openzeppelin/token/erc721/enumerable/library.cairo
the interfaces directories have been removed. Interfaces now sit alongside the library:
- openzeppelin/token/erc20/library.cairo
- openzeppelin/token/erc20/IERC20.cairo

martriay

Looks very good! Amazing progress and I think this is going to be a very positive change for the project. I'm slightly concerned about conflicts between this and the docs integration PRs. Don't forget to merge the latest main as soon there's anything new merged to avoid changes to be lost in the file migration.

e.g. the recent SPDX identifiers bump might get lost otherwise

martriay · 2022-07-14T13:38:13Z

CONTRIBUTING.md

  - And a visual guide:

-    ```
+```python


why python?

The linter expects a language with the code block. Python is an arbitrary solution since this is just a visual aid

martriay · 2022-07-15T17:31:02Z

docs/ERC20.md

+  - [ERC20Mintable](#erc20mintable)
+  - [ERC20Pausable](#erc20pausable)
+  - [ERC20Upgradeable](#erc20upgradeable)


what a lovely race condition with #396

docs/ERC721.md

docs/Introspection.md

docs/Security.md

martriay · 2022-07-15T17:48:22Z

src/openzeppelin/token/erc721/utils/ERC721Holder.cairo

@@ -1,14 +1,14 @@
 # SPDX-License-Identifier: MIT
-# OpenZeppelin Contracts for Cairo v0.2.0 (token/erc721/utils/ERC721_Holder.cairo)
+# OpenZeppelin Contracts for Cairo v0.2.0 (token/erc721/utils/ERC721Holder.cairo)


mh this one is not strictly following the presets/ directory requirement in CONTRIBUTING. not sure if it needs to, but at least it's worth noticing

maybe moving utils so it's erc721/presets/utils instead? or just removing the directory entirely

Great point...I'll try the former and see how we like it

martriay · 2022-07-15T17:50:46Z

tests/mocks/ERC721SafeMintableMock.cairo

@@ -1,12 +1,13 @@
+# SPDX-License-Identifier: MIT


why doesn't it need the file name?

I initially used the openzeppelin-contracts as a template and that repo doesn't include file names for mocks. Do you prefer we add them in?

martriay

We're getting close. Not sure what to do re: proxy. I wouldn't make Proxies all about upgrades, let alone remove the concept of upgrades from the names.

Either

openzeppelin/upgrades/library.cairo
openzeppelin/upgrades/presets/Proxy.cairo

or

openzeppelin/proxy/upgrades/library.cairo
openzeppelin/proxy/presets/UpgradeableProxy.cairo

tox.ini

tests/mocks/Ownable.cairo

src/openzeppelin/proxy/presets/Proxy.cairo

andrew-fleming · 2022-07-22T19:37:59Z

We're getting close. Not sure what to do re: proxy. I wouldn't make Proxies all about upgrades, let alone remove the concept of upgrades from the names.

Either
openzeppelin/upgrades/library.cairo
openzeppelin/upgrades/presets/Proxy.cairo
or
openzeppelin/proxy/upgrades/library.cairo
openzeppelin/proxy/presets/UpgradeableProxy.cairo

I think the former makes the most sense. I'll update and see what we think

martriay

Proxy looks good! We're almost done: I just noticed theres some broken links in the docs e.g.

cairo-contracts/docs/Access.md

Line 44 in 5cd09ad

    
           OpenZeppelin Contracts for Cairo provides [Ownable](../src/openzeppelin/access/ownable.cairo) for implementing ownership in your contracts.

martriay · 2022-07-23T20:18:22Z

docs/Access.md

 ### Quickstart

-Integrating [Ownable](../src/openzeppelin/access/ownable.cairo) into a contract first requires assigning an owner. The implementing contract's constructor should set the initial owner by passing the owner's address to Ownable's [initializer](#initializer) like this:
+Integrating [Ownable](../src/openzeppelin/access/library.cairo) into a contract first requires assigning an owner. The implementing contract's constructor should set the initial owner by passing the owner's address to Ownable's [initializer](#initializer) like this:


Suggested change

Integrating [Ownable](../src/openzeppelin/access/library.cairo) into a contract first requires assigning an owner. The implementing contract's constructor should set the initial owner by passing the owner's address to Ownable's [initializer](#initializer) like this:

Integrating [Ownable](../src/openzeppelin/access/ownable/library.cairo) into a contract first requires assigning an owner. The implementing contract's constructor should set the initial owner by passing the owner's address to Ownable's [initializer](#initializer) like this:

andrew-fleming and others added 9 commits May 31, 2022 12:15

add lib and preset dirs

6294925

remove underscore from names, finish init reorganization

d154923

fix mock names, adjusted paths

850030a

update branch Merge branch 'main' of https://github.com/andrew-flemin…

751ea89

…g/cairo-contracts into refactor-structure

update paths in tests

eec911c

update attribution

ad2b4ab

Rename safemath.cairo to SafeMath.cairo

208a673

Rename initializable.cairo to Initializable.cairo

05b78a3

Rename pausable.cairo to Pausable.cairo

ea9966f

andrew-fleming requested review from JulissaDantes and frangio June 1, 2022 17:21

andrew-fleming added 3 commits June 3, 2022 15:08

update version in attribution

9588959

update branch Merge branch 'main' of https://github.com/andrew-flemin…

845ac1b

…g/cairo-contracts into refactor-structure

update branch Merge branch 'main' of https://github.com/andrew-flemin…

3396b07

…g/cairo-contracts into refactor-structure

JulissaDantes reviewed Jun 16, 2022

View reviewed changes

andrew-fleming added 2 commits June 17, 2022 15:33

update branch, fix paths

1a6a6a5

add conventions to CONTRIBUTING.md

a88af6a

ericglau added a commit to ericglau/contracts-wizard that referenced this pull request Jun 17, 2022

Update directory structure pending OpenZeppelin/cairo-contracts#350

3b60db7

andrew-fleming added 2 commits June 18, 2022 14:16

update names and paths in docs

3de6caa

fix paths in code snippets

87dc199

andrew-fleming marked this pull request as draft June 24, 2022 18:55

ericglau added a commit to ericglau/contracts-wizard that referenced this pull request Jun 27, 2022

Revert "Update directory structure pending OpenZeppelin/cairo-contrac…

f329e91

…ts#350" This reverts commit 3b60db7.

martriay suggested changes Jul 13, 2022

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

andrew-fleming and others added 6 commits July 13, 2022 23:42

rename libs to lib

b0d59f8

Apply suggestions from code review

2d03060

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

Update CONTRIBUTING.md

66fc6c4

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

update link paths

cc573e5

update structure

58fb80d

fix compilation msg and import snippet

0128e7e

andrew-fleming marked this pull request as ready for review July 15, 2022 02:28

martriay suggested changes Jul 15, 2022

View reviewed changes

andrew-fleming added 2 commits July 15, 2022 15:43

update branch

d849fde

Apply suggestions from code review

cc9ccfb

andrew-fleming force-pushed the refactor-structure branch from caf886d to cc9ccfb Compare July 15, 2022 19:58

fix paths, move ERC721Holder

a551c45

andrew-fleming marked this pull request as draft July 15, 2022 21:40

andrew-fleming added 3 commits July 15, 2022 23:40

remove upgrades dir

7686794

fix pin

8a22da1

update branch

1984e9d

andrew-fleming marked this pull request as ready for review July 20, 2022 05:58

martriay suggested changes Jul 22, 2022

View reviewed changes

tox.ini Show resolved Hide resolved

tests/mocks/Ownable.cairo Show resolved Hide resolved

src/openzeppelin/proxy/presets/Proxy.cairo Show resolved Hide resolved

andrew-fleming marked this pull request as draft July 22, 2022 19:38

update paths for proxy

5cd09ad

andrew-fleming marked this pull request as ready for review July 22, 2022 21:33

martriay suggested changes Jul 23, 2022

View reviewed changes

update links and code block imports

678093f

martriay reviewed Jul 23, 2022

View reviewed changes

martriay approved these changes Jul 23, 2022

View reviewed changes

martriay merged commit 243df2b into OpenZeppelin:main Jul 23, 2022

ericglau mentioned this pull request Jul 26, 2022

Cairo: Update directory structure OpenZeppelin/contracts-wizard#157

Merged

	Integrating [Ownable](../src/openzeppelin/access/library.cairo) into a contract first requires assigning an owner. The implementing contract's constructor should set the initial owner by passing the owner's address to Ownable's [initializer](#initializer) like this:
	Integrating [Ownable](../src/openzeppelin/access/ownable/library.cairo) into a contract first requires assigning an owner. The implementing contract's constructor should set the initial owner by passing the owner's address to Ownable's [initializer](#initializer) like this:

Conversation

andrew-fleming commented Jun 1, 2022

Uh oh!

JulissaDantes left a comment

Choose a reason for hiding this comment

Uh oh!

martriay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrew-fleming commented Jul 15, 2022

Uh oh!

martriay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martriay left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrew-fleming commented Jul 22, 2022

Uh oh!

martriay left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martriay left a comment •

edited

Loading

martriay left a comment •

edited

Loading