Skip to content

fix(token): count composite-owner outputs once per enrollment ID in amount sums - #2148

Merged
AkramBitar merged 1 commit into
LFDT-Panurus:mainfrom
Built-by-Sign:fix/composite-output-accounting
Aug 11, 2026
Merged

fix(token): count composite-owner outputs once per enrollment ID in amount sums#2148
AkramBitar merged 1 commit into
LFDT-Panurus:mainfrom
Built-by-Sign:fix/composite-output-accounting

Conversation

@EvanYan1024

@EvanYan1024 EvanYan1024 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Fixes #2147

What

  • Add OutputStream.UniquePerOutput, keeping for each (Index, EnrollmentID) pair only the first output, and apply it in the ttxdb TransactionRecords and Movements sums (shared by the owner and auditor stores).
  • Add InputStream.UniquePerInput, keeping for each (token ID value, EnrollmentID) pair only the first input — rows with no token ID are all kept — and apply it in the Movements sent sum. The input side has the same shape (raised in review below): extractTransferInputs emits one row per member with the same token ID and AuditRecord fills every row with the token's full quantity.

Why

A composite owner (multisig, boolpolicy) is expanded into one audit row per member on both sides of a transaction, each row carrying the full amount. Members sharing an enrollment ID therefore multiply the amount by the member count in every eid-keyed sum: in our test environments a 100 deposit into a wallet whose owner held two keys of the same enrollment booked as 200 received, and review reproduced the symmetric sent side — a 40 spend from a two-member wallet booked as -80.

Deduplicating at extraction time instead would drop data that identity consumers need — RevocationHandles() must surface a revoked second key and ByRecipient() must see every member — so the collapse happens only where amounts aggregate.

Testing

  • token/request_composite_output_test.go: composite owners with same-enrollment and cross-enrollment members, issue and transfer paths; asserts identity consumers still see every member row.
  • token/services/storage/ttxdb/store_test.go: TransactionRecords/Movements book a composite-owner output once per enrollment ID; the composite-spend fixture expands the spent input into two member rows sharing one token ID, pinning the movement to -6 instead of -46.
  • token/stream_test.go: table-driven contract tests for UniquePerOutput and UniquePerInput — the same (Index, EnrollmentID) / (token ID, EnrollmentID) pair collapses to the first row (for inputs via distinct pointers to the same token ID value); the same index or token ID with different enrollment IDs, and different indexes or token IDs with the same enrollment ID, all survive; inputs with no token ID are all kept.

Docs

docs/services/storage/ttxdb.md gains a "Composite Owners" note: one audit row per member for identity/revocation visibility, amount aggregations count each (output index, enrollment ID) / (token ID, enrollment ID) pair once.

@EvanYan1024
EvanYan1024 force-pushed the fix/composite-output-accounting branch 2 times, most recently from 92f93c6 to 11bd6ca Compare August 5, 2026 06:10
@AkramBitar
AkramBitar requested review from AkramBitar and adecaro and removed request for AkramBitar August 5, 2026 09:05
AkramBitar

This comment was marked as resolved.

@AkramBitar

Copy link
Copy Markdown
Contributor

@EvanYan1024

Thanks a lot for this PR.

Nice fix. One thing I wanted to ask about: does the same apply to the input side? extractTransferInputs looks like it expands members the same way (for _, sender := range input.Senders, same token ID), and AuditRecord() fills each row with the full quantity — so received is fixed but sent would still count once per member. Spending 40 from a two-member wallet gave me -80 rather than -40, so I may be missing something about that path. If it's the same shape, would an InputStream.UniquePerInput() keyed on (Id, EnrollmentID) be the natural fit, given Input has no index? Fine as a follow-up if you'd prefer. What do you think?

@EvanYan1024

Copy link
Copy Markdown
Contributor Author

Thanks Akram, you are right. I traced the input path and confirmed that it has the same representation as the output side: extractTransferInputs emits one row per composite-owner member with the same token ID, and AuditRecord fills every row with the token’s full quantity. Movements currently sums those rows directly, so two members sharing an enrollment ID count a 40-unit input as 80.

InputStream.UniquePerInput() keyed by the token ID value (TxId, Index) plus EnrollmentID is the appropriate symmetric fix. The member rows should remain intact for identity and revocation consumers, with deduplication applied only at the amount aggregation boundary.

I’ll include this in the current PR since it is the same accounting issue rather than leave movements partially fixed. I’ll also update the composite-spend test to use two input member rows, add the UniquePerInput contract tests, and update the documentation accordingly.

Thanks a lot for the concrete -80 reproduction — it made the input-side gap easy to confirm.

Comment thread token/stream.go Outdated
@AkramBitar

AkramBitar commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@EvanYan1024 Thanks a lot, I have the following concerns, I think they may warrant a follow-up rather than changes in this PR. What do you think?

1. Different enrollment IDs. Both keys include EnrollmentID, so ordinary multisig still multiplies — and ttxdb/store.go:374-376 errors on such an input (expected at most 1 input enrollment id), so the transaction can't be recorded at all. Intended, or should the key drop the eID?

2. Input has no index. UniquePerInput keys on *t.Id, but Metadata.filterTransfer never copies TokenID and ttx/marshaller.go:126 runs that filter outbound — so Id == nil is routine and the dedup no-ops there. Harmless today. Worth giving Input an index like Output.Index?

