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

📌 Contribution guidelines #246

Open
Vectorized opened this issue Apr 18, 2022 · 0 comments
Open

📌 Contribution guidelines #246

Vectorized opened this issue Apr 18, 2022 · 0 comments

Comments

@Vectorized
Copy link
Collaborator

Vectorized commented Apr 18, 2022

On Golfing

While we love gas efficient code, we need to strike a balance between performance, readability, extendability.

As such, we will need to be more selective on gas optimizations going forward.

While it is possible to rewrite 80%+ of the library in assembly, substituting a 1 liner in Solidity with 30 lines of assembly to save 3 gas is not exactly conducive for maintaining the overall balance.

We have purposefully included some examples for demonstration purposes where appropriate, for example the included _toString method, which would likely be substituted with an external assembly based library otherwise; but for the general scope, it is best to stick to Solidity if possible.

More gas savings can be done by proper choice of overall approach outside of ERC721A,
and combining + utilizing various libraries (e.g. Solmate, OpenZeppelin, ERC721A) to their fullest potential.

Nevertheless, if you have an interesting idea, do feel free to open a issue or PR. Even if the gas saved may not be much, it can open new insights into the subtleties of the language and inspire new ideas. Your PR may not be accepted (balance is a very subjective issue), but it will still be greatly appreciated.

Extensions to be Excluded

The OpenZeppelin project has several ERC721 extensions.

  • ERC721URIStorage.sol
  • ERC721Pausable.sol
  • ERC721Royalty.sol

In my opinion, the functionality of these extensions should NOT be included in ERC721A for the following reasons:

  • They are orthogonal to ERC721A's logic.

    • Can be implemented easily by devs with just a few lines of code via overriding the beforeTokensTransfer hook.
    • Self implementations can be easily made more efficient (e.g. use a bool packed with the sale state for Pausable).
  • Manual overriding of the beforeTokensTransfer hook gives more flexibility and neater inheritance, especially when combining two or more features.

  • For repo maintainability.

    • Some extensions (Royalty, Pausable) are mainly just importing OpenZeppelin's libraries + a few lines of code.
    • Heavy duplication of documentation and test cases must be maintained on our side to match any updates on OpenZeppelin's side.
    • Alternatively, in-sourcing the code will lead to a loss of focus.

Our main purpose is to provide efficient batch minting for the ERC721 standard.
When applicable, we may include additional features (e.g. holding startTimestamp, _numberMinted)
when the additional gas overhead is neligible ("storage hitchhiking").

The remaining extensions (Burnable, Queryable) are heavily interwoven with ERC721A's core logic.
These should be kept.

I would like to take this chance to refer readers to the comment of the t11s:
Screen Shot 2022-04-19 at 3 36 04 AM
https://twitter.com/transmissions11/status/1510015319334535170

As much as we like to offer features for convenience, we must also know when to exclude features.

If you need code examples on how to self-implement these excluded functionality, please feel free to request.

@Vectorized Vectorized pinned this issue Apr 18, 2022
@Vectorized Vectorized unpinned this issue Apr 19, 2022
@Vectorized Vectorized linked a pull request Apr 29, 2022 that will close this issue
@Vectorized Vectorized reopened this Apr 30, 2022
@Vectorized Vectorized pinned this issue Apr 30, 2022
@Vectorized Vectorized changed the title Extensions to be excluded. 📌 Extensions to be excluded. Jun 6, 2022
@Vectorized Vectorized changed the title 📌 Extensions to be excluded. 📌 Contribution Guidelines Jun 15, 2022
@Vectorized Vectorized changed the title 📌 Contribution Guidelines 📌 Contribution guidelines Jun 15, 2022
@Vectorized Vectorized removed a link to a pull request Jun 15, 2022
@holic holic mentioned this issue Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant