Test the Holding parser

[gyorgybalazsi]

What this PR does

Old behavior

token::holding::Holding parses a utility-registry holding contract out of the
ledger's JSON. It had no test. cbtc-lib carried a copy of the same struct and
also had no test, so the parser was untested in both repositories.

New behavior

The parser has 14 unit tests. Nothing about the parser itself changes, so no
caller sees a difference.

Verification

• cargo test --workspace — 184 passed, 0 failed, 39 ignored. The 39 are the
  live devnet tests, which need credentials this run did not have.
• cargo clippy --workspace --all-targets -- -D warnings — clean, in a fresh
  target directory so the result is a real compile rather than a cache hit.
• cargo fmt --check — clean.
• Two deliberate mutations of the parser prove the tests catch a break. Reading
  the instrument id from the top level instead of the nested object fails 3
  tests. Treating a null lock as a lock fails 1 test. I reverted both and
  confirmed the file matches its original byte for byte.
• Not covered: list_holdings itself, which fetches the contracts these
  tests parse. It still has no test, because a test needs a stub ledger.

The change that enables it

• A contract() helper builds a JsActiveContract carrying a createArgument
  payload. Holding reads the concrete template payload, not an interface view,
  so the existing fixture in active_contracts.rs does not fit.

Caveats

• The tests assert on exact error strings. A reworded message breaks a test.
  That is deliberate: the messages tell an operator which field the registry
  omitted, so a silent rewording is worth catching.
• No new dependency. The tests use serde_json, already a dev dependency.

Details

What changed

• crates/token/src/holding.rs — a tests module, 152 lines.
  • from_active_contract: the four parsed fields, the nested instrument read,
    and all six error messages.
  • is_locked_in_contract: a real lock, a null lock, a missing lock field, and
    a contract with no createArgument.
• CHANGELOG.md — an entry under [Unreleased].

Why the null-lock case matters

A holding carries lock as an explicit JSON null when it is unlocked. A parser
that checked only whether the field is present would call every holding locked,
and every transfer would then skip its own funds. One test pins that behavior,
and the mutation run confirms the test fails without it.

---

Companion: DLC-link/cbtc-lib#66
deletes the duplicated struct, which leaves this parser the only one.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

There is a minor but objective test naming typo (“holding holding”) that should be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds unit test coverage for the existing token::holding::Holding JSON parser and documents the addition in the changelog, without changing parser behavior seen by callers.

Changes:

• Added a dedicated #[cfg(test)] module in crates/token/src/holding.rs covering successful parsing, error cases, and lock/null-lock behavior.
• Added an [Unreleased] changelog entry noting the new unit tests.

File summaries

File	Description
crates/token/src/holding.rs	Adds 14 unit tests for Holding::from_active_contract and Holding::is_locked_in_contract, including null-lock semantics.
CHANGELOG.md	Documents the new unit tests under [Unreleased].

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

The changes are limited to test additions and a changelog entry, with only a minor test-name typo to optionally correct.

Review details

Suppressed comments (1)

crates/token/src/holding.rs:197

• Test name has a duplicated word ("a_holding_holding...") which makes the intent harder to read/search; rename to remove the repetition.

    #[test]
    fn a_holding_with_a_lock_is_locked() {
        let mut argument = payload();

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The change is limited to adding tests and a changelog entry, with only a minor alignment nit between a test assertion and the PR’s stated intent.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/token/src/holding.rs:152

• PR description says the unit tests assert exact error strings, but this test only checks starts_with, so certain message changes could slip through. If the intent is to pin the full message, assert equality against the exact formatted error (while still deriving the parse-error detail from the same decimal parser).

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[gyorgybalazsi]

Round 2 came back with one suppressed finding, and it was right. The PR body said the tests assert exact error strings, but rejects_an_amount_that_is_not_a_decimal checked only a prefix.

Fixed in 9aa9643, using the approach the comment suggested. The test now compares the whole message and derives the parser's own detail from DamlDecimal::parse, so it pins the full string without hardcoding wording this crate does not own. Rewording the format string fails the test; I checked by doing it.

Round 1's single finding, the doubled word in a test name, was fixed in 3cb3264.

cargo test --workspace 184 passed, 0 failed, 39 ignored. cargo fmt --check clean.

🤖 Generated with Claude Code

[Copilot]

🟢 Approval recommended

Changes are limited to tests and changelog updates, and the additions are low-risk and consistent with the stated PR intent.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

crates/token/src/holding.rs:156

• This test name suggests the amount is being rejected specifically because it’s a JSON number, but the parser currently treats any non-string amount as if the field were missing (because it uses as_str()). Renaming the test to reflect that behavior will make failures easier to interpret without changing semantics.
  crates/token/src/holding.rs:162
• The asserted error message says the field is missing even though it’s present but of the wrong type. Adding a brief explanation here will prevent future readers from “fixing” the test without realizing it’s documenting the current parser behavior.

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[hubagaspar91]

Doesn't V2 holding have Account as owner? Do we have to support parsing that? Or does CIP112 affect the owner field of Holdings at all?

[gyorgybalazsi]

Closing unmerged. The 14 parser tests here move into the Holding struct fix, which rewrites the fixture: this branch's fixture names instrument.admin, and the Daml type InstrumentIdentifier has no admin field — its fields are source, id and scheme. The replacement carries 19 tests and the corrected fixture. Design: cip-112 docs/specs/2026-09-11-canton-lib-holding-struct-design.md