Skip to content

fix: Settings Address Zero Check Fix #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
FemiOje wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: Settings Address Zero Check Fix #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33130b5
fix: Settings Address Zero Check Fix
FemiOje Nov 2, 2025
3794406
refactor: Simplify default settings handling for zero address case
FemiOje Nov 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/test_starknet/src/token.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,6 @@ pub mod test_context_coverage;
pub mod test_examples_coverage;
#[cfg(test)]
pub mod test_core_token_coverage;
#[cfg(test)]
pub mod test_token_uri_settings_address;

90 changes: 90 additions & 0 deletions packages/test_starknet/src/token/test_token_uri_settings_address.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
use game_components_test_starknet::minigame::mocks::minigame_starknet_mock::{
IMinigameStarknetMockDispatcherTrait, IMinigameStarknetMockInitDispatcherTrait,
};
use game_components_token::interface::IMinigameTokenMixinDispatcherTrait;
use openzeppelin_token::erc721::interface::{
IERC721MetadataDispatcher, IERC721MetadataDispatcherTrait,
};

// setup helpers
use super::setup::{
ALICE, OWNER, ZERO_ADDRESS, deploy_full_token_contract, deploy_minigame_registry_contract,
deploy_mock_game,
};

// ================================================================================================
// TOKEN_URI TESTS - Settings Address Zero Check Fix
// ================================================================================================

/// Test that token_uri works correctly when settings_address is zero (the fix)
/// This test verifies the bug fix where we check if settings_address is zero
/// before attempting a contract call, preventing panics
#[test]
fn test_token_uri_with_zero_settings_address() {
let registry = deploy_minigame_registry_contract();

// Deploy mock game and initialize it with ZERO_ADDRESS for settings_address
let (minigame_dispatcher, minigame_init_dispatcher, minigame_mock_dispatcher) =
deploy_mock_game();

// Deploy token contract with registry first (needed for game initialization)
let (token_dispatcher, _erc721_dispatcher, _, token_address) = deploy_full_token_contract(
Option::Some("TestToken"),
Option::Some("TT"),
Option::Some("https://test.com/"),
Option::None, // royalty_receiver
Option::Some(0), // royalty_fraction
Option::Some(registry.contract_address),
Option::None // event_relayer_address
);

// Initialize the mock game with zero settings_address to simulate the bug scenario
// game will auto-register during initialization
minigame_init_dispatcher
.initializer(
OWNER(), // game_creator
"TestGame", // game_name
"A test game", // game_description
"Dev", // game_developer
"Pub", // game_publisher
"Action", // game_genre
"", // game_image
Option::None, // game_color
Option::None, // client_url
Option::None, // renderer_address (not needed for this test)
Option::Some(ZERO_ADDRESS()), // settings_address - zero address
Option::None, // objectives_address
token_address // minigame_token_address
);

// Mint a token with the game
let token_id = token_dispatcher
.mint(
Option::Some(minigame_dispatcher.contract_address),
Option::None, // player_name
Option::None, // settings_id
Option::None, // start
Option::None, // end
Option::None, // objective_ids
Option::None, // context
Option::None, // client_url
Option::None, // renderer_address
ALICE(),
false // soulbound
);

// Set score for the token using end_game
minigame_mock_dispatcher.end_game(token_id, 100);

let erc721_metadata = IERC721MetadataDispatcher {
contract_address: token_dispatcher.contract_address,
};

// Before the fix, this would attempt to call a contract at zero address and panic
// After the fix, it should return default settings_details without panicking
let token_uri = erc721_metadata.token_uri(token_id.into());

// 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");
}
68 changes: 35 additions & 33 deletions packages/token/src/examples/full_token_contract.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,34 +2,32 @@
// This demonstrates how to configure and use the modular components

use core::num::traits::Zero;
use starknet::{ContractAddress, syscalls::call_contract_syscall};
use starknet::storage::{StoragePointerReadAccess};
use game_components_metagame::extensions::context::structs::GameContextDetails;
use game_components_minigame::extensions::settings::structs::GameSettingDetails;
use game_components_minigame::interface::{IMinigameDispatcher, IMinigameDispatcherTrait};
use game_components_minigame::structs::GameDetail;
use game_components_utils::renderer::{create_custom_metadata, create_default_svg};
use openzeppelin_introspection::src5::SRC5Component;
use openzeppelin_token::common::erc2981::erc2981::{DefaultConfig, ERC2981Component};

// Core imports
use openzeppelin_token::erc721::{ERC721Component, interface::IERC721Metadata};
use openzeppelin_introspection::src5::SRC5Component;
use openzeppelin_token::common::erc2981::erc2981::{DefaultConfig, ERC2981Component};
use starknet::ContractAddress;
use starknet::storage::StoragePointerReadAccess;
use starknet::syscalls::call_contract_syscall;

// Game components imports
use crate::core::core_token::CoreTokenComponent;
use crate::structs::TokenMetadata;
use crate::examples::minigame_registry_contract::{
IMinigameRegistryDispatcher, IMinigameRegistryDispatcherTrait,
};
use crate::extensions::context::context::ContextComponent;
use crate::extensions::minter::minter::MinterComponent;
use crate::extensions::objectives::objectives::ObjectivesComponent;
use crate::extensions::context::context::ContextComponent;
use crate::extensions::renderer::renderer::RendererComponent;
use crate::extensions::settings::settings::SettingsComponent;

use crate::examples::minigame_registry_contract::{
IMinigameRegistryDispatcher, IMinigameRegistryDispatcherTrait,
};

use crate::interface::{ITokenEventRelayerDispatcher, ITokenEventRelayerDispatcherTrait};

use game_components_metagame::extensions::context::structs::GameContextDetails;
use game_components_minigame::extensions::settings::structs::GameSettingDetails;
use game_components_minigame::interface::{IMinigameDispatcher, IMinigameDispatcherTrait};
use game_components_minigame::structs::GameDetail;
use game_components_utils::renderer::{create_default_svg, create_custom_metadata};
use crate::structs::TokenMetadata;


#[starknet::contract]
Expand Down Expand Up @@ -290,26 +288,30 @@ pub mod FullTokenContract {
Result::Err(_) => array![].span(),
};

let mut settings_calldata = array![];
settings_calldata.append(token_metadata.settings_id.into());
// 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,
}
};
Comment on lines +291 to 315
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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(),
                    }
                };

Comment on lines +291 to 315
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


let minted_by_address = self.minter.get_minter_address(token_metadata.minted_by);
Expand Down