fix: Settings Address Zero Check Fix#5
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request adds a new test module for token URI behavior when settings_address is zero, and updates the full token contract to handle this case gracefully by using default settings and avoiding unnecessary syscalls when the address is zero. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant FullTokenContract
participant SettingsContract
Note over FullTokenContract: New Logic (with zero-address guard)
Caller->>FullTokenContract: Call token_uri()
FullTokenContract->>FullTokenContract: Check if settings_address == ZERO_ADDRESS
alt settings_address is zero
FullTokenContract->>FullTokenContract: Use default_settings
Note over FullTokenContract: Return token_uri with default settings
else settings_address is non-zero
FullTokenContract->>SettingsContract: Call settings_details()
alt Call succeeds
SettingsContract-->>FullTokenContract: Return GameSettingDetails
else Call fails
FullTokenContract->>FullTokenContract: Use default_settings as fallback
Note over FullTokenContract: Consistent error handling
end
FullTokenContract->>FullTokenContract: Return token_uri with retrieved/fallback settings
end
FullTokenContract-->>Caller: token_uri
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Summary of ChangesHello @FemiOje, 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 enhances the robustness of the Highlights
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
|
![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 effectively addresses a panic caused by a zero settings_address in token_uri. The fix, which involves adding a pre-check for the zero address, is correct and well-implemented. The inclusion of a new, focused test case is excellent as it ensures the bug is resolved and prevents regressions. I have one suggestion to refactor a small portion of the code to reduce duplication and improve maintainability.
| // Check if settings_address is zero before attempting contract call | ||
| let settings_details = if settings_address.is_zero() { | ||
| GameSettingDetails { name: "", description: "", settings: array![].span() } | ||
| } else { | ||
| let mut settings_calldata = array![]; | ||
| settings_calldata.append(token_metadata.settings_id.into()); | ||
|
|
||
| let settings_details = | ||
| match call_contract_syscall( | ||
| settings_address, settings_details_selector, settings_calldata.span(), | ||
| ) { | ||
| Result::Ok(result) => { | ||
| // Try to deserialize the result as GameSettingDetails | ||
| let mut result_span = result; | ||
| match Serde::<GameSettingDetails>::deserialize(ref result_span) { | ||
| Option::Some(settings_details) => settings_details, | ||
| Option::None => GameSettingDetails { | ||
| name: "", description: "", settings: array![].span(), | ||
| }, | ||
| } | ||
| }, | ||
| Result::Err(_) => GameSettingDetails { | ||
| name: "", description: "", settings: array![].span(), | ||
| }, | ||
| Result::Ok(result) => { | ||
| // Try to deserialize the result as GameSettingDetails | ||
| let mut result_span = result; | ||
| match Serde::<GameSettingDetails>::deserialize(ref result_span) { | ||
| Option::Some(settings_details) => settings_details, | ||
| Option::None => GameSettingDetails { | ||
| name: "", description: "", settings: array![].span(), | ||
| }, | ||
| } | ||
| }, | ||
| Result::Err(_) => GameSettingDetails { | ||
| name: "", description: "", settings: array![].span(), | ||
| }, | ||
| } | ||
| }; |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, you can define a closure to create the default GameSettingDetails. This avoids repeating the struct instantiation in multiple places.
// Check if settings_address is zero before attempting contract call
let get_default_settings = || GameSettingDetails { name: "", description: "", settings: array![].span() };
let settings_details = if settings_address.is_zero() {
get_default_settings()
} else {
let mut settings_calldata = array![];
settings_calldata.append(token_metadata.settings_id.into());
match call_contract_syscall(
settings_address, settings_details_selector, settings_calldata.span(),
) {
Result::Ok(result) => {
// Try to deserialize the result as GameSettingDetails
let mut result_span = result;
match Serde::<GameSettingDetails>::deserialize(ref result_span) {
Option::Some(settings_details) => settings_details,
Option::None => get_default_settings(),
}
},
Result::Err(_) => get_default_settings(),
}
};
![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
🧹 Nitpick comments (1)
packages/test_starknet/src/token/test_token_uri_settings_address.cairo (1)
22-90: Well-structured test for the zero-address fix.The test correctly verifies that
token_uri()no longer panics whensettings_addressis zero and returns a non-empty URI. The test setup properly initializes the game withZERO_ADDRESS()for settings (line 55) and exercises the fixed code path.Consider adding an assertion to verify that the returned URI contains expected default content (e.g., checking for presence of game name or specific metadata structure), rather than just checking non-empty. This would provide stronger validation that the default settings are being used correctly.
For example:
// Verify that token_uri was generated successfully (non-empty) - // The exact content depends on the implementation, but it should not be empty assert!(token_uri.len() > 0, "Token URI should not be empty"); + // Optionally verify it contains expected game metadata + assert!(token_uri.contains("TestGame"), "Token URI should contain game name");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/test_starknet/src/token.cairo(1 hunks)packages/test_starknet/src/token/test_token_uri_settings_address.cairo(1 hunks)packages/token/src/examples/full_token_contract.cairo(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/*/src/**/*.cairo
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/*/src/**/*.cairo: Verify correct usage of component architecture with #[starknet::component] in Cairo code
Check SRC5 interface discovery implementation for capability detection in Cairo contracts
Ensure proper storage isolation using #[substorage(v0)] in Cairo contracts
Validate dispatcher pattern usage for cross-contract calls in Cairo code
Confirm extensive use of Option types for optional parameters in Cairo contracts
Verify interface IDs are defined as constants (e.g., IMINIGAME_ID) in Cairo contracts
Every function must include clear explanation of what it does and why, parameter descriptions with types and constraints, return value documentation, and example usage when appropriate
Use descriptive variable names (e.g., liquidity_unlock_timestamp not t)
Contracts must follow this organization: 1. Interfaces 2. Events 3. Storage 4. Constructor 5. External functions 6. Internal functions
All error messages must be implemented as descriptive constants
Events must be emitted for all state changes in Cairo contracts
Review for proper implementation of optional extensions: settings, objectives, context, minter, renderer
Ensure game lifecycle is implemented: Setup → Mint → Play → Sync → Complete
Token metadata structure and state tracking must be properly implemented
Review for proper usage of account abstraction features in Starknet contracts
Review for proper usage of L1-L2 messaging patterns in Starknet contracts
Review for proper usage of STARK-friendly cryptography in contracts
Files:
packages/token/src/examples/full_token_contract.cairopackages/test_starknet/src/token/test_token_uri_settings_address.cairopackages/test_starknet/src/token.cairo
🧠 Learnings (12)
📚 Learning: 2025-08-01T09:49:40.817Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-01T09:49:40.817Z
Learning: Applies to packages/*/src/**/minigame_token*.cairo : Implement IMinigameTokenData trait with score() and game_over() functions in relevant contracts
Applied to files:
packages/token/src/examples/full_token_contract.cairopackages/test_starknet/src/token/test_token_uri_settings_address.cairopackages/test_starknet/src/token.cairo
📚 Learning: 2025-08-01T09:50:10.672Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-01T09:50:10.672Z
Learning: Applies to src/**/*.cairo : Use OpenZeppelin contracts for token and introspection functionality.
Applied to files:
packages/token/src/examples/full_token_contract.cairo
📚 Learning: 2025-08-01T09:50:10.672Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-01T09:50:10.672Z
Learning: Applies to src/**/*.cairo : Use Option types for optional parameters in contracts.
Applied to files:
packages/token/src/examples/full_token_contract.cairo
📚 Learning: 2025-08-01T09:50:10.672Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-01T09:50:10.672Z
Learning: Applies to src/**/*.cairo : Use component architecture with #[starknet::component] in Cairo contracts.
Applied to files:
packages/token/src/examples/full_token_contract.cairopackages/test_starknet/src/token.cairo
📚 Learning: 2025-08-01T09:50:10.672Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-01T09:50:10.672Z
Learning: Applies to tests/integration/**/*.cairo : Add integration tests for cross-contract interactions.
Applied to files:
packages/test_starknet/src/token/test_token_uri_settings_address.cairopackages/test_starknet/src/token.cairo
📚 Learning: 2025-08-01T09:49:40.817Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-01T09:49:40.817Z
Learning: Applies to packages/*/src/**/*.cairo : Verify correct usage of component architecture with #[starknet::component] in Cairo code
Applied to files:
packages/test_starknet/src/token.cairo
📚 Learning: 2025-08-01T09:49:40.817Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-01T09:49:40.817Z
Learning: Applies to packages/*/src/**/*.cairo : Review for proper usage of account abstraction features in Starknet contracts
Applied to files:
packages/test_starknet/src/token.cairo
📚 Learning: 2025-08-01T09:49:40.817Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-01T09:49:40.817Z
Learning: Applies to packages/*/src/**/*.cairo : Ensure proper storage isolation using #[substorage(v0)] in Cairo contracts
Applied to files:
packages/test_starknet/src/token.cairo
📚 Learning: 2025-08-01T09:49:40.817Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-01T09:49:40.817Z
Learning: Applies to packages/*/src/**/*.cairo : Token metadata structure and state tracking must be properly implemented
Applied to files:
packages/test_starknet/src/token.cairo
📚 Learning: 2025-08-01T09:49:40.817Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-01T09:49:40.817Z
Learning: Applies to packages/*/src/**/*.cairo : Review for proper usage of STARK-friendly cryptography in contracts
Applied to files:
packages/test_starknet/src/token.cairo
📚 Learning: 2025-08-01T09:50:10.672Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-01T09:50:10.672Z
Learning: Applies to src/**/*.cairo : Use StarkNet 2.10.1 for all StarkNet-specific code.
Applied to files:
packages/test_starknet/src/token.cairo
📚 Learning: 2025-08-01T09:50:10.672Z
Learnt from: CR
Repo: Provable-Games/game-components PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-01T09:50:10.672Z
Learning: Applies to src/**/*.cairo : Use storage isolation via #[substorage(v0)] in contracts.
Applied to files:
packages/test_starknet/src/token.cairo
🔇 Additional comments (2)
packages/test_starknet/src/token.cairo (1)
24-25: LGTM!The new test module is properly integrated following the existing pattern with appropriate
#[cfg(test)]guards.packages/token/src/examples/full_token_contract.cairo (1)
5-9: LGTM!Import reorganization properly supports the zero-address check implementation by adding necessary types (
GameSettingDetails,GameContextDetails, etc.) and syscall support.Also applies to: 15-17, 21-24, 30-30
| // Check if settings_address is zero before attempting contract call | ||
| let mut default_settings = GameSettingDetails { | ||
| name: "", description: "", settings: array![].span(), | ||
| }; | ||
|
|
||
| let settings_details = if settings_address.is_zero() { | ||
| default_settings | ||
| } else { | ||
| let mut settings_calldata = array![]; | ||
| settings_calldata.append(token_metadata.settings_id.into()); | ||
|
|
||
| let settings_details = | ||
| match call_contract_syscall( | ||
| settings_address, settings_details_selector, settings_calldata.span(), | ||
| ) { | ||
| Result::Ok(result) => { | ||
| // Try to deserialize the result as GameSettingDetails | ||
| let mut result_span = result; | ||
| match Serde::<GameSettingDetails>::deserialize(ref result_span) { | ||
| Option::Some(settings_details) => settings_details, | ||
| Option::None => GameSettingDetails { | ||
| name: "", description: "", settings: array![].span(), | ||
| }, | ||
| } | ||
| }, | ||
| Result::Err(_) => GameSettingDetails { | ||
| name: "", description: "", settings: array![].span(), | ||
| }, | ||
| Result::Ok(result) => { | ||
| // Try to deserialize the result as GameSettingDetails | ||
| let mut result_span = result; | ||
| match Serde::<GameSettingDetails>::deserialize(ref result_span) { | ||
| Option::Some(settings_details) => settings_details, | ||
| Option::None => default_settings, | ||
| } | ||
| }, | ||
| Result::Err(_) => default_settings, | ||
| } | ||
| }; |
There was a problem hiding this comment.
Good fix with minor refinement needed.
The zero-address check correctly prevents panics by avoiding unnecessary syscalls when settings_address is zero. The code properly reuses default_settings for all fallback scenarios, which addresses the previous review comment about duplication.
Issue: Remove unnecessary mut keyword on line 292:
- let mut default_settings = GameSettingDetails {
+ let default_settings = GameSettingDetails {
name: "", description: "", settings: array![].span(),
};The variable is never mutated, so mut is not needed.
Optional refinement: Consider adding a brief inline comment explaining the zero-address check for future maintainers:
// Check if settings_address is zero before attempting contract call
+ // to avoid panics on uninitialized/optional settings contracts
let default_settings = GameSettingDetails {🤖 Prompt for AI Agents
In packages/token/src/examples/full_token_contract.cairo around lines 291 to
315, the variable declaration "let mut default_settings = ..." unnecessarily
uses the mut keyword even though the variable is never mutated; remove the mut
to make it "let default_settings = ...". Also optionally add a brief inline
comment above the zero-address check explaining that it avoids a syscall when
settings_address is zero for future maintainers.
1 participant
Fix: Prevent panic when
settings_addressis zero intoken_uriSummary
This PR fixes a bug in
FullTokenContract.token_uri()where the function would panic when attempting to call a contract at a zerosettings_address. The fix adds a zero-check before making the contract call, preventing panics and returning default settings details instead.Problem
When
token_uri()is called on a token associated with a game that has a zerosettings_address, the function would attempt to call a contract at address0x0, causing a panic with the error:This occurred because the code would call
call_contract_syscall()with a zero address without checking if the address was valid first.Solution
Added a zero-check using
settings_address.is_zero()before attempting to call the settings contract. Whensettings_addressis zero, the function now returns a defaultGameSettingDetailsstruct instead of attempting the contract call.Changes
packages/token/src/examples/full_token_contract.cairo:call_contract_syscallforsettings_addressGameSettingDetailswhensettings_addressis zeropackages/test_starknet/src/token/test_token_uri_settings_address.cairo:test_token_uri_with_zero_settings_addresstoken_uri()does not panic whensettings_addressis zeropackages/test_starknet/src/token.cairo:Code Changes
Before (Buggy Code)
After (Fixed Code)
Testing
test_token_uri_with_zero_settings_addresspassestoken_uri()returns a valid token URI (non-empty) whensettings_addressis zerotoken_uri()with zerosettings_addressChecklist
Summary by CodeRabbit
Bug Fixes
Tests