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.

Sign up for GitHub

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

Jump to bottom

ISemver policy #13508

Open
JosepBove opened this issue Dec 20, 2024 · 2 comments · May be fixed by #13567
Open

ISemver policy #13508

JosepBove opened this issue Dec 20, 2024 · 2 comments · May be fixed by #13567
Labels
T-evm-safety Team: EVM Safety

Comments

@JosepBove
Copy link

JosepBove commented Dec 20, 2024 •
edited
Loading

This issue stems from a discussion with @maurelian about whether RISC-V.sol should implement the ISemver interface and the need to clarify and potentially standardize our policy on the use of ISemver across contracts.

The file RISC-V.sol is currently vendored and does not inherit from ISemver. This might be due to its origin as a copy-paste from another repository where ISemver does not exist. While its inclusion could improve consistency, this raises questions about our general approach to vendor contracts and ISemver.

In our discussion, we also noted that ISemver is primarily used to enforce implementation of versioning. However, this practice might be inconsistent with other parts of the codebase.

Questions that raised:

  • Should all on-chain contracts, including vendor contracts like RISC-V.sol, implement ISemver?
  • Should we enforce the ISemver interface only through checks rather than inheritance, especially for vendor contracts?
@maurelian
Copy link
Contributor

If there was only one actionable thing to come out of this, it would be adding a semgrep check to ensure onchain contracts conform to some standard.

@maurelian maurelian added the T-evm-safety Team: EVM Safety label Dec 20, 2024
@tynes
Copy link
Contributor

tynes commented Dec 20, 2024

  • RISCV.sol should implement ISemver. Its vendored from asterisc because we don't know how to work without a monorepo. https://github.com/ethereum-optimism/asterisc
  • Why would RISCV.sol inherit ISystemConfig? You left out context here. Do you mean IRISCV.sol? If so, @smartcontracts @mds1 are the decision makers here. I am of the opinion yes, because it will make writing assertions easier and have less of an impact on compile times (just import the interface rather than the impl to make the calls)
  • What is the benefit of not inheriting ISemver? Seems like extra tooling requirement that we need to maintain rather than just using the compiler itself

Abuchtela added a commit to Abuchtela/optimism that referenced this issue Jan 2, 2025
@Abuchtela
Implement ISemver in all on-chain contracts
1595401 
Fixes ethereum-optimism#13508

Add `ISemver` implementation to `RISCV.sol` and semgrep rule for `ISemver` compliance.

* **RISCV.sol**
  - Add `ISemver` to the list of inherited contracts.
  - Add the `version()` function from `ISemver`.
  - Set the `version` constant to "1.2.0-rc.1".

* **.semgrep/rules/sol-rules.yaml**
  - Add a semgrep rule to ensure all on-chain contracts implement `ISemver`.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/ethereum-optimism/optimism/issues/13508?shareId=XXXX-XXXX-XXXX-XXXX).
@Abuchtela Abuchtela linked a pull request Jan 2, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
T-evm-safety Team: EVM Safety
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement ISemver in all on-chain contracts Abuchtela/optimism
3 participants
@tynes @maurelian @JosepBove