Skip to content
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 #335

Closed
martriay opened this issue May 23, 2022 · 4 comments · Fixed by #350
Closed

Refactor directory structure #335

martriay opened this issue May 23, 2022 · 4 comments · Fixed by #350

Comments

@martriay
Copy link
Contributor

martriay commented May 23, 2022

Taking into consideration that:

  1. it's not clear which modules are libraries and which are contracts (presets), other than libs being lowercased and contracts Sna_Melcased
  2. some libraries are called library.cairo and some others are named as the module (e.g. ownable.cairo)
    • some users also complained about their workflows being affected by having multiple modules with the same name (library.cairo)
  3. the switch to namespaces enabled happier imports such as from openzeppelin.upgrades.library import Proxy
  4. ERC721_Enumerable should probably live under ERC721
  5. should we rename ERC20_Mintable.cairo to ERC20Mintable.cairo? it's weird

I suggest to put presets and libraries in different directories, thus allowing libraries to be named after their namespaces and not library.cairo. Maybe something like:

  • openzeppelin.library.token import ERC20 and openzeppelin.presets.token import ERC20Mintable
  • openzeppelin.token.library import ERC20 and openzeppelin.token.presets import ERC20Mintable

Although that doesn't fully solve 4

@andrew-fleming
Copy link
Collaborator

With those examples, we'd still have to include the module name first before namespace, right? If so, I think this would be more accurate:

  • openzeppelin.library.token.erc20 import ERC20 and openzeppelin.presets.token.erc20 import ERC20Mintable
  • openzeppelin.token.library.erc20 import ERC20 and openzeppelin.token.presets.erc20 import ERC20Mintable

If we're following the examples, we could include ERC721Enumerable in the ERC721 lib. Importing would look something like this:

  • openzeppelin.library.token.erc721 import ERC721
  • openzeppelin.library.token.erc721 import ERC721Enumerable

And in the actual library module:

# src/openzeppelin/.../erc721

namespace ERC721:
...
end

namespace ERC721Enumerable:
...
end

I don't love this approach because I think it's easier to navigate/maintain the repo with the extension separated. The precedent this sets would be that extensions should be included in the same module :( but this is consistent with the proposed examples and solves 4.

@frangio
Copy link
Contributor

frangio commented May 27, 2022

I liked this proposal:

  • openzeppelin.token.library import ERC20 and openzeppelin.token.presets import ERC20Mintable

If we need to include erc20 in the path I think it should be:

  • openzeppelin.token.erc20.library and openzeppelin.token.erc20.presets

That is, library and presets should be the last components in the path.

@martriay
Copy link
Contributor Author

With those examples, we'd still have to include the module name first before namespace, right?

I think we can do either.

I don't love this approach because I think it's easier to navigate/maintain the repo with the extension separated. The precedent this sets would be that extensions should be included in the same module :( but this is consistent with the proposed examples and solves 4.

We could also do:

from openzeppelin.token.erc721.library import ERC721

and

from openzeppelin.token.erc721.enumerable.library import ERC721Enumerable

I liked this proposal:

  • openzeppelin.token.library import ERC20 and openzeppelin.token.presets import ERC20Mintable

If we need to include erc20 in the path I think it should be:

  • openzeppelin.token.erc20.library and openzeppelin.token.erc20.presets

That is, library and presets should be the last components in the path.

Although this helps keep files together, it does not solve 2.

@martriay martriay added this to the v0.2.1 milestone Jul 4, 2022
@martriay martriay moved this to 📋 Backlog in Cairo team Jul 4, 2022
@martriay martriay moved this from 📋 Backlog to 🏗 In progress in Cairo team Jul 4, 2022
@martriay
Copy link
Contributor Author

martriay commented Jul 13, 2022

Following up on my last comment, even though that final proposal does not fully solve 2, it does solve the consistency issue (ownable.cairo -> ownable/library.cairo). Regarding the user's workflow issue, I think this alternative might be the lesser evil and it's still solvable outside this library, so I think we should follow that path.

Repository owner moved this from 🏗 In progress to ✅ Resolved in Cairo team Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants