Fix: split Type::Null into Null and TypedNull variants

[cds-amal]

Summary

Refactors the Type enum to split untyped and typed nulls into separate variants. This fixes a silent type corruption bug in serde round-trips and enables Strum::Display derives.

Motivation: Broken Round-Trip

Type::try_from is used by serde when types appear as strings in config or manifest files. ABI/IDL codecs (EVM/SVM) construct Types programmatically, serialize them to strings for storage, and later deserialize them back.

This round-trip was silently corrupting types:

// ABI constructs a type
let from_abi = Type::array(Type::String);

// Serialized for storage
let serialized = from_abi.to_string(); // "array[string]"

// Parsed back later
let parsed = Type::try_from(serialized)?;

// BUG: parsed == Type::String, not Type::Array(...)
assert_eq!(from_abi, parsed); // FAILED

This affects any workflow where:

1. An action returns an array type derived from an ABI
2. The type is stored in a manifest or config
3. The type is later parsed for validation or execution

For example, doc/addons/actions.json includes entries like:

{ "typing": "array[buffer]" }

Root Cause

The Type::Null(Option<Box<Type>>) design forced manual to_string() implementation. When Strum was added for other variants, the manual implementation conflicted with Strum's Display trait, causing nested types to serialize incorrectly.

Additional issues:

• Nested null serialization: null<null<string>> became "null<Null>"
• Array parsing returned inner type directly instead of wrapping in Type::Array
• No validation for malformed types like "null<>" or "array[]"

Solution

Split Type::Null(Option<Box<Type>>) into two variants:

#[derive(Clone, Debug, Eq, PartialEq, Hash, StrumDisplay)]
pub enum Type {
    #[strum(serialize = "null")]
    Null,                     // Untyped null

    #[strum(to_string = "null<{0}>")]
    TypedNull(Box<Type>),     // Typed null
    // ...
}

This enables Strum::Display to handle all serialization consistently, eliminating the manual implementation and its bugs.

Behavior Comparison

Input	Before	After
Type::Null	"null"	"null"
Type::typed_null(Type::String)	"null<string>"	"null<string>"
Type::typed_null(Type::typed_null(Type::String))	"null<Null>"	"null<null<string>>"
Parse "array[integer]"	Type::Integer	Type::Array(Box::new(Type::Integer))
Parse "null<>"	Type::typed_null(???)	Error: "empty inner type"
Parse "array[]"	Panic	Error: "empty inner type"

Test Coverage

• Correct nested null serialization
  • Guarded by: test_deep_null_nesting, test_type_null_serialization_format
• Array parsing returns wrapped type
  • Guarded by: test_type_null_parsing, test_deep_array_nesting
• Empty inner types rejected with contextual errors
  • Guarded by: test_empty_inner_type_errors, test_invalid_inner_type_errors
• Full serde round-trip consistency
  • Guarded by: test_type_null_serde_roundtrip, test_cross_nesting

---

Files Changed

File	Description
types/types.rs	Enum split, Strum derives, parser fixes
types/functions.rs	Updated pattern matching
types/type_compatibility.rs	New TypeChecker module
types/mod.rs	Module exports
types/tests/mod.rs	Expanded test coverage

[Copilot]

Pull request overview

This PR refactors the Type enum to split the single Null(Option<Box<Type>>) variant into two separate variants: Null for untyped nulls and TypedNull(Box<Type>) for typed nulls. This change fixes a critical serialization round-trip bug where array types were being corrupted when parsed from strings, and enables the use of Strum's Display derive for consistent type-to-string serialization.

Key Changes:

• Split Type::Null(Option<Box<Type>>) into Type::Null and Type::TypedNull(Box<Type>) variants
• Added Strum Display derive to auto-generate serialization, replacing manual to_string() implementation
• Fixed array parsing bug where "array[integer]" incorrectly returned Type::Integer instead of Type::Array(Box::new(Type::Integer))
• Enhanced parser validation with explicit checks for empty inner types and improved error messages
• Added comprehensive test coverage for nested types, round-trip serialization, and error cases

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
crates/txtx-addon-kit/src/types/types.rs	Core refactoring: split Null variant, added Strum derives, removed manual to_string(), fixed array parsing bug, improved error handling in TryFrom
crates/txtx-addon-kit/src/types/functions.rs	Updated pattern matching to handle both Type::Null and Type::TypedNull in array wildcard checks
crates/txtx-addon-kit/src/types/type_compatibility.rs	New module providing TypeChecker with value and type compatibility checking logic
crates/txtx-addon-kit/src/types/mod.rs	Added export for new type_compatibility module
crates/txtx-addon-kit/src/types/tests/mod.rs	Added 150+ lines of comprehensive tests covering serialization, parsing, nesting, round-trips, and error handling

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The pattern matches both Type::Null and Type::TypedNull(_) as wildcards for "any array", but the comment only mentions "null". This could be confusing as Type::TypedNull(Box::new(Type::String)) would semantically suggest "a null that's specifically a string type", not "any type". Consider whether Type::TypedNull(_) should really act as a wildcard, or if only Type::Null should serve this purpose. If both are intentionally wildcards, update the comment to clarify this behavior.

[cds-amal]

Oof... peviously, null was overloaded to mean both "no type" and "nullable T." Treating these as equivalent caused null to behave as an absorbing value, erasing inner type structure (for example, nested nulls and arrays). Splitting Null from TypedNull(T) restores correct semantics and preserves structure across parsing and serialization.

This PR also surfaces an open design question around TypedNull semantics.

Current behavior

if matches!(**inner, Type::Null | Type::TypedNull(_))

Under this logic, both array[null] and array[null<string>] act as wildcards and accept any array element type.

Observations

In the SVM implementation, TypedNull(T) maps directly to Solana/Anchor’s Option<T>:

// addons/svm/types/src/subgraph/idl.rs
IdlType::Option(idl_type) => Type::typed_null(...)

This suggests null semantically means "string or null", not an unconstrained wildcard.

Open questions

For @lgalabru and @MicaiahReid:

1. Do you expect TypedNull to have different semantics across chains (SVM vs EVM, etc.)?
2. Should array[null] match any array, or only arrays whose element type is compatible with T?
3. Is the current wildcard behavior intentional (for flexibility), or an unintended side effect?

I’m happy to adjust based on guidance here, for example:

• Keep current behavior and document it
• Restrict wildcards to Type::Null only
• Something else...

Let me know which direction you prefer.