feat: refactor agent_skills to ContractAddress-based SkillsComponent#69
feat: refactor agent_skills to ContractAddress-based SkillsComponent#69starknetdev merged 2 commits intonextfrom
Conversation
…killsComponent Replace the agent_skills ByteArray field with a ContractAddress-based extension system mirroring the existing renderer pattern. This enables per-token skills address overrides with fallback to registry-level defaults. - Add IAgentSkills and IMinigameTokenSkills interfaces - Add SkillsComponent extension with per-token storage and fallback - Add OptionalSkills trait with NoOpSkills for minimal contracts - Update GameMetadata.agent_skills (ByteArray) → skills_address (ContractAddress) - Add skills_address parameter to mint/mint_game across all components - Update all mock contracts, test files, and examples - Replace deprecated contract_address_const::<0>() with 0.try_into().unwrap() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=22623795395 attempt=1 sha=c0e0343cdaa948fc81c1b4ae9dae7fe82362d4e2 scope=general Todo List
Review FindingsAfter analyzing the PR changes with the scoped diff command: git diff 00499e839d2cac339a071dd201aaaae998835f66...c0e0343cdaa948fc81c1b4ae9dae7fe82362d4e2 -- . ':(exclude)packages/**'No issues found. All 65 files changed in this PR are located within the
Since the scope of this general engineering review is limited to changes outside Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=22623795395 attempt=1 sha=c0e0343cdaa948fc81c1b4ae9dae7fe82362d4e2 scope=packages Review Tasks
Security Analysis[MEDIUM] packages/embeddable_game_standard/src/token/extensions/skills/skills.cairo:52-64 - Missing token existence validation fn reset_token_skills(ref self: ComponentState<TContractState>, token_id: felt252) {
let contract_address = get_contract_address();
let erc721 = IERC721Dispatcher { contract_address };
// First check token exists, then check ownership
let token_owner = erc721.owner_of(token_id.into()); // This will revert for non-existent tokens
let caller = get_caller_address();
assert!(token_owner == caller, "MinigameToken: Caller is not owner of token");
// ... rest of function
}[LOW] packages/embeddable_game_standard/src/token/extensions/skills/skills.cairo:66-78 - Unbounded loop in batch operation fn reset_token_skills_batch(ref self: ComponentState<TContractState>, token_ids: Span<felt252>) {
assert!(token_ids.len() <= 100, "MinigameToken: Batch size too large");
// ... rest of function
}[INFO] packages/embeddable_game_standard/src/token/token_component.cairo:178-206 - Skills address resolution logic is sound Cairo Idioms[INFO] All
[INFO] Component integration follows established patterns
Testing Coverage[HIGH] packages/embeddable_game_standard/src/token/extensions/skills/skills.cairo - Missing comprehensive test coverage
[MEDIUM] Test files updated but missing negative path coverage
Gas Optimization[LOW] packages/embeddable_game_standard/src/token/extensions/skills/skills.cairo:80-97 - Batch function performs redundant storage reads fn get_skills_address_batch(self: @ComponentState<TContractState>, token_ids: Span<felt252>) -> Array<ContractAddress> {
let mut results = array![];
let mut index = 0;
loop {
if index >= token_ids.len() { break; }
let token_id = *token_ids.at(index);
let addr = self.token_skills.entry(token_id).read(); // Direct storage read
results.append(addr);
index += 1;
}
results
}[INFO] Storage packing in token_id remains efficient
Additional Findings[INFO] Interface design follows SRC5 standards correctly
[INFO] Event emission properly implemented
[LOW] packages/interfaces/src/structs/registry.cairo - Skills address type consistency Summary3 MEDIUM, 3 LOW, 6 INFO findings identified. The refactor is architecturally sound and follows established patterns. Main concerns are missing test coverage for the new component and standard batch operation safety validations. |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the agent skills implementation to use a contract address-based approach, enhancing flexibility and extensibility. It introduces a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring, replacing the agent_skills: ByteArray with a more scalable skills_address: ContractAddress. The introduction of a new SkillsComponent follows the existing architectural patterns, which is excellent for consistency and maintainability. The changes are applied thoroughly across all relevant files, including contracts, interfaces, and tests. The addition of the OptionalSkills trait and NoOpSkills implementation for minimal contracts is a thoughtful touch that enhances the modularity of the standard. Overall, this is a high-quality contribution that improves the design of the codebase. I have one minor suggestion regarding some misplaced comments in the test files to improve readability.
| Option::Some("https://game.com"), // client_url | ||
| Option::Some(RENDERER_ADDRESS()), // renderer_address | ||
| Option::Some(RENDERER_ADDRESS()), | ||
| Option::None, // renderer_address |
There was a problem hiding this comment.
The comment for this parameter is incorrect. It should be // skills_address instead of // renderer_address. This seems to be a recurring pattern in other test files as well where the new skills_address parameter was added. Please correct these comments for better code clarity.
Option::None, // skills_address
- Create comprehensive test_skills.cairo with 25 tests covering: get/set, has_custom, reset (owner-only), batch operations, events, SRC5 interface, integration, and fuzz tests - Add skills methods to IMinigameTokenMixin for test accessibility - Fix 15 misplaced "// renderer_address" comments that should be "// skills_address" across test files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=22624892104 attempt=1 sha=b48bb5e3af6407888693ed207655dca5218d2c0a scope=general Todo List
Review FindingsAfter analyzing the PR changes with the scoped diff command: git diff 00499e839d2cac339a071dd201aaaae998835f66...b48bb5e3af6407888693ed207655dca5218d2c0a -- . ':(exclude)packages/**'No issues found. All 65 files changed in this PR are located within the
Since the scope of this general engineering review is limited to changes outside Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=22624892104 attempt=1 sha=b48bb5e3af6407888693ed207655dca5218d2c0a scope=packages Review Tasks
Security Analysis[MEDIUM] packages/embeddable_game_standard/src/token/extensions/skills/skills.cairo:52-64 - Missing token existence validation fn reset_token_skills(ref self: ComponentState<TContractState>, token_id: felt252) {
let contract_address = get_contract_address();
let erc721 = IERC721Dispatcher { contract_address };
// First check token exists, then check ownership
let token_owner = erc721.owner_of(token_id.into()); // This will revert for non-existent tokens
let caller = get_caller_address();
assert!(token_owner == caller, "MinigameToken: Caller is not owner of token");
// ... rest of function
}[LOW] packages/embeddable_game_standard/src/token/extensions/skills/skills.cairo:66-78 - Unbounded loop in batch operation fn reset_token_skills_batch(ref self: ComponentState<TContractState>, token_ids: Span<felt252>) {
assert!(token_ids.len() <= 100, "MinigameToken: Batch size too large");
// ... rest of function
}[INFO] packages/embeddable_game_standard/src/token/token_component.cairo:178-206 - Skills address resolution logic is sound Cairo Idioms[INFO] All
[INFO] Component integration follows established patterns
Testing Coverage[HIGH] packages/embeddable_game_standard/src/token/extensions/skills/skills.cairo - Missing comprehensive test coverage
[MEDIUM] Test files updated but missing negative path coverage
Gas Optimization[LOW] packages/embeddable_game_standard/src/token/extensions/skills/skills.cairo:80-97 - Batch function performs redundant storage reads fn get_skills_address_batch(self: @ComponentState<TContractState>, token_ids: Span<felt252>) -> Array<ContractAddress> {
let mut results = array![];
let mut index = 0;
loop {
if index >= token_ids.len() { break; }
let token_id = *token_ids.at(index);
let addr = self.token_skills.entry(token_id).read(); // Direct storage read
results.append(addr);
index += 1;
}
results
}[INFO] Storage packing in token_id remains efficient
Additional Findings[INFO] Interface design follows SRC5 standards correctly
[INFO] Event emission properly implemented
[LOW] packages/interfaces/src/structs/registry.cairo - Skills address type consistency Summary3 MEDIUM, 3 LOW, 6 INFO findings identified. The refactor is architecturally sound and follows established patterns. Main concerns are missing test coverage for the new component and standard batch operation safety validations. |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
2 participants
Summary
agent_skills: ByteArraywithskills_address: ContractAddressacrossGameMetadata, registry, and all mint functionsSkillsComponentextension (mirrors the renderer pattern) with per-token storage and registry fallbackIAgentSkillsandIMinigameTokenSkillsSRC5 interfaces for external skills contractsOptionalSkillstrait withNoOpSkillsfor minimal contractscontract_address_const::<0>()with0.try_into().unwrap()Test plan
snforge test)🤖 Generated with Claude Code