Dead inputSum accumulation in ValidateTransferActionTokenTypes: value-conservation check documented but not performed

[Effi-S]

Fixes #2426

Problem

ValidateTransferActionTokenTypes in token/core/common/auditor.go documents that, when validateValueSum is true (privacy-preserving tokens like zkatdlog), it "validates that the sum of input values equals the sum of output values":

// When validateValueSum is true (for privacy-preserving tokens like zkatdlog), this also validates
// that the sum of input values equals the sum of output values using the provided precision.

In practice this validation is not performed. inputSum is initialized and accumulated inside the input loop, but it is never consumed: there is no output-sum accumulation and no comparison before the function returns nil. The value-conservation guarantee promised by the Godoc and the validateValueSum parameter is therefore a no-op, and the accumulation is dead.

Impact

• The documented behaviour (input/output value conservation) does not match the implementation.
• Callers passing validateValueSum = true may believe value conservation is enforced when it is not.
• Dead accumulation and an effectively unused parameter path.

Notes

Pre-existing; surfaced during review of #2394 (#2394 (review)).

[AkramBitar]

Thanks for this — the core change is right. I confirmed against the pre-PR source that inputSum was genuinely dead: it was accumulated in the input loop and then the function returned nil with no output-sum accumulation and no comparison. Removing it together with validateValueSum/precision is the correct resolution of #2426. Build is clean and token/core/common, token/core/fabtoken/... and token/core/zkatdlog/nogh/v1/audit all pass locally.

My one substantive concern: the PR replaces the inaccurate comment with three new comments that are also inaccurate about where value conservation is enforced — the same defect class #2426 was filed for. Details inline (3 medium, 2 low).

For reference, conservation actually lives in the validation path, not in either auditor:

• fabtoken: inputSum.Cmp(outputSum) at token/core/fabtoken/v1/transfer.go:259
• zkatdlog: TypeAndSumVerifier.Verify at token/core/zkatdlog/nogh/v1/transfer/typeandsum.go:336

One nit outside the diff: the commit subject removde the dead inputSum accumulation and all related parameters has a typo and doesn't follow the repo's conventional-commit style — something like fix(audit): remove dead inputSum accumulation from transfer type validation would match the surrounding history. Sign-off is present.

[AkramBitar]

Reviewed the cleanup — the deletion is correct and this should merge. inputSum was genuinely accumulated and never compared to anything, and both "it's enforced elsewhere" claims check out: zkatdlog's conservation comes from TypeAndSumVerifier (token/core/zkatdlog/nogh/v1/transfer/typeandsum.go:318, wired from bftransfer.go:93 / csptransfer.go:192), and the new claim that TransferMetadata.Match checks neither output types nor values is accurate (token/driver/request.go:874 only checks counts, extra signers and issuer). The audit tokens the removed code parsed come from the auditor's own local DB via queryEngine.ListAuditTokens, so dropping the ToQuantity parse loses no trust-boundary check. Build, go vet and the three affected packages' tests pass on this head; all call sites of the narrowed signatures are updated with no stale references in code or docs/.

Two non-blocking nits inline: the new comment points at the wrong function, and precision is now dead in this package too.

One thing worth raising explicitly: after this change the fabtoken auditor performs no value check at all — TransferAuditValidate runs Match (counts/signers/issuer) plus input-type consistency, and never compares declared input quantities against its own audit tokens. So the auditor's signature does not attest to value conservation; an inflated transfer is caught only later, at commit, by TransferBalanceValidate. Nothing regressed here (the accumulation was dead), and the cleanup label suggests this is accepted — but closing #2426 this way settles the "should the auditor verify conservation?" question by omission rather than by decision. If that's intended, the new comments are the right place to say so outright.