Conversation
0xferit
force-pushed
the
feature/osu-1514-add-date-based-version-tagging-ddmmyy-to-contracts-with-ci
branch
from
3dd4d90 to
e34b3f4
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dd4d9036e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Code Coverage Report for src/ files
|
0xferit
force-pushed
the
feature/osu-1514-add-date-based-version-tagging-ddmmyy-to-contracts-with-ci
branch
from
b07cc94 to
ee7ec9b
Compare
0xferit
force-pushed
the
feature/osu-1514-add-date-based-version-tagging-ddmmyy-to-contracts-with-ci
branch
from
ee7ec9b to
b135ee5
Compare
|
@copilot review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e37564491e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a1b170352
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e68d9ad86b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b5e7c0588
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5698da10b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| function extractContractNames(sourceCode) { | ||
| const names = []; | ||
| const regex = /\b(?:abstract\s+)?contract\s+([A-Za-z_][A-Za-z0-9_]*)\b/g; |
There was a problem hiding this comment.
This regex scans raw source text and matches comment phrases like contract address or contract for, producing fake names that are then passed to forge inspect in findInspectableContractsInSource; each false candidate spawns extra inspect subprocesses and can significantly inflate check time on heavily documented contracts. Parsing declarations without comments (or using a Solidity-aware parser) avoids this CI performance regression.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a13521155a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (oldPath.endsWith(".sol") && newPath.endsWith(".sol")) { | ||
| changedFiles.add(newPath); | ||
| renamedNewToOld[newPath] = oldPath; |
There was a problem hiding this comment.
When a Solidity file is renamed (R status), this branch only adds newPath to changedFiles, so later discovery never scans the old path. In the common refactor case where a file rename also renames the contract symbol, the old contract ID is never analyzed as removed, and analyzeContract can misclassify the change as a newly added contract instead of a breaking removal/rename. That underestimates required version bumps and can let incompatible changes pass the semver gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a13521155a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
scripts/sol-semver.mjs
Outdated
| if (/\bAPI_VERSION\b\s*=\s*"(\d+\.\d+\.\d+)"/.test(source)) { | ||
| hasVersion = true; | ||
| } | ||
| inspectable.push(...findInspectableContractsInSource(worktreeDir, sourcePath, source)); |
There was a problem hiding this comment.
This discovery path treats a file as versioned when any API_VERSION constant is present, but then adds every inspectable contract from that file to analysis. In a multi-contract file where only one contract is versioned, helper contracts without API_VERSION will still be checked and then fail under-bump with missing declared API_VERSION, which blocks valid PRs that only touch those helpers. Discovery should only enqueue contracts whose own declaration contains API_VERSION.
Useful? React with 👍 / 👎.
skimaharvey
left a comment
skimaharvey
left a comment
There was a problem hiding this comment.
Approving — this is a solid foundation for enforcing semver discipline on versioned contracts. A few points worth considering:
1. CI cost — two full forge build per run
The check creates two git worktrees and runs forge soldeer install + forge clean + forge build --skip test --skip script in each. On a growing codebase that's going to get expensive fast. Worth keeping an eye on wall-clock time once this lands on real PRs, and considering build artifact caching or only building the contracts in scope down the road.
2. Lock file machinery is wired up but never invoked
~100 lines of lock file validation (lock-entry-present, lock-format-valid, lock-not-downgraded, lock-vs-declared, lock-updated) are implemented but the CI workflow never passes --lock-file, so none of it runs. If this is intentional prep for a future phase, a comment or TODO explaining that would help — otherwise it reads as dead code.
3. Failure output is raw JSON
When the check fails, developers get a JSON blob in the CI logs. Writing a human-readable summary to $GITHUB_STEP_SUMMARY (even just the failures array formatted as a markdown table) would make version-bump violations much faster to act on without having to dig through structured output.
4. Bytecode-only diffs require a patch bump
After metadata stripping, any bytecode change (gas optimizations, internal refactors, compiler upgrades) forces a patch bump to API_VERSION. This is strict — these changes have zero impact on integrators. Might lead to version inflation or developers reflexively over-bumping. Worth a deliberate call on whether patch-level bytecode diffs should be advisory rather than enforced.
5. Storage comparison blind spot on mappings/dynamic arrays
forge inspect <contract> storage only reports fixed-layout variables. Mappings and dynamic arrays use hashed slots and don't appear in the layout output. So a change like mapping(address => uint) to mapping(bytes32 => uint) (breaking for storage integrity behind a proxy) would be invisible to the storage layer check. Not something the script can easily fix — it's a Forge limitation — but worth documenting as a known gap so reviewers know storage checks aren't exhaustive.
6. compareStorage early-returns on first issue
The function returns on the first breaking change it finds. If a contract has multiple storage problems, developers only see one per CI cycle, leading to a fix-one-rerun-fix-another loop. Collecting all issues before returning would save round-trips.
Thanks. Let's harden before merging. |
3 participants
Automates contract semver checks: analysis now targets only contracts that declare API_VERSION, and CI uses safe.directory for merge-base to avoid dubious-ownership failures.