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

Suggestion: add contract version #138

Closed
ahbanavi opened this issue Feb 26, 2022 · 6 comments · Fixed by #232
Closed

Suggestion: add contract version #138

ahbanavi opened this issue Feb 26, 2022 · 6 comments · Fixed by #232

Comments

@ahbanavi
Copy link
Contributor

ahbanavi commented Feb 26, 2022

It would be a good idea to add the release version on top of the contracts/ERC721A.sol file and update it when a new version releases.
Some devs or savvy people would like to know what version of ERC721A the implemented contract uses.

// SPDX-License-Identifier: MIT
// ERC721A v2.2.0
// Creator: Chiru Labs
.
.
.
@ahbanavi
Copy link
Contributor Author

ahbanavi commented Mar 1, 2022

@cygaar please consider this for next version release, I'm waiting to hear your thoughts before closing this issue.

@Austinhs
Copy link
Contributor

Just to add to the argument - It would be nice to have this version stamp as well for contract review. It's really different when you are reviewing someone with v1 instead of v2, would save a lot of time on parsing the contract to try and figure out when did they copy/install the ERC721A contract

@Vectorized
Copy link
Collaborator

Vectorized commented Mar 31, 2022

Probably will look into some workflow that can run a script to do this some time after next version.

I suggest either a shell script (e.g. scripts/replace-contract-versions.sh), or js snippet (e.g. scripts/replace-contract-versions.js) that can read from package.json and update the files in contracts.

For an example, see https://github.com/Vectorized/ERC721A-Upgradeable/

If anyone wants to make the script and/or workflow file, feel free to open a PR.

@ahbanavi
Copy link
Contributor Author

ahbanavi commented Mar 31, 2022

It would be nice to have this version stamp as well for contract review.

@Austinhs Exactly, that was my main intention for raising this issue.

@ahbanavi
Copy link
Contributor Author

ahbanavi commented Apr 7, 2022

I suggest either a shell script (e.g. scripts/replace-contract-versions.sh), or js snippet (e.g. scripts/replace-contract-versions.js) that can read from package.json and update the files in contracts.

I'm working on a script to change the package.json version and comments simultaneously, so maintainers could just update all files on bump version PR, like #179 #39

I'm using this issue to discuss the implementation of this.

you can see the script here at scripts/release/update-version.js
the usage is as simple as:

./scripts/release/update-version.js 3.2.0

or even use it as:

npm run update-version 3.2.0

And then, the package.json version and all other files will update.
The header of all .sol files will look like this:

// SPDX-License-Identifier: MIT
// ERC721A Contracts v3.2.0
// Creator: Chiru Labs

We can also ignore the mocks, but adding a version comment will not do any harm, IMO.

I would like to mention that this script will update all files, apart from whether they have been modified since the last tag version.
We can update just modified files using git, like the OZ implementation of this script. See here

Probably will look into some workflow

I don't like changing source code and committing it to the main branch in the workflow; problems may occur. Using this script manually is okay, IMO.

I'm waiting to hear your thoughts on this implementation before opening a PR.
@cygaar @Vectorized

@Vectorized
Copy link
Collaborator

@ahbanavi Make a PR.

My 2 cents:

While OZ only updates the version in the updated files, I think having the same version number across all files has the advantage that it is easy to know two files are meant for each other. For example #214 was caused by the files being from different versions.

Also, it is simpler to just replace the version number in all the .sol files.

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

Successfully merging a pull request may close this issue.

3 participants