Cairo: Fix missing use clause in hook for ERC20 votes#637
Cairo: Fix missing use clause in hook for ERC20 votes#637ericglau merged 6 commits intoOpenZeppelin:masterfrom
Conversation
WalkthroughAdds missing StarkNet ContractAddress use/import clauses in ERC20 hook generation for pausable and votes paths in Cairo and Cairo Alpha, updates corresponding test snapshots to include the import, and adds a changeset to bump @openzeppelin/wizard-cairo with a patch note. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
User descriptionFixes compile issue identified in #623 (comment) PR TypeBug fix Description
Diagram Walkthroughflowchart LR
A["ERC20 Hook Functions"] --> B["Add ContractAddress Import"]
B --> C["Fix Compilation Errors"]
C --> D["Update Test Snapshots"]
|
| Relevant files | |||||
|---|---|---|---|---|---|
| Bug fix |
| ||||
| Documentation |
| ||||
| Tests |
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.changeset/old-corners-scream.md (1)
5-5: Nit: Reflect that pausable hooks also gained the missing importThe PR also adds the ContractAddress use clause for the pausable hook path. Consider clarifying the changeset description to avoid confusion.
Apply this diff to make the note more complete:
-Fix missing use clause in hooks for ERC20 votes +Fix missing use clause in hooks for ERC20 votes (and pausable) hookspackages/core/cairo/src/erc20.ts (1)
101-177: Optional: Add the ContractAddress use clause once per hooks block to avoid duplicationMinor simplification: since both hook variants use ContractAddress in their signatures, import it once after adding the hooks trait instead of in each branch. The builder likely dedupes, but this reduces repetition and intent is clearer.
Apply this refactor:
function addHooks(c: ContractBuilder, allOpts: Required<ERC20Options>) { const usesCustomHooks = allOpts.pausable || allOpts.votes; if (usesCustomHooks) { const hooksTrait = { name: 'ERC20HooksImpl', of: 'ERC20Component::ERC20HooksTrait<ContractState>', tags: [], priority: 1, }; c.addImplementedTrait(hooksTrait); + // ContractAddress is required for hook arguments in all custom hook paths + c.addUseClause('starknet', 'ContractAddress'); if (allOpts.pausable) { - c.addUseClause('starknet', 'ContractAddress'); const beforeUpdateFn = c.addFunction(hooksTrait, { name: 'before_update', args: [ { name: 'ref self', type: 'ERC20Component::ComponentState<ContractState>', }, { name: 'from', type: 'ContractAddress' }, { name: 'recipient', type: 'ContractAddress' }, { name: 'amount', type: 'u256' }, ], code: [], }); beforeUpdateFn.code.push( 'let contract_state = self.get_contract();', 'contract_state.pausable.assert_not_paused();', ); } if (allOpts.votes) { if (!allOpts.appName) { throw new OptionsError({ appName: 'Application Name is required when Votes are enabled', }); } if (!allOpts.appVersion) { throw new OptionsError({ appVersion: 'Application Version is required when Votes are enabled', }); } addVotesComponent( c, toFelt252(allOpts.appName, 'appName'), toFelt252(allOpts.appVersion, 'appVersion'), 'SNIP12 Metadata', ); - c.addUseClause('starknet', 'ContractAddress'); const afterUpdateFn = c.addFunction(hooksTrait, { name: 'after_update', args: [ { name: 'ref self', type: 'ERC20Component::ComponentState<ContractState>', }, { name: 'from', type: 'ContractAddress' }, { name: 'recipient', type: 'ContractAddress' }, { name: 'amount', type: 'u256' }, ], code: [], }); afterUpdateFn.code.push( 'let mut contract_state = self.get_contract_mut();', 'contract_state.votes.transfer_voting_units(from, recipient, amount);', ); } } else { c.addUseClause('openzeppelin::token::erc20', 'ERC20HooksEmptyImpl'); } }packages/core/cairo_alpha/src/erc20.ts (1)
101-177: Optional: Import ContractAddress once per hooks blockMirror the small dedup from the Cairo stable file here to keep both generators consistent and slightly cleaner.
Apply this refactor:
function addHooks(c: ContractBuilder, allOpts: Required<ERC20Options>) { const usesCustomHooks = allOpts.pausable || allOpts.votes; if (usesCustomHooks) { const hooksTrait = { name: 'ERC20HooksImpl', of: 'ERC20Component::ERC20HooksTrait<ContractState>', tags: [], priority: 1, }; c.addImplementedTrait(hooksTrait); + c.addUseClause('starknet', 'ContractAddress'); if (allOpts.pausable) { - c.addUseClause('starknet', 'ContractAddress'); const beforeUpdateFn = c.addFunction(hooksTrait, { name: 'before_update', args: [ { name: 'ref self', type: 'ERC20Component::ComponentState<ContractState>', }, { name: 'from', type: 'ContractAddress' }, { name: 'recipient', type: 'ContractAddress' }, { name: 'amount', type: 'u256' }, ], code: [], }); beforeUpdateFn.code.push( 'let contract_state = self.get_contract();', 'contract_state.pausable.assert_not_paused();', ); } if (allOpts.votes) { if (!allOpts.appName) { throw new OptionsError({ appName: 'Application Name is required when Votes are enabled', }); } if (!allOpts.appVersion) { throw new OptionsError({ appVersion: 'Application Version is required when Votes are enabled', }); } addVotesComponent( c, toFelt252(allOpts.appName, 'appName'), toFelt252(allOpts.appVersion, 'appVersion'), 'SNIP12 Metadata', ); - c.addUseClause('starknet', 'ContractAddress'); const afterUpdateFn = c.addFunction(hooksTrait, { name: 'after_update', args: [ { name: 'ref self', type: 'ERC20Component::ComponentState<ContractState>', }, { name: 'from', type: 'ContractAddress' }, { name: 'recipient', type: 'ContractAddress' }, { name: 'amount', type: 'u256' }, ], code: [], }); afterUpdateFn.code.push( 'let mut contract_state = self.get_contract_mut();', 'contract_state.votes.transfer_voting_units(from, recipient, amount);', ); } } else { c.addUseClause('openzeppelin::token::erc20', 'ERC20HooksEmptyImpl'); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
packages/core/cairo/src/erc20.test.ts.snapis excluded by!**/*.snappackages/core/cairo_alpha/src/erc20.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
.changeset/old-corners-scream.md(1 hunks)packages/core/cairo/src/erc20.test.ts.md(1 hunks)packages/core/cairo/src/erc20.ts(2 hunks)packages/core/cairo_alpha/src/erc20.test.ts.md(1 hunks)packages/core/cairo_alpha/src/erc20.ts(2 hunks)
⏰ 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 (solidity, default)
- GitHub Check: validate-cairo-alpha
- GitHub Check: validate-cairo
🔇 Additional comments (4)
packages/core/cairo/src/erc20.test.ts.md (1)
1114-1114: LGTM: Added ContractAddress import in votes non-upgradeable snapshotThis aligns the snapshot with the generated hooks requiring ContractAddress and fixes the compile issue.
packages/core/cairo_alpha/src/erc20.test.ts.md (1)
1117-1117: LGTM: Added ContractAddress import in votes non-upgradeable snapshot (alpha)Snapshot now correctly imports ContractAddress for hook argument types.
packages/core/cairo/src/erc20.ts (1)
113-113: LGTM: Importing ContractAddress fixes the hook compile errorAdding c.addUseClause('starknet', 'ContractAddress') before defining before_update/after_update ensures the hook arg types resolve in both pausable and votes flows. Matches the updated snapshots and addresses the reported issue.
Also applies to: 154-154
packages/core/cairo_alpha/src/erc20.ts (1)
113-113: LGTM: Alpha generator now imports ContractAddress for hooksSame fix as Cairo stable: ensures hook arg types resolve in both pausable and votes paths.
Also applies to: 154-154
CoveMB
left a comment
CoveMB
left a comment
There was a problem hiding this comment.
LGTM would compilation tests have catch that?
Yes, it already tests some compilation but missed this combination. Due to compilation times, it's infeasible to test every combination for Cairo. |
4 participants
Fixes compile issue identified in #623 (comment)