fix(minibf): Stricter metadata standard definition for /assets#882
fix(minibf): Stricter metadata standard definition for /assets#882
Conversation
📝 WalkthroughWalkthroughThis change introduces CIP-25 metadata validation functionality by creating a new validation module in the Cardano crate and integrating it into asset routes to validate metadata before mapping to on-chain standards. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/cardano/src/cip68.rs`:
- Around line 259-266: The helper cip25_is_string_array currently returns true
for empty JsonValue::Array because Iterator::all is true on empty iterators;
update cip25_is_string_array to reject empty arrays by checking that the array
is non-empty (e.g., !items.is_empty()) in addition to verifying every element is
a JsonValue::String, so callers (fields like image/src) won’t accept an empty
array.
- Around line 211-236: Add unit tests exercising cip25_metadata_is_valid and
cip25_files_are_valid (and helper behavior via inputs) covering: a V1 case where
all known keys are plain strings (expect true), a V2 case allowing string arrays
for keys like "image"/"description" (expect true), a case missing required keys
(expect false), a case with wrong types for a known key (expect false), and both
valid and invalid "files" nested structures (expect true/false accordingly). Put
tests in the same crate test module (using Cip25MetadataVersion::V1/V2 and
constructing HashMap<String, JsonValue> inputs), assert boolean results from
cip25_metadata_is_valid, and add focused unit tests calling
cip25_files_are_valid for edge cases; then run cargo test --workspace
--all-features to verify.
- Around line 10-15: CIP25_FILES_REQUIRED_KEYS currently omits "name" but per
CIP-25 the files entries must require name, mediaType, and src; update the
CIP25_FILES_REQUIRED_KEYS constant to include "name" along with "mediaType" and
"src" (the constant named CIP25_FILES_REQUIRED_KEYS should list "name",
"mediaType", "src") so it matches the CDDL schema and aligns with the existing
CIP25_FILES_STRING_KEYS reference.
🧹 Nitpick comments (2)
crates/cardano/src/cip68.rs (1)
245-253:cip25_key_in_listis a trivial wrapper aroundcontains.This adds an indirection without real benefit. You could inline
list.contains(&key)at call sites, or if you prefer the abstraction, a brief doc comment explaining why would help. This is a minor stylistic point.crates/minibf/src/routes/assets.rs (1)
130-142: Logic is correct — clean two-step version gating.The progression from raw version number →
Cip25MetadataVersion→ validatedOnchainMetadataStandard(orNone) is sound. Defaulting toV1when the version field is absent or not2aligns with CIP-25 semantics.One minor readability note:
versionis shadowed three times in this function (lines 119, 130, 135) with three different types (Option<i64>,Cip25MetadataVersion,Option<OnchainMetadataStandard>). This is idiomatic Rust but may trip up future readers. Consider using distinct names (e.g.,cip25_versionandmetadata_standard) if you feel it helps clarity.
crates/cardano/src/cip68.rs
Outdated
| pub fn cip25_metadata_is_valid( | ||
| metadata: &HashMap<String, JsonValue>, | ||
| version: Cip25MetadataVersion, | ||
| ) -> bool { | ||
| if !cip25_required_keys_present(metadata) { | ||
| return false; | ||
| } | ||
|
|
||
| let allow_string_array = matches!(version, Cip25MetadataVersion::V2); | ||
|
|
||
| for (key, value) in metadata.iter() { | ||
| if cip25_key_in_list(CIP25_STRING_KEYS, key) { | ||
| if !cip25_is_string(value) { | ||
| return false; | ||
| } | ||
| } else if cip25_key_in_list(CIP25_STRING_OR_STRING_ARRAY_KEYS, key) { | ||
| if !cip25_is_string_or_string_array(value, allow_string_array) { | ||
| return false; | ||
| } | ||
| } else if key == "files" && !cip25_files_are_valid(value, allow_string_array) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validation logic looks correct but lacks unit test coverage.
The function correctly enforces required keys, type constraints per version, and delegates to cip25_files_are_valid for nested structure validation. However, there are no tests for any of the new validation functions (cip25_metadata_is_valid, cip25_files_are_valid, helpers). Given the centrality of this validation to the PR's purpose (stricter metadata), please add tests covering at least:
- V1 valid metadata (string values only)
- V2 valid metadata (string arrays for
image/description) - Missing required keys →
false - Wrong type for a known key →
false - Valid and invalid
filesstructures
As per coding guidelines, "Run cargo test --workspace --all-features to verify functionality of all changes".
🤖 Prompt for AI Agents
In `@crates/cardano/src/cip68.rs` around lines 211 - 236, Add unit tests
exercising cip25_metadata_is_valid and cip25_files_are_valid (and helper
behavior via inputs) covering: a V1 case where all known keys are plain strings
(expect true), a V2 case allowing string arrays for keys like
"image"/"description" (expect true), a case missing required keys (expect
false), a case with wrong types for a known key (expect false), and both valid
and invalid "files" nested structures (expect true/false accordingly). Put tests
in the same crate test module (using Cip25MetadataVersion::V1/V2 and
constructing HashMap<String, JsonValue> inputs), assert boolean results from
cip25_metadata_is_valid, and add focused unit tests calling
cip25_files_are_valid for edge cases; then run cargo test --workspace
--all-features to verify.
crates/cardano/src/cip68.rs
Outdated
| fn cip25_is_string_array(value: &JsonValue) -> bool { | ||
| match value { | ||
| JsonValue::Array(items) => items | ||
| .iter() | ||
| .all(|item| matches!(item, JsonValue::String(_))), | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
cip25_is_string_array accepts empty arrays.
An empty JsonValue::Array(vec![]) would pass cip25_is_string_array (since all on an empty iterator returns true). Depending on the spec intent, an empty array for image or src is likely not meaningful. Consider whether this should be rejected.
🤖 Prompt for AI Agents
In `@crates/cardano/src/cip68.rs` around lines 259 - 266, The helper
cip25_is_string_array currently returns true for empty JsonValue::Array because
Iterator::all is true on empty iterators; update cip25_is_string_array to reject
empty arrays by checking that the array is non-empty (e.g., !items.is_empty())
in addition to verifying every element is a JsonValue::String, so callers
(fields like image/src) won’t accept an empty array.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/cardano/src/cip25.rs`:
- Around line 59-66: The function cip25_is_string_array currently returns true
for empty arrays due to iter().all vacuity; update cip25_is_string_array to
require the JsonValue::Array to be non-empty and that every element is a
JsonValue::String, i.e., check items.is_empty() == false (or items.len() > 0) in
addition to the existing all(...) predicate so empty arrays (which would be
invalid for required keys like "image") fail validation.
- Around line 1-103: The PR adds non-trivial validation logic but lacks unit
tests; add a new test module exercising cip25_metadata_is_valid and
cip25_files_are_valid (and helper behavior via cip25_is_string_or_string_array)
with cases for: valid V1 and V2 metadata, missing required keys, wrong types
(e.g., numeric name), V1 rejecting arrays for "image"/"description" while V2
accepts them, files validation (valid file entries, missing required file keys,
wrong types), and edge cases (empty files array, empty string arrays); organize
tests using serde_json::json! to build HashMap<String, JsonValue>, assert
true/false for each case, and run cargo test --workspace --all-features to
verify.
| use std::collections::HashMap; | ||
|
|
||
| use serde_json::Value as JsonValue; | ||
|
|
||
| const CIP25_REQUIRED_KEYS: &[&str] = &["name", "image"]; | ||
| const CIP25_STRING_KEYS: &[&str] = &["name", "mediaType"]; | ||
| const CIP25_STRING_OR_STRING_ARRAY_KEYS: &[&str] = &["image", "description"]; | ||
| const CIP25_FILES_REQUIRED_KEYS: &[&str] = &["name", "mediaType", "src"]; | ||
| const CIP25_FILES_STRING_KEYS: &[&str] = &["name", "mediaType"]; | ||
| const CIP25_FILES_STRING_OR_STRING_ARRAY_KEYS: &[&str] = &["src"]; | ||
|
|
||
| #[derive(Debug, Clone, Copy)] | ||
| pub enum Cip25MetadataVersion { | ||
| V1, | ||
| V2, | ||
| } | ||
|
|
||
| pub fn cip25_metadata_is_valid( | ||
| metadata: &HashMap<String, JsonValue>, | ||
| version: Cip25MetadataVersion, | ||
| ) -> bool { | ||
| if !cip25_required_keys_present(metadata) { | ||
| return false; | ||
| } | ||
|
|
||
| let allow_string_array = matches!(version, Cip25MetadataVersion::V2); | ||
|
|
||
| for (key, value) in metadata.iter() { | ||
| if cip25_key_in_list(CIP25_STRING_KEYS, key) { | ||
| if !cip25_is_string(value) { | ||
| return false; | ||
| } | ||
| } else if cip25_key_in_list(CIP25_STRING_OR_STRING_ARRAY_KEYS, key) { | ||
| if !cip25_is_string_or_string_array(value, allow_string_array) { | ||
| return false; | ||
| } | ||
| } else if key == "files" && !cip25_files_are_valid(value, allow_string_array) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| true | ||
| } | ||
|
|
||
| fn cip25_required_keys_present(metadata: &HashMap<String, JsonValue>) -> bool { | ||
| CIP25_REQUIRED_KEYS | ||
| .iter() | ||
| .all(|key| metadata.contains_key(*key)) | ||
| } | ||
|
|
||
| fn cip25_key_in_list(list: &[&str], key: &str) -> bool { | ||
| list.contains(&key) | ||
| } | ||
|
|
||
| fn cip25_is_string(value: &JsonValue) -> bool { | ||
| matches!(value, JsonValue::String(_)) | ||
| } | ||
|
|
||
| fn cip25_is_string_array(value: &JsonValue) -> bool { | ||
| match value { | ||
| JsonValue::Array(items) => items | ||
| .iter() | ||
| .all(|item| matches!(item, JsonValue::String(_))), | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| fn cip25_is_string_or_string_array(value: &JsonValue, allow_array: bool) -> bool { | ||
| cip25_is_string(value) || (allow_array && cip25_is_string_array(value)) | ||
| } | ||
|
|
||
| fn cip25_files_are_valid(value: &JsonValue, allow_array: bool) -> bool { | ||
| let JsonValue::Array(items) = value else { | ||
| return false; | ||
| }; | ||
|
|
||
| for item in items.iter() { | ||
| let JsonValue::Object(map) = item else { | ||
| return false; | ||
| }; | ||
|
|
||
| if !CIP25_FILES_REQUIRED_KEYS | ||
| .iter() | ||
| .all(|key| map.contains_key(*key)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| for (key, value) in map.iter() { | ||
| if cip25_key_in_list(CIP25_FILES_STRING_KEYS, key) { | ||
| if !cip25_is_string(value) { | ||
| return false; | ||
| } | ||
| } else if cip25_key_in_list(CIP25_FILES_STRING_OR_STRING_ARRAY_KEYS, key) | ||
| && !cip25_is_string_or_string_array(value, allow_array) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
No unit tests for the new validation module.
This module introduces non-trivial validation logic (version differentiation, files sub-structure, type checks) that would benefit from direct unit tests covering:
- Valid V1/V2 metadata
- Missing required keys
- Wrong types (e.g., number for
name) - V1 rejecting arrays for
image/description - V2 accepting arrays
filesvalidation (valid, missing required file keys, wrong types)- Edge cases: empty
filesarray, empty string arrays
As per coding guidelines, "Run cargo test --workspace --all-features to verify functionality of all changes."
Would you like me to generate a test suite for this module?
🤖 Prompt for AI Agents
In `@crates/cardano/src/cip25.rs` around lines 1 - 103, The PR adds non-trivial
validation logic but lacks unit tests; add a new test module exercising
cip25_metadata_is_valid and cip25_files_are_valid (and helper behavior via
cip25_is_string_or_string_array) with cases for: valid V1 and V2 metadata,
missing required keys, wrong types (e.g., numeric name), V1 rejecting arrays for
"image"/"description" while V2 accepts them, files validation (valid file
entries, missing required file keys, wrong types), and edge cases (empty files
array, empty string arrays); organize tests using serde_json::json! to build
HashMap<String, JsonValue>, assert true/false for each case, and run cargo test
--workspace --all-features to verify.
| fn cip25_is_string_array(value: &JsonValue) -> bool { | ||
| match value { | ||
| JsonValue::Array(items) => items | ||
| .iter() | ||
| .all(|item| matches!(item, JsonValue::String(_))), | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty arrays pass validation — verify this is intentional per CIP-25.
cip25_is_string_array returns true for JsonValue::Array(vec![]) because iter().all(...) is vacuously true. For "image" (a required key), an empty array would be semantically invalid (no URI). Consider adding a non-empty check.
Proposed fix
fn cip25_is_string_array(value: &JsonValue) -> bool {
match value {
- JsonValue::Array(items) => items
+ JsonValue::Array(items) => !items.is_empty() && items
.iter()
.all(|item| matches!(item, JsonValue::String(_))),
_ => false,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn cip25_is_string_array(value: &JsonValue) -> bool { | |
| match value { | |
| JsonValue::Array(items) => items | |
| .iter() | |
| .all(|item| matches!(item, JsonValue::String(_))), | |
| _ => false, | |
| } | |
| } | |
| fn cip25_is_string_array(value: &JsonValue) -> bool { | |
| match value { | |
| JsonValue::Array(items) => !items.is_empty() && items | |
| .iter() | |
| .all(|item| matches!(item, JsonValue::String(_))), | |
| _ => false, | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@crates/cardano/src/cip25.rs` around lines 59 - 66, The function
cip25_is_string_array currently returns true for empty arrays due to iter().all
vacuity; update cip25_is_string_array to require the JsonValue::Array to be
non-empty and that every element is a JsonValue::String, i.e., check
items.is_empty() == false (or items.len() > 0) in addition to the existing
all(...) predicate so empty arrays (which would be invalid for required keys
like "image") fail validation.
2 participants
Summary by CodeRabbit
Release Notes