nfts: lock_item_properties emits misleading events when no state changes occur

[Maksandre]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Description of bug

If an item's metadata or attributes are "locked", they cannot be "unlocked" later:

polkadot-sdk/substrate/frame/nfts/src/lib.rs

Line 1379 in 607a1b2

/// When the metadata or attributes are locked, it won't be possible the unlock them.

.

However, the ItemPropertiesLocked event is emitted with parameters that mirror the input values rather than the actual state changes.

When executing lock_item_properties on an item with already locked metadata or attributes, and setting lock_metadata or lock_attributes to false:

1. The locked values stay locked, which is correct
2. The transaction succeeds, which is controversial
3. The ItemPropertiesLocked event indicates an unlocked state, which is misleading

Steps to reproduce

1. Create a collection with default settings
2. Create an item for a collection
3. Execute lockItemProperties - set true for lockMetadata and lockAttributes
4. Execute lockItemProperties - set false for lockMetadata and lockAttributes

Check that the ItemPropertiesLocked event is emitted with lockMetadata: false and lockAttributes: false

[Pray4Love1]

ChatGPT said:

this is a runtime design flaw in event emission logic not in state transition logic. the state correctly preserves locked status on already-locked properties. however the event emitted reflects user input not actual state mutation. this is misleading and violates substrate event emission principles where events should reflect what happened not what was requested.

the cause is clear in the current substrate nfts pallet.

lock_item_properties does not short-circuit event emission on no-op
it emits ItemPropertiesLocked event with parameters as passed by the caller even if state was unchanged.

the fix is mechanical

in frame/nfts/src/lib.rs lock_item_properties function
before emitting the ItemPropertiesLocked event, read the actual post-operation state and emit that instead of caller input.

existing incorrect behavior (simplified)

Self::deposit_event(Event::ItemPropertiesLocked {
collection,
item,
locked_metadata: lock_metadata,
locked_attributes: lock_attributes,
});

proposed corrected behavior

let actual_locked_metadata = item_info.locked_metadata;
let actual_locked_attributes = item_info.locked_attributes;

Self::deposit_event(Event::ItemPropertiesLocked {
collection,
item,
locked_metadata: actual_locked_metadata,
locked_attributes: actual_locked_attributes,
});

this ensures the event reflects reality not input.

optionally if no state change occurs emit no event. this aligns with general substrate practice to avoid misleading events on noop extrinsics.

patch logic example

if lock_metadata == item_info.locked_metadata && lock_attributes == item_info.locked_attributes {
return Ok(().into());
}

Self::deposit_event(Event::ItemPropertiesLocked {
collection,
item,
locked_metadata: item_info.locked_metadata,
locked_attributes: item_info.locked_attributes,
});

this avoids both user confusion and tooling misinterpretation.