feat: DEFI-2671: schedule task for processing withdrawals by maciejdfinity · Pull Request #48 · dfinity/cksol

maciejdfinity · 2026-03-13T10:51:02Z

This PR adds pending withdrawal processing. It creates signed transactions for the withdrawals. The status of the withdrawal is changed to sent. The code to actually broadcast the transaction will be added in a follow up PR.

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T14:41:57Z

+pub async fn process_pending_withdrawals<R: CanisterRuntime>(runtime: &R) {
+    log!(Priority::Info, "processing pending withdrawals");
+


process_pending_withdrawals logs an Info message on every timer tick, even when there are no pending withdrawals. Given this runs every minute, it can generate sustained log volume; consider logging only when there is actual work (after the pending check), or lowering this to Debug.

I removed this log.

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (2)

libs/types/src/lib.rs:214

The WithdrawSolStatus::TxSent doc comment still implies the transaction has been sent to the network, but the minter now sets this status as soon as the transaction is signed (before any broadcast). Please align the public type documentation (or rename the variant) with the actual semantics, or delay setting TxSent until after submission succeeds.

/// Retrieve the status of a withdrawal request.
#[derive(Clone, Eq, PartialEq, Hash, Debug, CandidType, Deserialize)]
pub enum WithdrawSolStatus {
    /// Withdrawal request is not found.
    NotFound,

    /// Withdrawal request is waiting to be processed.
    Pending,

    /// Solana transaction was signed and is sent to the network.
    TxSent(SolTransaction),

    /// Solana transaction is finalized.
    TxFinalized(TxFinalizedStatus),

minter/src/main.rs:83

withdraw_sol(&IcCanisterRuntime::new(), ...) takes a reference to a temporary runtime and then awaits, which is likely to fail to compile with a "temporary value dropped while borrowed" error (the runtime reference must live across .await). Create a local IcCanisterRuntime binding (or pass the runtime by value) and pass a reference to that instead.

    cksol_minter::withdraw_sol::withdraw_sol(
        &IcCanisterRuntime::new(),
        minter_account,
        ic_cdk::api::msg_caller(),
        args.from_subaccount,
        args.amount,
        args.address,
    )
    .await

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

lpahlavi

Thanks for the changes @maciejdfinity! I missed an important point in the first round of reviews, but we should actually batch multiple withdrawals in a single transaction, see my comments below and the design document.

lpahlavi · 2026-03-25T09:33:46Z

+    };
+
+    let Some(pending_requests) =
+        read_state(|state| state.next_pending_withdrawal_requests(MAX_WITHDRAWALS_PER_BATCH))


Sorry, I wasn't clear enough. The thing is, we want to read the pending withdrawal requests from the state once, chunk it into batches, and submit a withdrawal transaction just once per pending withdrawal in a single timer execution. The problem with the current way is that if a specific withdrawal is problematic and e.g. we can't build a withdrawal transaction with it, we will keep fetching it from the state and the timer execution will never stop.

Instead, I would read all the pending withdrawal requests from the state once, chunk them into withdrawal batches, and submit them each exactly once.

lpahlavi · 2026-03-25T09:45:17Z

    Ok(WithdrawSolOk { block_index })
 }