3. Auditor views. integration/token/fungible/views/auditor.go:77-78, 100-101, 132-133 (also interop/views/auditor.go:54-55) still use un-deduped Sum(), so the limit checks double for same-eID composites and contradict the new docs note. Here or separate PR?

@EvanYan1024

Copy link
Copy Markdown
Contributor Author

Thanks — I've done (3) here and would leave (1) as is and (2) as a follow-up. Reasoning below.

  1. Different enrollment IDs. I'd keep the enrollment ID in the key. The expected at most 1 input enrollment id guard you point at is pre-existing on main and this PR doesn't touch it, so a cross-enrollment composite input is refused by the store before any deduplication could matter — dropping the eID from the key would quietly change the outcome for a case the store deliberately rejects, which feels like a separate decision rather than a detail of this change.

It is also a no-op where it is actually used: in Movements the deduplication runs after ByEnrollmentID(eID), so every row in the stream already carries the same enrollment ID. The eID in the key only guards a caller who reaches for UniquePerInput on an unfiltered stream.

  1. Input has no index. Your mechanism is right — filterTransfer builds &driver.TransferInputMetadata{} and copies only Senders, never TokenID — but I'd put it slightly differently than "harmless today". The auditor receives unfiltered metadata (ttx/auditor.go:219 and :262 call tx.Bytes() with no eIDs), so Id is populated and the deduplication does its job exactly where the audit balances are computed. The no-op case is the eID-filtered copy sent at collectendorsements.go:550, i.e. a counterparty's own ttxdb.

Giving Input an index is the right fix, and there is no cheap substitute: ActionIndex cannot serve as the key because several distinct tokens share one action index, so keying on it would over-deduplicate. The index has to be carried through TransferInputMetadata, which is a driver metadata change and larger than this PR. I'll open a follow-up issue for it unless you'd rather it land here.

  1. Auditor views. Done in 1d8f92b: the payment, cumulative and holding limit checks in integration/token/fungible/views/auditor.go and the payment limit in integration/token/interop/views/auditor.go now use UniquePerOutput() / UniquePerInput(), so the samples match the aggregation rule the documentation states. They run on the auditor path, where both streams carry the keys the deduplication needs. Deduplication only lowers the amounts a limit is evaluated against, so no existing expectation tightens.

I ran the token module tests with -race and the pinned linter over both modules, but not the integration suite locally — leaving that to CI.

@adecaro adecaro added this to the Q3/26 milestone Aug 11, 2026
@AkramBitar

Copy link
Copy Markdown
Contributor

@EvanYan1024

Thanks a lot for the great work you did on this PR. I really appreciate it!

Best Regards,
Akram

Problem

Tokens owned by a composite owner (multisig) list each member's enrollment ID
separately. Amount aggregations walked those entries individually, so a single
token was counted once per member — inflating auditor balances, movements, and
the sums that auditor limit checks are evaluated against.

Fix

Deduplicate before summing: each (output index, enrollment ID) /
(token ID, enrollment ID) pair is counted once.

  • token/stream.go — new dedup filters on the input/output streams.
  • token/services/storage/ttxdb/store.go — applied when building transaction
    records and movements.
  • integration/token/{fungible,interop}/views/auditor.go — applied at the
    payment, cumulative, and holding limit checks.
  • docs/services/storage/ttxdb.md — documents the aggregation rule.

Out of scope

Composite owners whose members have distinct enrollment IDs are unchanged:
such an input is still rejected by TransactionRecords, and Movements books
the full amount once per co-owner. Tracked separately.

@AkramBitar
AkramBitar force-pushed the fix/composite-output-accounting branch from 02a4134 to b65e73f Compare August 11, 2026 07:22

@AkramBitar AkramBitar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AkramBitar

Copy link
Copy Markdown
Contributor

@EvanYan1024

I would like to rebase and merge the changes. Could you please rebase and then squash all the commits to one?

Regards,
Akram

@EvanYan1024
EvanYan1024 force-pushed the fix/composite-output-accounting branch from b65e73f to 5699fa2 Compare August 11, 2026 08:52
@EvanYan1024

EvanYan1024 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@AkramBitar Done — rebased onto main

Ready to merge whenever you are. Thanks for the thorough review

Tokens owned by a composite owner (multisig) list each member's enrollment
ID separately. Amount aggregations walked those entries individually, so a
single token was counted once per member - inflating auditor balances,
movements, and the sums that auditor limit checks are evaluated against.

Deduplicate before summing: each (output index, enrollment ID) / (token ID,
enrollment ID) pair is counted once.

- token/stream.go - new dedup filters on the input/output streams.
- token/services/storage/ttxdb/store.go - applied when building transaction
  records and movements.
- integration/token/{fungible,interop}/views/auditor.go - applied at the
  payment, cumulative, and holding limit checks.
- docs/services/storage/ttxdb.md - documents the aggregation rule.

Composite owners whose members have distinct enrollment IDs are unchanged:
such an input is still rejected by TransactionRecords, and Movements books
the full amount once per co-owner. Tracked separately.

Fixes LFDT-Panurus#2147

Signed-off-by: Evan <evanyan@sign.global>
@AkramBitar
AkramBitar force-pushed the fix/composite-output-accounting branch from 5699fa2 to 3193823 Compare August 11, 2026 09:05
@AkramBitar
AkramBitar merged commit 4685203 into LFDT-Panurus:main Aug 11, 2026
152 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Composite-owner outputs are counted once per member in eid-keyed amount sums

3 participants