Add git commit hash in output comments for Community Contracts #627
Conversation
WalkthroughAdds commit-aware Community Contracts metadata to generated Solidity headers, utilities to detect Community Contracts imports and extract a pinned git commit from package.json, UI logic to hyperlink imports to that commit when present, pins the devDependency to a specific commit, adds tests and snapshots, and documents an OpenZeppelin declaration file. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Generator as printContract
participant Imports as importsCommunityContracts
participant Commit as getCommunityContractsGitCommit
User->>Generator: Request contract generation
Generator->>Imports: Check for Community Contracts imports
Imports-->>Generator: true/false
alt imports Community Contracts
Generator->>Commit: Read pinned commit from package.json
Commit-->>Generator: <commit-hash>
Generator-->>User: Emit header including Community Contracts commit
else
Generator-->>User: Emit header without community commit
end
sequenceDiagram
participant UI as injectHyperlinks
participant Code as GeneratedCode
UI->>Code: Replace OpenZeppelin Contracts import lines with links
UI->>Code: Scan for "Community Contracts commit <hash>"
alt Commit present
UI->>Code: Replace Community Contracts imports with tree URL at commit
UI->>Code: Linkify commit line to commit URL
else
UI-->>Code: Skip community-specific replacements
end
UI-->>Code: Return hyperlink-enhanced code
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/core/solidity/src/utils/community-contracts-git-commit.ts (2)
3-9: Consider making failure modes non-fatal at call sitesThrowing here is fine to enforce configuration correctness. However, if this bubbles up during generation, it will break the Wizard for users in case of a packaging/config misalignment. I recommend wrapping calls in print.ts with a try/catch and skipping the community line if resolution fails.
To implement a non-fatal path, update printCompatibleLibraryVersions in print.ts to catch errors (see suggested diff in that file’s comment).
11-19: Validate that the segment after '#' is a 40-hex Git commit hashGiven the header explicitly says “commit”, we should enforce a full 40-hex hash. This prevents tags/branches from slipping in and ensures the UI can reliably hyperlink to a commit.
Apply:
function extractGitCommitHash(dependencyName: string, dependencyVersion: string): string { const splitHash = dependencyVersion.split('#'); if (!dependencyVersion.startsWith('git+') || splitHash.length !== 2) { throw new Error( `Expected package dependency for ${dependencyName} in format git+<url>#<commit-hash>, but got ${dependencyVersion}`, ); } - return splitHash[1]!; + const hash = splitHash[1]!; + if (!/^[0-9a-fA-F]{40}$/.test(hash)) { + throw new Error( + `Expected a 40-hex commit hash for ${dependencyName}, but got: ${hash}`, + ); + } + return hash; }packages/core/solidity/src/utils/imports-libraries.ts (1)
3-5: Annotate return type and extract the package prefix constantMinor clarity improvements: add an explicit boolean return type and extract the package scope to a constant to avoid string duplication across the codebase and future typos.
-import type { Contract } from '../contract'; +import type { Contract } from '../contract'; + +const COMMUNITY_CONTRACTS_PREFIX = '@openzeppelin/community-contracts/'; -export function importsCommunityContracts(contract: Contract) { - return contract.imports.some(i => i.path.startsWith('@openzeppelin/community-contracts/')); +export function importsCommunityContracts(contract: Contract): boolean { + return contract.imports.some(i => i.path.startsWith(COMMUNITY_CONTRACTS_PREFIX)); }packages/ui/src/solidity/inject-hyperlinks.ts (3)
10-11: Quiet false-positive lint and improve robustness of the commit regex.
- The static-analysis warning about “RegExp from variable” is a false positive here because the source pattern is a static literal. However, prefer
.sourceto avoid the warning and be explicit.- Consider accepting uppercase hex too (harmless and more robust).
Apply this diff:
- const compatibleCommunityContractsRegexSingle = /OpenZeppelin Community Contracts commit ([a-f0-9]{40})/; - const compatibleCommunityContractsRegexGlobal = new RegExp(compatibleCommunityContractsRegexSingle, 'g'); + const compatibleCommunityContractsRegexSingle = /OpenZeppelin Community Contracts commit ([A-Fa-f0-9]{40})/; + const compatibleCommunityContractsRegexGlobal = new RegExp(compatibleCommunityContractsRegexSingle.source, 'gi');
15-18: Potential version mismatch for upgradeable imports; suggest function replacer to select the correct tag.You’re using
contractsVersionfor bothcontractsandcontracts-upgradeable. If their tags ever diverge, upgradeable links will 404. Use a function replacer and, if available in this package, import@openzeppelin/contracts-upgradeable/package.jsonto select the right tag.Example (requires adding the import; if not feasible in this package, please verify versions are always in lockstep):
// At top of file (if dependency is available) import { version as contractsUpgradeableVersion } from '@openzeppelin/contracts-upgradeable/package.json'; // Then, replace usage: result = code.replace( importContractsRegex, (_m, org: string, pkg: 'contracts' | 'contracts-upgradeable', rest: string) => { const tag = pkg === 'contracts-upgradeable' ? contractsUpgradeableVersion : contractsVersion; return `"<a class="import-link" href="https://github.com/OpenZeppelin/openzeppelin-${pkg}/blob/v${tag}/contracts/${rest}" target="_blank" rel="noopener noreferrer">${org}${pkg}/${rest}</a>"`; }, );If you’d rather not add a new dependency, please confirm that the versions (and tags) of
@openzeppelin/contractsand@openzeppelin/contracts-upgradeableare guaranteed to match across releases in the environment where this runs. If not, I can propose an alternative that builds the link against the installed package’spackage.jsonat runtime.
20-30: Consider fallback hyperlinking when no commit line is present.If the commit comment isn’t detected, community-contract imports remain plain text. Optional: fall back to linking to the default branch (e.g., HEAD) to preserve a consistent UX.
Possible fallback:
// else branch after the existing if (...) result = result.replace( importCommunityContractsRegex, `"<a class="import-link" href="https://github.com/OpenZeppelin/openzeppelin-community-contracts/blob/HEAD/contracts/$3" target="_blank" rel="noopener noreferrer">$1$2$3</a>"`, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/core/solidity/openzeppelin-contracts.d.ts(1 hunks)packages/core/solidity/package.json(1 hunks)packages/core/solidity/src/print.ts(3 hunks)packages/core/solidity/src/utils/community-contracts-git-commit.ts(1 hunks)packages/core/solidity/src/utils/imports-libraries.ts(1 hunks)packages/ui/src/solidity/inject-hyperlinks.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/solidity/src/print.ts (2)
packages/core/solidity/src/utils/imports-libraries.ts (1)
importsCommunityContracts(3-5)packages/core/solidity/src/utils/community-contracts-git-commit.ts (1)
getCommunityContractsGitCommit(3-9)
🪛 ast-grep (0.38.6)
packages/ui/src/solidity/inject-hyperlinks.ts
[warning] 10-10: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(compatibleCommunityContractsRegexSingle, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build (solidity, default)
- GitHub Check: check
- GitHub Check: build (stellar, compile)
- GitHub Check: build (stylus, default)
- GitHub Check: build (stellar, default)
- GitHub Check: mcp
- GitHub Check: build (cairo, default)
- GitHub Check: format-lint
- GitHub Check: deno-check
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
packages/core/solidity/package.json (1)
25-25: Pinned commit for community contracts: good for determinismPinning to a specific commit via a git+ URL aligns with the new commit-aware header logic and ensures reproducible output.
packages/core/solidity/src/utils/community-contracts-git-commit.ts (1)
1-1: ✔️ JSON import support and packaging verified
- packages/core/solidity/tsconfig.json extends the root tsconfig.base.json, which has
resolveJsonModule: true.- Although
package.jsonisn’t listed under"files", the npm manifest (package.json) is always included in published packages.No changes are needed.
packages/core/solidity/openzeppelin-contracts.d.ts (1)
2-13: JSDoc additions improve maintainabilityClear docs for version, sources, and dependencies. No type changes; looks good.
packages/core/solidity/src/print.ts (2)
20-21: New utils are integrated in the right placeImports for importsCommunityContracts and getCommunityContractsGitCommit are accurate and scoped to this module.
34-36: Header composition via helper improves readabilityReplacing the hard-coded header with printCompatibleLibraryVersions keeps printContract focused and easier to extend.
|
@SocketSecurity ignore npm/[email protected] |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/ui/src/solidity/inject-hyperlinks.ts (2)
5-6: Fix broken contracts repo URLs: capture repo without trailing slash and add missing slash before "blob".Current replacement yields URLs like https://github.com/OpenZeppelin/openzeppelin-contracts//blob/... due to capturing the trailing slash in group 2 and missing the separator before blob. Also fixes the displayed import path.
Apply this diff:
- const importContractsRegex = - /"(@openzeppelin\/)(contracts-upgradeable\/|contracts\/)((?:(?!\.\.)[^/]+\/)*?[^/]*?)"/g; + const importContractsRegex = + /"(@openzeppelin\/)(contracts-upgradeable|contracts)\/((?:(?!\.\.)[^/]+\/)*?[^/]*?)"/g; let result = code.replace( importContractsRegex, - `"<a class="import-link" href="https://github.com/OpenZeppelin/openzeppelin-$2blob/v${contractsVersion}/contracts/$3" target="_blank" rel="noopener noreferrer">$1$2$3</a>"`, + `"<a class="import-link" href="https://github.com/OpenZeppelin/openzeppelin-$2/blob/v${contractsVersion}/contracts/$3" target="_blank" rel="noopener noreferrer">$1$2/$3</a>"`, );Also applies to: 15-18
23-25: Use "blob" (not "tree") for Community Contracts file links.The file link should use blob//path to render the source file.
- `"<a class="import-link" href="https://github.com/OpenZeppelin/openzeppelin-community-contracts/tree/${compatibleCommunityContractsGitCommit}/contracts/$3" target="_blank" rel="noopener noreferrer">$1$2$3</a>"`, + `"<a class="import-link" href="https://github.com/OpenZeppelin/openzeppelin-community-contracts/blob/${compatibleCommunityContractsGitCommit}/contracts/$3" target="_blank" rel="noopener noreferrer">$1$2$3</a>"`,
🧹 Nitpick comments (2)
packages/ui/src/solidity/inject-hyperlinks.ts (2)
27-29: Optional: Link the commit hash to the commit page (more informative) instead of the repo tree.The current link goes to the repository tree at the commit. If you want users to see the actual commit details, point to /commit/ and adjust the title accordingly.
- `OpenZeppelin Community Contracts commit <a class="import-link" href="https://github.com/OpenZeppelin/openzeppelin-community-contracts/tree/$1" target="_blank" rel="noopener noreferrer" title="View repository at commit $1">$1</a>`, + `OpenZeppelin Community Contracts commit <a class="import-link" href="https://github.com/OpenZeppelin/openzeppelin-community-contracts/commit/$1" target="_blank" rel="noopener noreferrer" title="View commit $1">$1</a>`,
10-12: Avoid constructing RegExp from another RegExp’s source; use literals and allow uppercase hex.This silences the static-analysis warning, simplifies the code, and accepts uppercase A–F which GitHub may display.
- const compatibleCommunityContractsRegexSingle = /OpenZeppelin Community Contracts commit ([a-f0-9]{40})/; - const compatibleCommunityContractsRegexGlobal = new RegExp(compatibleCommunityContractsRegexSingle.source, 'g'); + const compatibleCommunityContractsRegexSingle = /OpenZeppelin Community Contracts commit ([A-Fa-f0-9]{40})/; + const compatibleCommunityContractsRegexGlobal = /OpenZeppelin Community Contracts commit ([A-Fa-f0-9]{40})/g;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/solidity/inject-hyperlinks.ts(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.38.6)
packages/ui/src/solidity/inject-hyperlinks.ts
[warning] 10-10: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(compatibleCommunityContractsRegexSingle.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui/src/solidity/inject-hyperlinks.ts (1)
10-12: Avoid constructing a RegExp from another RegExp’s source (silence false-positive and improve clarity)This isn’t vulnerable (pattern is static), but using a literal is clearer and avoids static-analysis warnings.
Apply this diff:
- const compatibleCommunityContractsRegexGlobal = new RegExp(compatibleCommunityContractsRegexSingle.source, 'g'); + const compatibleCommunityContractsRegexGlobal = /OpenZeppelin Community Contracts commit ([a-f0-9]{40})/g;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/solidity/src/print.ts(3 hunks)packages/ui/src/solidity/inject-hyperlinks.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/solidity/src/print.ts
🧰 Additional context used
🪛 ast-grep (0.38.6)
packages/ui/src/solidity/inject-hyperlinks.ts
[warning] 10-10: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(compatibleCommunityContractsRegexSingle.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Redirect rules - openzeppelin-contracts-wizard
- GitHub Check: Header rules - openzeppelin-contracts-wizard
- GitHub Check: Pages changed - openzeppelin-contracts-wizard
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (7)
packages/ui/src/solidity/inject-hyperlinks.ts (7)
5-8: Regexes for OZ import detection look correctThe patterns correctly:
- detect both contracts and contracts-upgradeable imports,
- avoid paths with ..,
- and capture a stable triplet for replacement.
No issues spotted here.
13-14: Commit extraction is straightforward and safeUsing optional chaining to extract the SHA works well and avoids exceptions when the line is absent.
15-18: Contracts/Upgradeable links are well-formedBuilding:
- openzeppelin-contracts/blob/v/contracts/ and
- openzeppelin-contracts-upgradeable/blob/v/contracts/
is correct for both repos.
27-29: Correct choice of “tree” for the commit-level linkUsing tree/$commit for the “View repository at commit” hyperlink is appropriate since it points to the repo root at that commit (blob would be for files).
32-32: Return value handling is cleanReturning the progressively built result string is clear and maintainable.
23-25: Fix broken Community Contracts file links: duplicated “contracts/” segment$3 already includes “contracts/...”. Adding “/contracts/” in the href creates a path like “.../contracts/contracts/...”, which 404s.
Apply this diff:
- `"<a class="import-link" href="https://github.com/OpenZeppelin/openzeppelin-community-contracts/blob/${compatibleCommunityContractsGitCommit}/contracts/$3" target="_blank" rel="noopener noreferrer">$1$2$3</a>"`, + `"<a class="import-link" href="https://github.com/OpenZeppelin/openzeppelin-community-contracts/blob/${compatibleCommunityContractsGitCommit}/$3" target="_blank" rel="noopener noreferrer">$1$2$3</a>"`,Note: Using blob for file links here is correct (thanks for aligning with prior feedback).
Likely an incorrect or invalid review comment.
20-30: Commit line emission and link injection are properly gated on community-contracts importsThe core printer in
packages/core/solidity/src/print.tsonly callsconst commit = getCommunityContractsGitCommit(); lines.push(`// Compatible with OpenZeppelin Community Contracts commit ${commit}`);inside
if (importsCommunityContracts(contract)) { … }and
getCommunityContractsGitCommit()always returns a string (or logs an error if misconfigured). The UI injector inpackages/ui/src/solidity/inject-hyperlinks.tsthen looks for that same commit marker before applying any hyperlinks.Because both commit-line emission and hyperlink injection share the
importsCommunityContractscheck, any contract importing@openzeppelin/community-contracts/...will always get the commit line — and thus the links — and no-import cases fall back cleanly. No further changes needed.
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/solidity/src/utils/community-contracts-git-commit.ts (1)
11-25: Add unit tests for parsing edge casesConsider covering:
- Valid: git+https://...git#<40-hex>
- Valid: github:OpenZeppelin/community-contracts#<40-hex>
- Invalid: missing “#”, non-hex, short/long hash
This will lock in behavior and prevent regressions if the dependency format changes.
I can draft a test file under packages/core/solidity/test for these scenarios. Want me to push a test scaffold?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/solidity/src/utils/community-contracts-git-commit.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/solidity/src/utils/community-contracts-git-commit.ts (1)
packages/core/stellar/src/contract.ts (1)
Error(19-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (stellar, compile)
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
packages/core/solidity/src/utils/community-contracts-git-commit.ts (2)
3-9: Exceptions fromgetCommunityContractsGitCommitAre Already HandledThe only usage of
getCommunityContractsGitCommit()inprint.ts(lines 62–68) is wrapped in atry/catchthat logs errors, ensuring graceful degradation if the dependency is missing or malformed. No further changes are needed.
1-9: No action needed: named JSON import is supportedThe
packages/core/solidity/tsconfig.jsonextends the roottsconfig.base.json, which has
–"resolveJsonModule": true
–"esModuleInterop": trueand it compiles to CommonJS without a separate bundler. At runtime Node’s
require('../../package.json')returns the JSON object, soimport { devDependencies } from '../../package.json';desugars into
const { devDependencies } = require('../../package.json');which works correctly. You can keep the named import as-is; no change is required here.
packages/core/solidity/src/utils/community-contracts-git-commit.ts
Outdated
Show resolved
Hide resolved
|
/improve |
PR Code Suggestions ✨
|
|||||||||||||
ernestognw
left a comment
ernestognw
left a comment
There was a problem hiding this comment.
Amazing, thanks @ericglau !
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/core/solidity/src/utils/community-contracts-git-commit.ts (1)
15-23: Document the lowercase normalization in JSDocSince extractGitCommitHash returns a lowercased hash, make this explicit in the doc to avoid surprises for callers comparing hashes case-sensitively.
/** * Extracts the git commit hash from a package dependency version string. * The expected format is `git+<url>#<commit-hash>`. * * @param dependencyName The name of the package dependency. * @param dependencyVersion The version string of the package dependency. - * @returns The git commit hash. + * @returns The git commit hash, normalized to lowercase. * @throws Error if the version string or commit hash is not in the expected format. */packages/core/solidity/src/utils/community-contracts-git-commit.test.ts (1)
1-116: Optional: add a test for 40-char uppercase normalizationYou test 7-char uppercase normalization; consider adding a 40-char uppercase case to fully cover the normalization branch for both extremes.
packages/ui/src/solidity/inject-hyperlinks.ts (2)
10-12: Avoid dynamic RegExp construction (minor; readability and static analysis)Static analysis flags this pattern, and it’s easy to avoid by declaring the global variant as a literal. It also reads simpler.
- const compatibleCommunityContractsRegexSingle = /Community Contracts commit ([a-fA-F0-9]{7,40})/; - const compatibleCommunityContractsRegexGlobal = new RegExp(compatibleCommunityContractsRegexSingle.source, 'g'); + const compatibleCommunityContractsRegexSingle = /Community Contracts commit ([a-fA-F0-9]{7,40})/; + const compatibleCommunityContractsRegexGlobal = /Community Contracts commit ([a-fA-F0-9]{7,40})/g;Note: Functionality remains identical; this primarily silences tooling and improves clarity.
3-33: Minor perf/readability: hoist constant regexes out of the functionThese regexes are pure constants. Hoisting them to module scope avoids re-allocating on every call and clarifies intent. Not critical, just a nicety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/core/solidity/src/account.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/stablecoin.test.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/core/solidity/package.json(1 hunks)packages/core/solidity/src/account.test.ts.md(77 hunks)packages/core/solidity/src/print.ts(3 hunks)packages/core/solidity/src/stablecoin.test.ts.md(4 hunks)packages/core/solidity/src/utils/community-contracts-git-commit.test.ts(1 hunks)packages/core/solidity/src/utils/community-contracts-git-commit.ts(1 hunks)packages/ui/src/solidity/inject-hyperlinks.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/core/solidity/src/stablecoin.test.ts.md
- packages/core/solidity/src/account.test.ts.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/solidity/package.json
- packages/core/solidity/src/print.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/solidity/src/utils/community-contracts-git-commit.test.ts (1)
packages/core/solidity/src/utils/community-contracts-git-commit.ts (1)
extractGitCommitHash(24-38)
🪛 ast-grep (0.38.6)
packages/ui/src/solidity/inject-hyperlinks.ts
[warning] 10-10: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(compatibleCommunityContractsRegexSingle.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: check
- GitHub Check: build (stylus, default)
- GitHub Check: build (stellar, default)
- GitHub Check: build (cairo, default)
- GitHub Check: build (stellar, compile)
- GitHub Check: build (solidity, default)
- GitHub Check: mcp
- GitHub Check: format-lint
- GitHub Check: deno-check
- GitHub Check: Redirect rules - openzeppelin-contracts-wizard
- GitHub Check: Header rules - openzeppelin-contracts-wizard
- GitHub Check: Pages changed - openzeppelin-contracts-wizard
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
packages/core/solidity/src/utils/community-contracts-git-commit.ts (2)
7-13: Dependency lookup and error handling look solidReading the pinned commit from devDependencies is straightforward, and the error message is clear for misconfiguration scenarios. No functional concerns here.
24-38: Validation is strict and consistent with tests; good normalizationThe parser enforces
git+and a single#, validates 7–40 hex, and normalizes to lowercase. This matches the test suite and is appropriate given you control the package.json format.packages/core/solidity/src/utils/community-contracts-git-commit.test.ts (4)
7-13: Covers the happy path wellValid 40-char lowercase hash is handled correctly. Good.
15-21: Nice: case normalization behavior explicitly testedVerifying uppercase short hash normalization helps prevent regressions.
25-68: Invalid format coverage is comprehensiveThe three format guards (missing git+, missing #, multiple #) align with the implementation’s error messages.
72-115: Invalid hash content coverage is thoroughLength bounds and non-hex detection are well tested.
packages/ui/src/solidity/inject-hyperlinks.ts (2)
15-18: OZ Contracts links are constructed correctlyThe capture for
contracts{,-upgradeable}/and theblob/v${contractsVersion}/contracts/$3href yield valid file URLs without double slashes. Looks good.
20-30: Correct: use blob for Community Contracts file links and link the commit line
- File imports to Community Contracts correctly use
blob/${commit}.- The commit line linking to
tree/$1matches the intent (“View repository at commit …”).
| @@ -0,0 +1,5 @@ | |||
| import type { Contract } from '../contract'; | |||
|
|
|||
| export function importsCommunityContracts(contract: Contract) { | |||
There was a problem hiding this comment.
nits: as it returns a boolean maybe could be named doesImportsCommunityContracts
There was a problem hiding this comment.
The s in imports implies boolean
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ui/src/common/styles/global.css (1)
149-151: Use design tokens and add hover/focus states for accessibility and consistency.Hard-coding #7d828d bypasses your theme system and may have insufficient contrast in some themes. Prefer CSS vars and provide hover/focus styles so links look clickable.
Apply this diff:
-.comment-link { - color: #7d828d; -} +.comment-link { + color: var(--gray-4); + text-decoration: underline; + text-underline-offset: 2px; +} +.comment-link:hover, +.comment-link:focus-visible { + color: var(--gray-3); +}packages/ui/src/solidity/inject-hyperlinks.ts (1)
10-12: Avoid constructing a RegExp from another at runtime; simplify flags instead.This triggers static analysis warnings even though it’s safe here. Duplicating the pattern with desired flags improves clarity and avoids the false positive.
Apply this diff:
- const compatibleCommunityContractsRegexSingle = /Community Contracts commit ([a-fA-F0-9]{7,40})/; - const compatibleCommunityContractsRegexGlobal = new RegExp(compatibleCommunityContractsRegexSingle.source, 'g'); + const compatibleCommunityContractsRegexSingle = /Community Contracts commit ([a-f0-9]{7,40})/i; + const compatibleCommunityContractsRegexGlobal = /Community Contracts commit ([a-f0-9]{7,40})/gi;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui/src/common/styles/global.css(1 hunks)packages/ui/src/solidity/inject-hyperlinks.ts(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.38.6)
packages/ui/src/solidity/inject-hyperlinks.ts
[warning] 10-10: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(compatibleCommunityContractsRegexSingle.source, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Redirect rules - openzeppelin-contracts-wizard
- GitHub Check: Header rules - openzeppelin-contracts-wizard
- GitHub Check: Pages changed - openzeppelin-contracts-wizard
- GitHub Check: build (stellar, compile)
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
packages/ui/src/solidity/inject-hyperlinks.ts (4)
5-8: Regexes for OZ imports look good.The patterns are scoped and exclude any path with “..”, reducing injection risk in hrefs. Grouping matches the replacement logic.
15-18: Contracts URL generation remains correct.Using the trailing slash in group 2 with “openzeppelin-$2blob” continues to resolve to openzeppelin-contracts/blob and openzeppelin-contracts-upgradeable/blob as intended.
27-29: Correct choice of “tree” for the commit anchor.Linking the commit hash to “tree/” is appropriate for showing the repo at that commit. Keeping file links on “blob/” is also correct.
20-29: No contracts/contracts/ duplication possible – current URL logic is correct.Verified across the repo that every import from
@openzeppelin/community-contracts/…omits a leadingcontracts/segment (no instances of@openzeppelin/community-contracts/contracts/...in source files). The only occurrence of/contracts/lives in the Solidity remappings file, not in code imports. Therefore$3will never start withcontracts/, and the existing unconditionalcontracts/$3prefix will not produce duplicate paths.You can safely ignore the proposed normalization change.
Likely an incorrect or invalid review comment.
3 participants
Fixes #621
Note: This does not include hardhat/forge instructions on how to install a specific commit. It only adds a comment to the contract code, which is not the right context for additional instructions.