Skip to content

fix(ci): skip storage check for new contracts without baseline#1047

Closed
wbobbynmworley wants to merge 1 commit into
ubiquity:developmentfrom
wbobbynmworley:fix/972-storage-check-for-new-contracts
Closed

fix(ci): skip storage check for new contracts without baseline#1047
wbobbynmworley wants to merge 1 commit into
ubiquity:developmentfrom
wbobbynmworley:fix/972-storage-check-for-new-contracts

Conversation

@wbobbynmworley

Copy link
Copy Markdown

Summary

Closes #972

When a new contract is added to the codebase, both core-contracts-storage-check.yml and diamond-storage-check.yml workflows fail because there is no existing storage baseline to compare against.

Changes

core-contracts-storage-check.yml

Added check-new-contracts step that filters out contracts without a storage baseline (storage-layouts/.json). If ALL changed contracts are new (no baseline), the is_new_only flag is set to rue and the check_storage_layout job is skipped entirely.

diamond-storage-check.yml

Same fix applied to the diamond library storage check workflow.

QA

Per #972 requirements, this PR covers all test scenarios:

  1. No storage updates → CI check_storage_layout job doesn't run (matrix is empty)
  2. Storage update, no collision → CI passes, baseline updated
  3. Storage update, collision detected → CI fails (unchanged behavior)
  4. New contract added → CI passes (fixed by skipping new contracts without baseline)

How to Test

  1. Add a new contract to packages/contracts/src/dollar/core/
  2. Open a PR → CI should pass (storage check skipped for new contract)
  3. Modify an existing contract's storage → CI should check and pass/fail appropriately

Bounty: (Issue #972)

Closes ubiquity#972

Problem: When a new contract is added, core-contracts-storage-check.yml
and diamond-storage-check.yml fail because there's no storage baseline
to compare against.

Solution: Add a step that checks if each changed contract/library has
an existing storage baseline in storage-layouts/. Skip storage checks
for contracts that are new (no baseline exists).

QA covered:
1. No storage updates -> CI passing
2. Storage update, no collision -> CI passing
3. Storage update, collision -> CI failing
4. New contract added -> CI passing (fixed)
@wbobbynmworley wbobbynmworley requested a review from rndquu as a code owner May 19, 2026 04:06
@ubiquity-os ubiquity-os Bot closed this May 19, 2026
@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Both GitHub Actions workflows (core-contracts-storage-check.yml and diamond-storage-check.yml) now detect when only newly added contracts or libraries exist—those lacking corresponding storage-layout baseline files. Each workflow adds a detection step that filters the contract/library matrix, sets an is_new_only output flag, and updates job outputs to support fallback logic. The downstream storage-layout check job execution condition is extended to skip when is_new_only == 'true', preventing false CI failures on new contract or library additions.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding logic to skip storage checks for new contracts without baselines.
Description check ✅ Passed Description includes issue link, detailed changes, QA validation of all four scenarios, and testing instructions matching the template.
Linked Issues check ✅ Passed Code changes fully implement all objectives: both workflows skip checks for new contracts/libraries, preserve existing behavior for updates, and QA covers all four scenarios.
Out of Scope Changes check ✅ Passed Changes are narrowly scoped to the two storage check workflows; no unrelated modifications present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1d792cff-0b30-4a0e-9616-6a66f417cc88

📥 Commits

Reviewing files that changed from the base of the PR and between be9e35e and 840db95.

📒 Files selected for processing (2)
  • .github/workflows/core-contracts-storage-check.yml
  • .github/workflows/diamond-storage-check.yml

Comment on lines +60 to +61
grep -v "^$CONTRACT$" contracts.txt > contracts_filtered.txt || true
mv contracts_filtered.txt contracts.txt

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use fixed-string filtering when removing matrix entries.

grep -v "^$CONTRACT$" treats $CONTRACT as regex. A path like src/dollar/core/Foo.sol:Foo contains regex-significant chars (e.g., .), so this can remove unintended lines and silently skip required checks.

Suggested patch
-                grep -v "^$CONTRACT$" contracts.txt > contracts_filtered.txt || true
+                grep -Fvx -- "$CONTRACT" contracts.txt > contracts_filtered.txt || true
                 mv contracts_filtered.txt contracts.txt

Comment on lines +59 to +60
grep -v "^$LIB$" contracts.txt > contracts_filtered.txt || true
mv contracts_filtered.txt contracts.txt

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use fixed-string matching for library filtering as well.

Dynamic regex in grep -v "^$LIB$" can overmatch and drop non-target libraries from contracts.txt, which may skip checks that should run.

Suggested patch
-                grep -v "^$LIB$" contracts.txt > contracts_filtered.txt || true
+                grep -Fvx -- "$LIB" contracts.txt > contracts_filtered.txt || true
                 mv contracts_filtered.txt contracts.txt

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 this pull request may close these issues.

CI: fix check_storage_layout for new contracts

1 participant