refactor(minter): model submitted transactions as a MinterTransaction enum by gregorydemay · Pull Request #252 · dfinity/cksol

gregorydemay · 2026-10-06T14:03:24Z

Pure refactoring in preparation for durable-nonce withdrawals: the transactions the minter submits and monitors are modelled as an enum with one variant per kind (deposit sweep, withdrawal), replacing a struct tagged by a purpose field. Every place handling a submitted transaction now matches exhaustively on its kind, so that the per-kind decisions (what to do on success, failure, or expiry) are explicit and the upcoming nonce-based withdrawal variant can reuse the same finalization infrastructure.

No behaviour change: stored events and the Candid interface are unchanged.

🤖 Generated with Claude Code

Copilot

Copilot review overview

🟡 Changes recommended

Transaction-size validation changes signer invocation and error precedence despite the stated behavior-preserving scope.

Review effort: Balanced
Findings: 1

Open (1)

Oversized transactions bypass signing, changing error precedence · New

What changed in this PR

Refactors submitted transaction state into kind-specific MinterTransaction variants while preserving durable events and monitoring flows.

Changes:

Replaces SolanaTransaction with sweep and withdrawal variants.
Updates monitoring and tests for enum accessors.
Moves transaction-size validation before signing.

File	Description
`minter/src/state/mod.rs`	Introduces and processes the transaction enum.
`minter/src/state/tests.rs`	Updates sweep assertions.
`minter/src/monitor/mod.rs`	Uses enum accessors during monitoring.
`minter/src/monitor/tests.rs`	Updates resubmission assertions.
`minter/src/withdraw/tests.rs`	Verifies withdrawal variant metadata.
`minter/src/sol_transfer/mod.rs`	Validates size before signing.
`minter/src/sol_transfer/tests.rs`	Updates oversized-transaction test.
`minter/src/test_fixtures/runtime.rs`	Records timer delays.

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

+    ensure_within_transaction_size(&transaction.message)?;
+    transaction.signatures =
+        sign_bytes(signer_derivation_paths, signer, transaction.message_data()).await?;


zeropath-ai · 2026-10-06T15:08:28Z

✅ No security or compliance issues detected. Reviewed everything up to a8431f3.

Security Overview

🔎 Scanned files: 8 changed file(s)
🔗 Scan Link: https://zeropath.com/app/repositories/58f1cdfe-c0c2-41c1-8d28-3d6efcaf3266?scanId=2c84c14d-6c5d-4a3a-be31-1382363ff6eb&codeScanTypes=PrScan&tab=issues&detectionTypes=traditional,businessLogic,customRule,IAC,SECRETS,CICD,EOL,uncategorized

Detected Code Changes

Change Type	Relevant files
Enhancement	► minter/src/monitor/mod.rs Modify block_height accessors to tx.block_height() instead of field access ► minter/src/monitor/mod.rs Update resubmit_transactions to use tx.message() and tx.signers()
Enhancement	► minter/src/monitor/tests.rs Update assertion to use resubmitted.block_height()
Enhancement	► minter/src/sol_transfer/mod.rs Introduce ensure_within_transaction_size and adjust message/signatures handling to use transaction.message_data()
Enhancement	► minter/src/sol_transfer/tests.rs Refactor to construct transactions with TestCanisterRuntime and updated signature of create_signed_batch_withdrawal_transaction call
Enhancement	► minter/src/state/mod.rs Refactor transaction types from SolanaTransaction to MinterTransaction with variants SweepDeposit and Withdrawal ► minter/src/state/mod.rs Add methods message(), signers(), block_height() for MinterTransaction ► minter/src/state/mod.rs Update state fields and accessors to use MinterTransaction
Enhancement	► minter/src/state/tests.rs Update tests to match MinterTransaction::SweepDeposit pattern and verify deposit_ids and signers via new structure
Enhancement	► minter/src/test_fixtures/runtime.rs Rename and adapt timer-related fields: set_timer_call_count to derive from set_timer_delays length; adjust set_timer to record delays
Enhancement	► minter/src/withdraw/tests.rs Update imports to use MinterTransaction; adjust submitted transaction assertions to use block_height() and pattern matching for Withdrawal variant

