-
Notifications
You must be signed in to change notification settings - Fork 328
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.
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
Refactor directory structure #350
Refactor directory structure #350
Conversation
…g/cairo-contracts into refactor-structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good, should we also update the contributing guidelines of our CONTRIBUTING file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments on the CONTRIBUTING section to orient the refactor design towards the latest proposal.
Co-authored-by: Martín Triay <martriay@gmail.com>
Co-authored-by: Martín Triay <martriay@gmail.com>
Some things to note in this latest iteration:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
- `openzeppelin.token.erc20.presets.ERC20Mintable` | ||
- And a visual guide: | ||
|
||
``` | ||
```python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why python?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter expects a language with the code block. Python is an arbitrary solution since this is just a visual aid
- [ERC20Mintable](#erc20mintable) | ||
- [ERC20Pausable](#erc20pausable) | ||
- [ERC20Upgradeable](#erc20upgradeable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what a lovely race condition with #396
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe moving utils
so it's erc721/presets/utils
instead? or just removing the directory entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point...I'll try the former and see how we like it
@@ -1,12 +1,13 @@ | |||
# SPDX-License-Identifier: MIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why doesn't it need the file name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need
caf886d
to
cc9ccfb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
|
||
### 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
Resolves #335.
This PR proposes to separate libraries and preset contracts in separate directories. For library-only directories (like
security
), this PR includes thelibrary
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 nowERC20MintableBurnable.cairo
.