+
+pub async fn process_pending_withdrawals<R: CanisterRuntime>(runtime: &R) {


Sorry, I missed this on the initial reviews, but this actually needs a bit of refactoring because we want to batch multiple withdrawals into a single transaction. So we should basically batch at two levels:

we want to send withdrawal transactions with 10 withdrawals per transaction

we want to make at most 10 submitTransaction calls to the SOL RPC canister concurrently

You can have a look at monitor.rs in #66 where I cleaned things up a bit using itertools.

Also, note that we should fetch a new recent blockhash for every round of transactions we send out to make sure it doesn't expire in the meantime.

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

minter/src/main.rs:82

withdraw_sol is async and now takes runtime: &R, but this call borrows a temporary (&IcCanisterRuntime::new()). That reference will need to live across .await, which typically fails to compile (temporary value dropped while borrowed). Create a local let runtime = IcCanisterRuntime::new(); and pass &runtime, or change the API back to taking R by value.

    cksol_minter::withdraw_sol::withdraw_sol(
        &IcCanisterRuntime::new(),
        minter_account,
        ic_cdk::api::msg_caller(),
        args.from_subaccount,
        args.amount,
        args.address,
    )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-25T10:51:48Z

+    let sign_futures: Vec<_> = withdrawal_params
+        .iter()
+        .map(|(sources, destination)| {
+            create_signed_transfer_transaction(
+                minter_account,
+                sources,
+                *destination,
+                recent_blockhash,
+                &signer,
+            )
+        })
+        .collect();
+
+    let results = futures::future::join_all(sign_futures).await;
+
+    for (request, result) in pending_requests.into_iter().zip(results) {


process_pending_withdrawals signs all pending withdrawals concurrently (join_all). Each create_signed_transfer_transaction calls lazy_get_schnorr_master_key().await, which can race on a fresh install/upgrade when the key is still None: multiple concurrent calls will all reach the schnorr_public_key().await and then the second set_once_minter_public_key will panic. To avoid a canister trap, initialize the master key once before spawning the signing futures (or refactor signing to accept a pre-fetched master key / make the key-setting idempotent).

Suggested change

let sign_futures: Vec<_> = withdrawal_params

.iter()

.map(|(sources, destination)| {

create_signed_transfer_transaction(

minter_account,

sources,

*destination,

recent_blockhash,

&signer,

)

})

.collect();

let results = futures::future::join_all(sign_futures).await;

for (request, result) in pending_requests.into_iter().zip(results) {

for (request, (sources, destination)) in

pending_requests.into_iter().zip(withdrawal_params.into_iter())

{

let result = create_signed_transfer_transaction(

minter_account,

&sources,

destination,

recent_blockhash,

&signer,

)

.await;

Copilot · 2026-03-25T10:51:49Z

+            let transfer_amount = request
+                .withdrawal_amount
+                .checked_sub(request.withdrawal_fee)
+                .expect("BUG: withdrawal_amount must be >= withdrawal_fee");
+            let sources = vec![(minter_account, transfer_amount)];


checked_sub(...).expect(...) will trap the canister if a withdrawal request ever has withdrawal_amount < withdrawal_fee (e.g., due to a future config/validation change or corrupted state). Since this is processing code that runs in a timer, it would be safer to handle None by logging an error and skipping (or marking the request as invalid) rather than panicking.

Copilot · 2026-03-25T10:51:49Z

+        let len = d.array()?.ok_or_else(|| Error::message("expected definite-length array"))?;
+        let mut result = Vec::with_capacity(len as usize);
+        for _ in 0..len {
+            d.array()?;
+            let burn_index = LedgerBurnIndex::new(d.u64()?);
+            let sig_bytes = d.bytes()?;
+            let signature =
+                Signature::try_from(sig_bytes).map_err(|e| Error::message(e.to_string()))?;
+            result.push((burn_index, signature));


burn_index_signature_vec::decode reads each inner tuple with d.array()? but doesn’t verify the tuple length (and doesn’t handle indefinite-length arrays). Since encode always writes an array of length 2, consider validating that the inner array length is exactly 2 (and/or consuming the break for indefinite arrays) to make decoding robust and fail with a clearer error on malformed data.

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-25T12:50:57Z

            prop::collection::vec((arb_account(), any::<Lamport>()), 1..10)
                .prop_map(|deposits| EventType::ConsolidatedDeposits { deposits }),
+            prop::collection::vec((arb_ledger_burn_index(), arb_signature()), 1..10)
+                .prop_map(|transactions| EventType::SentWithdrawalTransaction { transactions },),


The .prop_map closure has an extra trailing comma after the EventType::SentWithdrawalTransaction { transactions } expression, which changes the closure to return a 1-tuple and should fail to compile/typecheck. Remove the trailing comma so the closure returns EventType.

Suggested change

.prop_map(|transactions| EventType::SentWithdrawalTransaction { transactions },),

.prop_map(|transactions| EventType::SentWithdrawalTransaction { transactions }),

Copilot · 2026-03-25T12:50:58Z

+    SentWithdrawalTransaction {
+        /// The burn block indices and corresponding transaction signatures.
+        #[cbor(n(0), with = "cbor::burn_index_signature_vec")]
+        transactions: Vec<(LedgerBurnIndex, Signature)>,


SentWithdrawalTransaction only records burn index + signature. Since a Solana signature is tied to the exact message/blockhash, without also persisting the signed transaction bytes/message (or at least enough data to deterministically reconstruct the exact transaction), the system cannot broadcast the already-signed transaction in a later timer tick/PR. Consider storing the full signed transaction (e.g., serialized Transaction/Message + required signer accounts) or reusing the existing SubmittedTransaction event shape for withdrawals.

Suggested change

transactions: Vec<(LedgerBurnIndex, Signature)>,

transactions: Vec<(LedgerBurnIndex, Signature)>,

/// The serialized bytes of the signed Solana transactions, in the same

/// order as the `transactions` field. When present, these bytes can be

/// used to (re)broadcast the exact transaction(s) later.

#[n(1)]

signed_transactions: Option<Vec<Vec<u8>>>,

Copilot · 2026-03-25T12:50:58Z

+    fn process_sent_withdrawal_transaction(
+        &mut self,
+        burn_block_index: &LedgerBurnIndex,
+        signature: &Signature,
+    ) {
+        assert!(
+            self.pending_withdrawal_requests
+                .remove(burn_block_index)
+                .is_some(),
+            "Attempted to send transaction for unknown withdrawal request: {:?}",
+            burn_block_index
+        );
+        assert_eq!(
+            self.sent_withdrawal_requests
+                .insert(*burn_block_index, *signature),
+            None,
+            "Attempted to send transaction for already sent withdrawal request: {:?}",
+            burn_block_index
+        );


process_sent_withdrawal_transaction removes the pending withdrawal request and only retains the signature. Given this PR explicitly does not broadcast transactions yet, this transition drops the information needed to later broadcast/retry the withdrawal (destination address, amount, fee, etc.) and effectively makes the withdrawal irrecoverable without replaying the audit log. Consider keeping the full request (and/or the signed transaction) in state until broadcast is successfully completed, and only then move it to a "sent/submitted" tracking structure.

Copilot · 2026-03-25T12:50:58Z

    // Solana transaction was signed and is sent to the network.
    TxSent : SolTransaction;



The TxSent documentation says the transaction "is sent to the network", but in this PR withdrawals are marked as TxSent immediately after signing and before any broadcast is implemented (per PR description / TODOs). This makes the public status misleading; consider adjusting the docs and/or introducing a distinct status for "signed but not broadcast" and only using TxSent after successful submission.

Copilot · 2026-03-25T12:50:58Z

-    /// Transaction is not signed yet.
-    TxCreated,
-
    /// Solana transaction was signed and is sent to the network.


The WithdrawSolStatus::TxSent docstring states the transaction "is sent to the network", but the minter currently sets this status right after signing while broadcast is deferred to a follow-up PR. To avoid misleading API consumers, update the docs/status model so TxSent only applies after successful broadcast, or add a separate status for "signed/ready".

Suggested change

/// Solana transaction was signed and is sent to the network.

/// Solana transaction was signed and is ready to be sent to the network.

lpahlavi

Thanks for the PR @maciejdfinity!

lpahlavi and others added 6 commits March 12, 2026 07:17

feat: create scheduled task to consolidate deposit funds

bcf34bf

Schedule global timer for transaction consolidation

e47b9e4

Merge branch 'main' into lpahlavi/consolidation-task

c842913

Clippy

b55b8a2

schedule task for processing withdrawals

d787c74

clippy

11329e9

Base automatically changed from lpahlavi/consolidation-task to main March 13, 2026 12:29

maciejdfinity added 23 commits March 13, 2026 13:49

Merge branch 'main' into maciej-withdraw-send

3c9ab21

creating and signing the transaction

6140a01

store the whole request

f6bf491

remove txcreated

122cd02

log errors

ac07f54

Merge branch 'main' into maciej-withdraw-send

2b668af

change create_signed_transfer_transaction target from Account to Address

025f466

canister_self in runtime

4b936d5

add block hash and slot stubs

f7b8aa2

add schnorr signer to runtime

9af949d

refactor MockSchnorrSigner into one implementation

a788455

check events generated in the test

94339b8

assert no events are recorded

3ee8179

test for failing hash calculation

aa0c0ac

refactor

7eb11f0

add comment

ac29698

check for status in tests

6c576c8

test failed signing

79fa9a4

Merge branch 'main' into maciej-withdraw-send

1bdcc61

up to max test

a4ec853

log info about guard, test

b921445

clippy

d3ea570

test if timer is started

290a1ea

update comment

dc334d2

Copilot AI review requested due to automatic review settings March 24, 2026 14:33

Copilot started reviewing on behalf of maciejdfinity March 24, 2026 14:34 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

maciejdfinity added 2 commits March 24, 2026 15:46

Merge branch 'main' into maciej-withdraw-send

a908be2

remove chatty log, adapt guard log message

a0e3a37

Copilot AI review requested due to automatic review settings March 24, 2026 14:54

Copilot started reviewing on behalf of maciejdfinity March 24, 2026 14:54 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

Comment thread minter/cksol-minter.did

Comment thread minter/src/main.rs

Comment thread minter/src/withdraw_sol/mod.rs

Comment thread minter/src/state/mod.rs

Merge branch 'main' into maciej-withdraw-send

b0d0ad7

lpahlavi reviewed Mar 25, 2026

View reviewed changes

use execute_http_mocks

25cabd2

Copilot AI review requested due to automatic review settings March 25, 2026 09:53

Copilot started reviewing on behalf of maciejdfinity March 25, 2026 09:53 View session

WITHDRAWAL_PROCESSING_DELAY

6aa53e5

Copilot AI reviewed Mar 25, 2026

View reviewed changes

Comment thread minter/src/main.rs

maciejdfinity added 2 commits March 25, 2026 11:06

use ready methods

90d334e

change to vec of withdrawals

db65a1f

Copilot AI review requested due to automatic review settings March 25, 2026 10:45

Copilot started reviewing on behalf of maciejdfinity March 25, 2026 10:45 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

remove next_pending_withdrawal_requests

9aeca1e

maciejdfinity requested a review from lpahlavi March 25, 2026 11:00

clippy

cfcf254

Copilot AI review requested due to automatic review settings March 25, 2026 12:41

Copilot started reviewing on behalf of maciejdfinity March 25, 2026 12:42 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

lpahlavi approved these changes Mar 25, 2026

View reviewed changes

maciejdfinity merged commit bc1c243 into main Mar 25, 2026
14 checks passed

maciejdfinity deleted the maciej-withdraw-send branch March 25, 2026 12:59

		pub async fn process_pending_withdrawals<R: CanisterRuntime>(runtime: &R) {
		log!(Priority::Info, "processing pending withdrawals");

	.prop_map(\|transactions\| EventType::SentWithdrawalTransaction { transactions },),
	.prop_map(\|transactions\| EventType::SentWithdrawalTransaction { transactions }),

-        transactions: Vec<(LedgerBurnIndex, Signature)>,
+        transactions: Vec<(LedgerBurnIndex, Signature)>,
+        /// The serialized bytes of the signed Solana transactions, in the same
+        /// order as the `transactions` field. When present, these bytes can be
+        /// used to (re)broadcast the exact transaction(s) later.
+        #[n(1)]
+        signed_transactions: Option<Vec<Vec<u8>>>,

		// Solana transaction was signed and is sent to the network.
		TxSent : SolTransaction;

	/// Solana transaction was signed and is sent to the network.
	/// Solana transaction was signed and is ready to be sent to the network.

Conversation

maciejdfinity commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

maciejdfinity Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lpahlavi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lpahlavi Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lpahlavi Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

lpahlavi left a comment

Choose a reason for hiding this comment

maciejdfinity commented Mar 13, 2026 •

edited

Loading