… enum Replace the SolanaTransaction struct and its purpose field with a MinterTransaction enum whose SweepDeposit and Withdrawal variants carry the deposit ids or burn indices directly, so that the transaction lifecycle handlers match exhaustively on the kind of transaction. Stored events and the Candid interface are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…signing Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

mbjorkqvist

Thanks @gregorydemay! Just a couple of findings from Claude.

mbjorkqvist · 2026-10-06T15:59:38Z

+        signers: Vec<Signer>,
+        /// The block height of the block whose blockhash the transaction uses.
+        block_height: BlockHeight,
+        /// Total transfer amount in lamports (excluding fees).


🤖 Since #221 removed consolidation, amount is only ever written, never read. Its doc is also inaccurate for this variant: it's set to the return value of deposits.sweep(…), which is the expected received amount, i.e. after the fee. A pure refactoring introducing a new type seems the natural moment to drop it; the draft #234 ("remove the per-transaction amount") does exactly that and will conflict with this PR (and #248 carries the field and its doc forward too). Could we drop amount here (and close #234), or land #234 first? Relatedly, deposit_ids and burn_indices are also only written; fine if the nonce work needs them, otherwise they duplicate what the state already tracks per deposit / withdrawal.

mbjorkqvist · 2026-10-06T15:59:38Z

+}
+
+impl MinterTransaction {
+    pub fn message(&self) -> &VersionedMessage {


🤖 nit: the three accessors each repeat an identical arm per variant; or-patterns (MinterTransaction::SweepDeposit { message, .. } | MinterTransaction::Withdrawal { message, .. } => message) would avoid that, as #248 already does, which would also shrink #248's diff.

Copilot

Copilot review overview

🔵 Needs a closer look

Transaction-size validation now changes signing behavior despite the stated behavior-preserving scope.

Review effort: Balanced
Findings: 1

Open (1)

Oversized transactions bypass signing, changing error precedence

gregorydemay added this pull request to stack #253 October 6, 2026 14:26

gregorydemay removed this pull request from stack #253 October 6, 2026 14:29

gregorydemay added this pull request to stack #254 October 6, 2026 14:29

Copilot AI balanced review requested due to automatic review settings October 6, 2026 15:01

gregorydemay removed this pull request from stack #254 October 6, 2026 15:02

gregorydemay added this pull request to stack #256 October 6, 2026 15:02

Copilot started reviewing on behalf of gregorydemay October 6, 2026 15:02 View session

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Comment thread minter/src/sol_transfer/mod.rs

Comment on lines +119 to +121

ensure_within_transaction_size(&transaction.message)?;

transaction.signatures =

sign_bytes(signer_derivation_paths, signer, transaction.message_data()).await?;

gregorydemay marked this pull request as ready for review October 6, 2026 15:06

gregorydemay requested a review from a team as a code owner October 6, 2026 15:06

gregorydemay requested review from eichhorl and mbjorkqvist October 6, 2026 15:07

gregorydemay and others added 3 commits October 6, 2026 15:45

test(minter): record the delays passed to set_timer in the test runtime

dfec018

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

refactor(minter): check the transaction size from the message before …

a8431f3

…signing Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

mbjorkqvist approved these changes Oct 6, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 7, 2026 05:32

gregorydemay force-pushed the refactor/minter-transaction-enum branch from ce22834 to a8431f3 Compare October 7, 2026 05:32

Copilot started reviewing on behalf of gregorydemay October 7, 2026 05:33 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

Navigation Menu

Repository navigation

refactor(minter): model submitted transactions as a MinterTransaction enum - #252

refactor(minter): model submitted transactions as a MinterTransaction enum#252
gregorydemay wants to merge 3 commits into
feat/nonce-account-readingfrom
refactor/minter-transaction-enum

gregorydemay commented Oct 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

zeropath-ai Bot commented Oct 6, 2026 •

edited

Loading

Uh oh!

mbjorkqvist left a comment

Uh oh!

mbjorkqvist Oct 6, 2026

Uh oh!

mbjorkqvist Oct 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gregorydemay commented Oct 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

zeropath-ai Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbjorkqvist left a comment

Choose a reason for hiding this comment

Uh oh!

mbjorkqvist Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

mbjorkqvist Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zeropath-ai Bot commented Oct 6, 2026 •

edited

Loading