fix: remove mock contracts from production#65
Conversation
WalkthroughPackage.json is updated to expand the files list from a single "contracts" entry to explicit subdirectories and specific contract files, add a new Changes
Sequence Diagram(s)sequenceDiagram
actor Dev
Dev->>Build: npm run build
Build->>Build: existing build steps
Build->>ArtifactsCopy: build:artifacts:copy
ArtifactsCopy->>FS: copy artifacts
ArtifactsCopy->>FS: rm ./dist/artifacts/contracts/mocks
Build->>AssertScript: ci:assert:no-mocks
AssertScript->>FS: check mocksDir exists?
alt mocks found
AssertScript->>AssertScript: aggregate errors
AssertScript->>Dev: exit(1) + error log
else no mocks
AssertScript->>Dev: success message
Build->>Dev: build complete ✓
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (failed) { | ||
| console.error(`\nERROR: Production artifacts contain mocks.\n${errors.map((e) => ` - ${e}`).join("\n")}`); |
There was a problem hiding this comment.
Unexpected console statement.
| console.error(`\nERROR: Production artifacts contain mocks.\n${errors.map((e) => ` - ${e}`).join("\n")}`); | ||
| process.exit(1); | ||
| } else { | ||
| console.log("No mock artifacts found in dist."); |
There was a problem hiding this comment.
Unexpected console statement.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ARCHITECTURE.md(1 hunks)package.json(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.29.0)
ARCHITECTURE.md
[high] 180-180: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
ARCHITECTURE.md
[grammar] ~249-~249: Ensure spelling is correct
Context: ...l relevant methods (remarkLengthLimit modifers in registry and escrow) - burn requir...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (4)
ARCHITECTURE.md (1)
1-284: Excellent architecture documentation—fills an important knowledge gap.This is comprehensive, well-organized, and provides exactly the kind of reference developers need to navigate the repository. The structure (overview → tech stack → layout → core contracts → SDK → tasks → workflows) is logical and maintains good cross-references to key files and concepts.
package.json (3)
16-26: Approve: explicit whitelisting prevents mock contracts from npm package.The expanded
filesarray now explicitly lists required contract source directories and core contract files while omittingcontracts/mocks. This is a more maintainable and intent-clear approach than a catch-all "contracts" entry.
54-54: Approve: build:artifacts:copy cleanup tightens production artifact handling.The script now removes debug artifacts (
.dbg.jsonfiles) and explicitly deletes the mock artifacts directory. This provides a good cleanup layer independent of the npmfilesarray configuration.
56-56: ci:assert:no-mocks validation confirmed — implementation is correct.All verification items have been confirmed:
- ✓
scripts/assertNoMocks.jsexists and is included in the PR- ✓ The script correctly detects and rejects mock artifacts via two methods: direct directory check for
dist/artifacts/contracts/mocksand recursive path traversal checking for/mocks/in artifact paths- ✓ The script runs after
build:artifacts:copycompletes (line 56 shows correct pipeline order) and exits with code 1 on mock detection, properly failing the buildThe implementation provides a solid production integrity gate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
71-73: Confirm the new ci:assert:no-mocks script is comprehensive.The new "ci:assert:no-mocks" script is correctly added with a trailing comma on the "coverage" line. However, this script references scripts/assertNoMocks.js, which is not included in the files for review.
Please provide scripts/assertNoMocks.js so I can verify:
- The assertion logic is comprehensive (checks for mock artifacts in all expected locations)
- The error messaging is clear for debugging
- No false positives or incomplete coverage
Alternatively, if you would like me to review the script once it's provided, I can assess its robustness and suggest improvements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json(3 hunks)
🔇 Additional comments (3)
package.json (3)
56-56: Verify the build pipeline ordering and script execution.The
ci:assert:no-mocksscript is now chained at the end of the build pipeline. This ordering is correct—the assertion runs after artifacts are cleaned. However, the assertNoMocks.js script itself is not provided for review, so I cannot verify what it actually checks.The following questions need confirmation:
- Does assertNoMocks.js check ./dist/artifacts/contracts for mock references?
- Does it search for both directory presence and file contents (in case mocks are conditionally imported)?
- Will it fail the build with a clear error message if mocks are detected?
- Are there any false positive risks (e.g., the word "mock" in comments or strings)?
Please provide the scripts/assertNoMocks.js file for review.
54-54: No action needed—the artifact cleanup is correctly implemented.The mocks directory is confirmed to be generated during the
hardhat compilestep (as evidenced by theassertNoMocks.jsvalidation script), and therm -rf ./dist/artifacts/contracts/mockscommand correctly removes it from the distribution. The-fflag makes it idempotent, and the path is correct relative to the project root where npm scripts execute. The removal is further validated by theci:assert:no-mocksstep that runs afterbuild:artifacts:copy.
16-26: Mock exclusion strategy is properly implemented across three defensive layers.Verification confirms the "files" list correctly excludes contracts/mocks, and only one mocks directory exists (./contracts/mocks, which will be excluded). The build process adds two additional safeguards: the build:artifacts:copy script explicitly removes mocks artifacts with
rm -rf ./dist/artifacts/contracts/mocks, and the ci:assert:no-mocks script validates post-build that no mock files remain in dist via both directory checks and deep path scanning. The assertion script exits with code 1 on failure, preventing release of builds containing mocks.
|
🎉 This PR is included in version 5.5.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Prevents mock contracts from being shipped or published by:
Summary by CodeRabbit