[N-01] M3 Audit - #1731
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
![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 (3)
packages/macros/src/attribute/with_components/parser.rs (3)
118-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated alias-resolution closure.
Lines 121-129 and 157-165 contain the same logic: take the last path segment as a local name, then find an import whose
local_namematches and whosesource_pathends with the target name. A small helper removes the duplication and keeps the two matchers in sync.♻️ Proposed helper
impl ModuleFacts { fn resolves_to(&self, local_name: &str, target: &str) -> bool { self.imports.iter().any(|import| { import.local_name == local_name && import .source_path .last() .is_some_and(|segment| segment == target) }) } }Then use
path.last().is_some_and(|last| last == name || self.resolves_to(last, name))in bothimplements_traitandhas_impl_available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/macros/src/attribute/with_components/parser.rs` around lines 118 - 167, Extract the duplicated import-alias matching logic from implements_trait and has_impl_available into a ModuleFacts helper such as resolves_to(local_name, target). Update both path matchers to use the helper while preserving their existing direct last-segment checks and matching behavior.
243-249: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGlob imports are dropped and can cause a spurious warning.
ast::UsePath::Staris ignored. If a user writesuse openzeppelin_token::erc20::*;and relies on that glob to bringERC20HooksEmptyImplinto scope,has_impl_availablereturnsfalseand the macro emitsERC20_HOOKS_IMPL_MISSING. The previous substring scan had the same gap only when the short name never appeared in the source, so this is a narrow case. If you want to close it, record the glob prefix and treat a matching parent path as satisfying the impl check.♻️ Optional: record glob prefixes
- ast::UsePath::Star(_) => {} + ast::UsePath::Star(_) => { + // Record the glob prefix so impl-availability checks can honour `use path::*;`. + imports.push(ImportedName { + source_path: prefix.to_vec(), + local_name: String::new(), + }); + }This requires a dedicated
glob_prefixesfield rather than reusingimports, so thatimports_nameand alias resolution are not affected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/macros/src/attribute/with_components/parser.rs` around lines 243 - 249, Update the import collection flow around collect_imports so ast::UsePath::Star records its parent path in a dedicated glob_prefixes collection. Thread that collection through the impl-availability check, and treat a matching parent path as satisfying has_impl_available without adding glob entries to imports or changing imports_name and alias resolution.
530-547: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive component paths from
component_info.Use
component_info.storageand construct internal implementation aliases fromcomponent_info.short_name()andcomponent_info.internal_impls. The current literals duplicateComponentInfometadata and can drift, causing incorrect warnings. Also use suffix-based matching consistently so aliased receivers such ass.pausable.pause()are detected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/macros/src/attribute/with_components/parser.rs` around lines 530 - 547, Update the AllowedComponents::Initializable and AllowedComponents::Pausable checks to derive component paths from component_info.storage, component_info.short_name(), and component_info.internal_impls instead of hardcoded literals. Build the internal implementation aliases from that metadata and use suffix-based call matching so aliased receivers such as s.pausable.pause() are recognized. Preserve the existing warnings when the required initialize, pause, or unpause calls are absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/macros/src/attribute/with_components/parser.rs`:
- Around line 494-496: The ImmutableConfig trait check can match a sibling
component because it uses a parent-path prefix without verifying the component
name. In packages/macros/src/attribute/with_components/parser.rs:494-496, append
component.name to component_parent_segments before calling
implements_imported_trait_from, and update that method to require an exact
path-length match. In
packages/macros/src/tests/test_with_components.rs:2408-2432, add a negative test
covering ERC721EnumerableComponent::ImmutableConfig when ERC721Consecutive is
declared, asserting IMMUTABLE_CONFIG_MISSING is produced.
- Around line 252-295: Update collect_call_paths to recognize turbofish generic
arguments while scanning backward from each call parenthesis. When encountering
the closing angle bracket of a balanced <...> group, skip the entire group
before continuing through identifiers and dot/colon-colon separators, preserving
the full paths for calls such as after_update::<T> and
ERC1155SupplyInternalImpl::<ContractState>::after_update.
In `@packages/macros/src/tests/test_with_components.rs`:
- Around line 2214-2240: Update both test fixtures in
packages/macros/src/tests/test_with_components.rs at lines 2214-2240 and
2340-2360 to construct the Cairo items from raw source using indoc!, then
convert that source into a TokenStream that preserves comments. Keep the
existing validation assertions and fixture behavior unchanged; both sites
require this conversion so comment exclusion is actually tested.
---
Nitpick comments:
In `@packages/macros/src/attribute/with_components/parser.rs`:
- Around line 118-167: Extract the duplicated import-alias matching logic from
implements_trait and has_impl_available into a ModuleFacts helper such as
resolves_to(local_name, target). Update both path matchers to use the helper
while preserving their existing direct last-segment checks and matching
behavior.
- Around line 243-249: Update the import collection flow around collect_imports
so ast::UsePath::Star records its parent path in a dedicated glob_prefixes
collection. Thread that collection through the impl-availability check, and
treat a matching parent path as satisfying has_impl_available without adding
glob entries to imports or changing imports_name and alias resolution.
- Around line 530-547: Update the AllowedComponents::Initializable and
AllowedComponents::Pausable checks to derive component paths from
component_info.storage, component_info.short_name(), and
component_info.internal_impls instead of hardcoded literals. Build the internal
implementation aliases from that metadata and use suffix-based call matching so
aliased receivers such as s.pausable.pause() are recognized. Preserve the
existing warnings when the required initialize, pause, or unpause calls are
absent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0202e76e-3195-42f6-835c-551380e73d6b
⛔ Files ignored due to path filters (1)
packages/macros/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/macros/Cargo.tomlpackages/macros/src/attribute/with_components/parser.rspackages/macros/src/tests/test_with_components.rs
💤 Files with no reviewable changes (1)
- packages/macros/Cargo.toml
| fn collect_call_paths(db: &dyn SyntaxGroup, node: SyntaxNode<'_>) -> Vec<Vec<String>> { | ||
| let terminals = node | ||
| .tokens(db) | ||
| .map(|terminal| (terminal.kind(db), terminal_text(db, terminal))) | ||
| .collect::<Vec<_>>(); | ||
| let mut calls = vec![]; | ||
|
|
||
| for (lparen_index, (kind, _)) in terminals.iter().enumerate() { | ||
| if *kind != SyntaxKind::TerminalLParen { | ||
| continue; | ||
| } | ||
|
|
||
| let mut cursor = lparen_index; | ||
| let mut reversed_path = vec![]; | ||
| loop { | ||
| if cursor == 0 { | ||
| break; | ||
| } | ||
| cursor -= 1; | ||
| let (kind, text) = &terminals[cursor]; | ||
| if *kind != SyntaxKind::TerminalIdentifier { | ||
| break; | ||
| } | ||
| reversed_path.push(text.clone()); | ||
|
|
||
| if cursor == 0 | ||
| || !matches!( | ||
| terminals[cursor - 1].0, | ||
| SyntaxKind::TerminalDot | SyntaxKind::TerminalColonColon | ||
| ) | ||
| { | ||
| break; | ||
| } | ||
| cursor -= 1; | ||
| } | ||
|
|
||
| if !reversed_path.is_empty() { | ||
| reversed_path.reverse(); | ||
| calls.push(reversed_path); | ||
| } | ||
| } | ||
|
|
||
| calls | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find turbofish-style calls in Cairo sources.
rg -nP --glob '*.cairo' '::<[^>]+>\s*\(' | head -50Repository: OpenZeppelin/cairo-contracts
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -i 'parser.rs|\.cairo$' packages/macros/src packages 2>/dev/null | head -120
printf '%s\n' '--- collect_call_paths context ---'
sed -n '220,320p' packages/macros/src/attribute/with_components/parser.rs
printf '%s\n' '--- call-path consumers ---'
rg -n -C 4 'collect_call_paths|has_constructor_call|has_call|ERC1155_SUPPLY_HOOKS_MISSING' packages/macros/src packages 2>/dev/null | head -240
printf '%s\n' '--- generic-call syntax in tracked sources ---'
rg -n -P --glob '*.cairo' '::\s*<|::<|>[[:space:]]*\(' . | head -200Repository: OpenZeppelin/cairo-contracts
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- parser structure ---'
ast-grep outline packages/macros/src/attribute/with_components/parser.rs 2>/dev/null | head -160
printf '%s\n' '--- all relevant call-path references ---'
rg -n -C 8 'collect_call_paths|constructor_calls|ERC1155_SUPPLY_HOOKS_MISSING|has_constructor_call|has_call' packages/macros/src/attribute/with_components/parser.rs packages/macros/src/attribute/with_components 2>/dev/null
printf '%s\n' '--- tracked Cairo files containing generic syntax ---'
git ls-files '*.cairo' | while IFS= read -r f; do
rg -n -H -P '::\s*<|::<|>[[:space:]]*\(' "$f" || true
done | head -240Repository: OpenZeppelin/cairo-contracts
Length of output: 50384
🌐 Web query:
Cairo language generic function call turbofish syntax ::<T>
💡 Result:
In the Cairo programming language, the turbofish syntax (::) is used to explicitly specify generic type parameters when calling a generic function or accessing a generic item within an expression [1][2]. This syntax is necessary because, in many expression contexts, the compiler cannot automatically infer the generic types, or the syntax might otherwise be ambiguous (such as the < symbol potentially being mistaken for a less-than operator) [1][3]. The syntax is formally documented in the Cairo book under operators and symbols [1][2]. Example usage: SomeFunction::(arg); EnumName::::Variant; This behavior is similar to Rust, where the turbofish is required to disambiguate and explicitly provide types in expression paths [1][3].
Citations:
- 1: https://book.cairo-lang.org/appendix-02-operators-and-symbols.html
- 2: https://www.starknet.io/cairo-book/appendix-02-operators-and-symbols.html
- 3: dev: generics type syntax contains turbofish starkware-libs/cairo#1672
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant hook and initializer calls ---'
rg -n -C 3 -P 'after_update|initializer|erc1155_supply|ERC1155SupplyInternalImpl' \
packages/token packages/presets packages/macros/src/tests/test_with_components.rs \
--glob '*.cairo' --glob '*.rs' | head -320
printf '%s\n' '--- direct turbofish calls in tracked Cairo files ---'
git ls-files '*.cairo' | while IFS= read -r f; do
rg -n -H -P '(^|[^[:alnum:]_])([[:alnum:]_:]+)::[^;\n()<>]+>\s*\(' "$f" || true
done | head -160
printf '%s\n' '--- parser tests around call recognition ---'
sed -n '2290,2420p' packages/macros/src/tests/test_with_components.rs
printf '%s\n' '--- path matching implementation ---'
sed -n '165,195p' packages/macros/src/attribute/with_components/parser.rsRepository: OpenZeppelin/cairo-contracts
Length of output: 29477
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
def collect_call_paths(terminals):
calls = []
for lparen_index, (kind, _) in enumerate(terminals):
if kind != "LParen":
continue
cursor = lparen_index
reversed_path = []
while True:
if cursor == 0:
break
cursor -= 1
kind, text = terminals[cursor]
if kind != "Identifier":
break
reversed_path.append(text)
if cursor == 0 or terminals[cursor - 1][0] not in {"Dot", "ColonColon"}:
break
cursor -= 1
if reversed_path:
calls.append(list(reversed(reversed_path)))
return calls
def has_suffix(path, suffix):
return len(path) >= len(suffix) and path[-len(suffix):] == suffix
cases = {
"plain qualified hook": [
("Identifier", "ERC1155SupplyInternalImpl"),
("ColonColon", "::"),
("Identifier", "after_update"),
("LParen", "("),
],
"generic qualified hook": [
("Identifier", "ERC1155SupplyInternalImpl"),
("ColonColon", "::"),
("LT", "<"),
("Identifier", "ContractState"),
("GT", ">"),
("ColonColon", "::"),
("Identifier", "after_update"),
("LParen", "("),
],
"generic initializer": [
("Identifier", "self"),
("Dot", "."),
("Identifier", "erc20"),
("Dot", "."),
("Identifier", "initializer"),
("ColonColon", "::"),
("LT", "<"),
("Identifier", "T"),
("GT", ">"),
("LParen", "("),
],
"direct generic call": [
("Identifier", "process_proof"),
("ColonColon", "::"),
("LT", "<"),
("Identifier", "Hasher"),
("GT", ">"),
("LParen", "("),
],
}
for name, terminals in cases.items():
paths = collect_call_paths(terminals)
print(name, "=>", paths)
if "hook" in name:
print(" matches qualified hook:",
any(has_suffix(path, ["ERC1155SupplyInternalImpl", "after_update"])
for path in paths))
if "initializer" in name:
print(" matches initializer:",
any(has_suffix(path, ["self", "erc20", "initializer"])
for path in paths))
PYRepository: OpenZeppelin/cairo-contracts
Length of output: 427
Handle turbofish arguments in collect_call_paths.
collect_call_paths skips after_update::<T>(...) and reduces ERC1155SupplyInternalImpl::<ContractState>::after_update(...) to ["after_update"]. Neither matches the required suffix, so the macro emits ERC1155_SUPPLY_HOOKS_MISSING. The same issue affects self.erc20.initializer::<T>(...). Skip the balanced <...> group during the backward scan.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/macros/src/attribute/with_components/parser.rs` around lines 252 -
295, Update collect_call_paths to recognize turbofish generic arguments while
scanning backward from each call parenthesis. When encountering the closing
angle bracket of a balanced <...> group, skip the entire group before continuing
through identifiers and dot/colon-colon separators, preserving the full paths
for calls such as after_update::<T> and
ERC1155SupplyInternalImpl::<ContractState>::after_update.
There was a problem hiding this comment.
explain why after_update is skipped
There was a problem hiding this comment.
@immrsd, collect_call_paths scans backward from each ( token. It only continues through identifier, . and :: tokens.
For this call:
after_update::<T>(...)The token directly before ( is >. The scanner expects an identifier, so it stops before it collects after_update. The call path is empty.
For this call:
ERC1155SupplyInternalImpl::<ContractState>::after_update(...)The scanner first collects after_update. It then crosses :: and reaches >. It stops there, so the collected path is only:
["after_update"]
The required suffix is:
["ERC1155SupplyInternalImpl", "after_update"]
The suffix check therefore fails and emits ERC1155_SUPPLY_HOOKS_MISSING.
The same token sequence causes initializer detection to fail for calls such as self.erc20.initializer::<T>(...). The scanner must skip the balanced <...> group before it continues toward the preceding path segments.
🐇🔍
🧠 Learnings used
Learnt from: immrsd
Repo: OpenZeppelin/cairo-contracts PR: 1660
File: packages/macros/src/attribute/with_components/parser.rs:153-160
Timestamp: 2026-03-10T04:00:20.726Z
Learning: In the Cairo contract macros module, detection of component usage and related constructs should rely on simple substring checks using contains (e.g., body_code.contains(...) or code.contains(...)). Do not replace these with regex or AST-based matching; this approach is an established convention across the module and should be preserved for consistency and maintainability.
You are interacting with an AI system.
There was a problem hiding this comment.
Addressed. collect_call_paths now skips balanced turbofish arguments while scanning backward, including nested generic arguments. This preserves the complete call path for both qualified hook calls and initializer calls.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
The refactor looks really good! Agree with 2 issues raised by @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
🐇✅ You are interacting with an AI system. |
* [N-01] M3 Audit (#1731) * fix: N-01 * feat: apply review updates (cherry picked from commit 13b7454) * fix: L-04 (#1732) (cherry picked from commit fc158d4) * fix: L-01 (#1733) (cherry picked from commit bd4e0e2) * [L-03] M3 Audit (#1734) * fix: L-04 * fix: L-01 * fix: L-03 * feat: apply review updates * fix: L-03 * feat: apply auditors feedback (cherry picked from commit 62e17d4) * fix: L-02 (#1743) (cherry picked from commit edd76a9) * [L-06] M3 Audit (#1736) * fix: L-06 * feat: apply review updates (cherry picked from commit 9a47516) * fix: L-07 (#1737) (cherry picked from commit 23da4ee) * fix: L-05 (#1738) (cherry picked from commit 00ec4c3) * fix: L-11 (#1739) (cherry picked from commit 9ba97df) * fix: L-08 (#1740) (cherry picked from commit 7b911da) * fix: L-09 (#1741) (cherry picked from commit 6bc3911) * [N-03] M3 Audit (#1744) * fix: N-03 * feat: apply review updates * feat: apply auditors review comments (cherry picked from commit 37ba01b) * feat: update CHANGELOG
2 participants
Replaces text-based component checks with syntax-aware validation and adds regression coverage for imports, aliases, initializers, and hooks.