refactor: use mint indices in ConsolidatedDeposits event#66
Merged
Conversation
lpahlavi
force-pushed
the
lpahlavi/consolidation-mint-indices
branch
4 times, most recently
from
5102da6 to
218b6e7
Compare
lpahlavi
force-pushed
the
lpahlavi/consolidation-mint-indices
branch
from
218b6e7 to
eadd0cc
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the minter’s deposit consolidation flow and event schema so ConsolidatedDeposits reports consolidated mint indices (instead of (Account, Lamport) pairs), and consolidation work is queued at mint time with per-account deposits merged into fewer transfer instructions.
Changes:
- Updated
ConsolidatedDepositsevent payload across CBOR, Candid (.did), and internal event types to emitmint_indices. - Reworked
Stateconsolidation tracking fromfunds_to_consolidate: BTreeMap<Account, Lamport>todeposits_to_consolidate: BTreeMap<LedgerMintIndex, (Account, Lamport)>, populated onMinted. - Updated consolidation logic/tests to batch by unique accounts and merge multiple deposits to the same account into a single transfer.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
minter/src/test_fixtures/mod.rs |
Updates proptest event generator to produce mint_indices for ConsolidatedDeposits. |
minter/src/state/tests.rs |
Adjusts state initialization expectations for renamed consolidation map. |
minter/src/state/mod.rs |
Renames/rekeys consolidation queue; moves enqueueing from accept → mint; updates consolidation replay handling. |
minter/src/state/event/cbor/mod.rs |
Adds minicbor encode/decode helpers for Vec<LedgerMintIndex>. |
minter/src/state/event.rs |
Changes ConsolidatedDeposits event definition to mint_indices with CBOR customization. |
minter/src/state/audit.rs |
Updates audit replay to apply consolidation by mint_indices. |
minter/src/main.rs |
Maps internal LedgerMintIndex list to candid nat64 list in get_events. |
minter/src/consolidate/tests.rs |
Updates consolidation tests and adds coverage for merging multiple deposits to one account into one transfer. |
minter/src/consolidate/mod.rs |
Groups deposits by account for transfer creation; emits ConsolidatedDeposits { mint_indices }. |
minter/cksol-minter.did |
Updates public Candid event schema for ConsolidatedDeposits. |
minter/Cargo.toml |
Adds itertools dependency (workspace). |
libs/types-internal/src/event.rs |
Updates Candid-compatible Rust event type for ConsolidatedDeposits to use mint_indices. |
Cargo.toml |
Adds itertools = "0.14.0" to workspace dependencies. |
Cargo.lock |
Locks itertools dependency addition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace the (Account, Lamport) pairs in the ConsolidatedDeposits event with mint indices. Rename funds_to_consolidate to deposits_to_consolidate and re-key it from Account to LedgerMintIndex. Deposits are now added to the consolidation queue during Minted (not AcceptedDeposit), and multiple deposits to the same account are merged into a single transfer instruction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lpahlavi
force-pushed
the
lpahlavi/consolidation-mint-indices
branch
from
eadd0cc to
2e3d998
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
870
to
877
| for event in &events_after { | ||
| if let EventType::ConsolidatedDeposits { deposits } = &event.payload { | ||
| let total: Lamport = deposits.iter().map(|(_, amount)| amount).sum(); | ||
| assert_eq!( | ||
| total, DEPOSIT_AMOUNT, | ||
| "Consolidated amount should match the deposit amount" | ||
| if let EventType::ConsolidatedDeposits { mint_indices } = &event.payload { | ||
| assert!( | ||
| !mint_indices.is_empty(), | ||
| "ConsolidatedDeposits should contain at least one mint index" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This integration assertion became very weak after switching from (Account, Lamport) to mint_indices: it only checks that the list is non-empty. Since the preceding call returns DepositStatus::Minted, you can extract the minted mint_block_index from the Minted event(s) in events_after and assert that at least one ConsolidatedDeposits.mint_indices contains that index (and possibly only indices for minted deposits in this test).
THLO
approved these changes
Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace the
(Account, Lamport)pairs in theConsolidatedDepositsevent with mint indices. The internalfunds_to_consolidatemap is renamed todeposits_to_consolidateand re-keyed fromAccounttoLedgerMintIndex. Deposits are now added to the consolidation queue when minted rather than when accepted. Multiple deposits to the same account are merged into a single transfer instruction during consolidation.