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

Update solidity contracts version dependency from fixed version to using ^ specifier #888

Closed
jimmychu0807 opened this issue Oct 29, 2024 · 3 comments · Fixed by #891
Closed
Assignees
Labels
refactoring ♻️ A code change that neither fixes a bug nor adds a feature

Comments

@jimmychu0807
Copy link
Contributor

jimmychu0807 commented Oct 29, 2024

Describe the improvement you're thinking about

Update all solidity contracts version from:

pragma solidity 0.8.23;

to

pragma solidity ^0.8.23;

or to a version range that core developers agree.

Additional context
The reason is because Semaphore is becoming a library (building block) for dApp developers, and they integrate Semaphore into their dApp contracts. Using a fixed version specification 0.8.23 in Semaphore is restricting the solidity version used in downstream development.

There maybe a repercussion of updating zk-kit.solidity as well.

@jimmychu0807 jimmychu0807 changed the title Update solidity contracts version dependency from fixed version to using carat (^) specifier Update solidity contracts version dependency from fixed version to using caret (^) specifier Oct 29, 2024
@jimmychu0807 jimmychu0807 changed the title Update solidity contracts version dependency from fixed version to using caret (^) specifier Update solidity contracts version dependency from fixed version to using ^/~ specifier Oct 29, 2024
@jimmychu0807 jimmychu0807 changed the title Update solidity contracts version dependency from fixed version to using ^/~ specifier Update solidity contracts version dependency from fixed version to using ^ specifier Oct 29, 2024
@cedoor
Copy link
Member

cedoor commented Oct 30, 2024

Hey @jimmychu0807, as a good practice we've tried to keep those versions fixed to avoid any unexpected behavior.

Context: #115

But curious to know your opinion on this.

@jimmychu0807
Copy link
Contributor Author

jimmychu0807 commented Oct 31, 2024

I agree there is actually a balance here, fixed version being restrictive and safe, and a wide version range being loosed and have a higher potential security risk.

A few thought points on this:

  • If the set of smart contracts are gear toward being an end-product, then using fixed version is very reasonable. We don't expect other dev to integrate them. If the smart contracts are expecting to be a library that expect downstream developers to integrate, using fixed version is restricting them as well. As a reference, none of the openzeppelin contracts use fixed version. I am hitting this issue as I integrate Semaphore into ERC-7579 module and require 0.8.25 as the min solidity version.

  • At this point Semaphore is becoming more and more like a library than an end-product. It provides zk functionalities that other dApps want to integrate. So currently, this restricts all downstream dApp contracts to use the solidity version Semaphore uses. Of course, they can always fork Semaphore repo and change it themselves.

  • One possible (and more balanced) approach is making the version range to be restrictive such as pragma solidity >=0.8.23 <=0.8.28; The LTE version could be the latest solidity version that core dev team have tested and work with Semaphore. This ensures the security safety while giving downstream dev flexibility to pick their solidity version.

@cedoor
Copy link
Member

cedoor commented Oct 31, 2024

One possible (and more balanced) approach is making the version range to be restrictive such as pragma solidity >=0.8.23 <=0.8.28; The LTE version could be the latest solidity version that core dev team have tested and work with Semaphore. This ensures the security safety while giving downstream dev flexibility to pick their solidity version.

I like this compromise!

Curious to hear @vplasencia's opinion here.

PR: #891

cedoor added a commit that referenced this issue Oct 31, 2024
@cedoor cedoor self-assigned this Oct 31, 2024
@cedoor cedoor added the refactoring ♻️ A code change that neither fixes a bug nor adds a feature label Oct 31, 2024
@cedoor cedoor moved this to 👀 In review in Semaphore Board Oct 31, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✔️ Done in Semaphore Board Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring ♻️ A code change that neither fixes a bug nor adds a feature
Projects
Status: ✔️ Done
Development

Successfully merging a pull request may close this issue.

2 participants