feat: resubmit expired transactions with new blockhash#56
Conversation
5f7b63e to
91a1d7d
Compare
6a2b658 to
de1863e
Compare
12ece65 to
d4b5182
Compare
Add tests for the consolidate_deposits function covering: - Early return when no funds to consolidate - Early return when task already active (guard) - Early return when fetching blockhash fails - Single consolidation request with event assertions - Multiple consolidation batches (11 accounts → 2 batches) Also adds canister_self() to CanisterRuntime trait to enable mocking in tests, and extracts MAX_TRANSFERS_PER_CONSOLIDATION constant for test visibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Record `ConsolidatedDeposits` and `SubmittedTransaction` events before calling `submit_transaction` to ensure they persist for resubmission even if the RPC call fails. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add timer task that resubmits all pending transactions every 60 seconds with a fresh blockhash. Uses ResubmittedTransaction event to track the signature change and updated slot. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add tests for the resubmit_transactions function covering: - Early return when no transactions to resubmit - Early return when task already active (guard) - Early return when fetching current slot fails - No resubmission when transaction not expired - Single expired transaction resubmission with event assertions - Event recorded even when submission fails 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
e2f05f1 to
39855fc
Compare
There was a problem hiding this comment.
Pull request overview
Adds an automated resubmission loop for Solana transactions that have expired due to blockhash age, wiring it into the canister’s timer infrastructure.
Changes:
- Introduce a
resubmitmodule with logic to re-sign expired submitted transactions using a fresh recent blockhash and submit them again. - Add a new timer (
RESUBMIT_TRANSACTIONS_DELAY) to periodically run the resubmission task guarded byTimerGuard. - Extend state/task plumbing for the new timer task and add unit tests around resubmission behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
minter/src/resubmit/mod.rs |
New resubmission timer task implementation (fetch slot/blockhash, batch, re-sign, submit, audit event). |
minter/src/resubmit/tests.rs |
New unit tests for early-return paths and single-transaction resubmission behavior. |
minter/src/state/mod.rs |
Expose submitted_transactions() accessor and add TaskType::ResubmitTransactions. |
minter/src/main.rs |
Register new timer interval to invoke resubmit_transactions. |
minter/src/lib.rs |
Export pub mod resubmit. |
minter/src/test_fixtures/mod.rs |
Align MINTER_ACCOUNT owner with the test runtime canister id. |
minter/src/sol_transfer/mod.rs |
Minor test module placement adjustment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let current_slot = match get_slot(&runtime).await { | ||
| Ok(slot) => slot, | ||
| Err(e) => { | ||
| log!(Priority::Info, "Failed to get current slot: {e}"); |
There was a problem hiding this comment.
Do we want to introduce the Error priority for such cases?
There was a problem hiding this comment.
I'm not sure. IMO this is expected to happen once in a while (e.g. due to provider or connectivity issues) so it's not really an error per se. I would reserve the error priority for something that would need to be looked at. WDYT?
| .collect::<Vec<_>>() | ||
| }); | ||
|
|
||
| while !expired_transactions.is_empty() { |
There was a problem hiding this comment.
Similarly to your comment in PR #48, don't we want to limit the number of resubmitted transactions?
There was a problem hiding this comment.
As discussed offline, I think we can assume that we don't run out of cycles, as long as we don't submit too many transactions in parallel.
| return; | ||
| } | ||
| }; | ||
| let new_slot = match get_slot(&runtime).await { |
There was a problem hiding this comment.
Once estimate_recent_blockhash is extended to return also the slot, we will refactor this to single call for (new_slot, new_blockhash), is that correct? In that case, maybe add a TODO?
Rename `resubmit` to `monitor`, `resubmit_transactions` to `monitor_submitted_transactions`, and `ResubmitTransactions` to `MonitorSubmittedTransactions`. No logic changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nsactions-v2 # Conflicts: # minter/src/consolidate/mod.rs # minter/src/consolidate/tests.rs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn minter_address() -> solana_address::Address { | ||
| use crate::state::SchnorrPublicKey; | ||
| let master_key = SchnorrPublicKey { | ||
| public_key: PublicKey::pocketic_key(PocketIcMasterPublicKeyId::Key1), | ||
| chain_code: [1; 32], | ||
| }; | ||
| derive_public_key(&master_key, vec![]) | ||
| .serialize_raw() | ||
| .into() | ||
| } | ||
|
|
||
| fn add_submitted_transaction(signature: Signature, slot: Slot) { | ||
| let message = Message::new_with_blockhash(&[], Some(&minter_address()), &Hash::default()); | ||
| mutate_state(|state| { | ||
| process_event( | ||
| state, | ||
| EventType::SubmittedTransaction { | ||
| signature, | ||
| transaction: message, | ||
| signers: vec![MINTER_ACCOUNT], |
There was a problem hiding this comment.
The test helper builds a Message whose fee payer is minter_address() derived from an empty derivation path, but the signers recorded in the SubmittedTransaction event are vec![MINTER_ACCOUNT] (which uses derivation_path(&MINTER_ACCOUNT) in production code). This mismatch means the test isn’t constructing a transaction message that corresponds to the signer derivation paths used by resubmit_transaction_with_new_blockhash, so it can’t catch bugs where re-signing produces a signature that doesn’t match the message’s fee payer/signers. Consider deriving the fee payer address from the same master key and derivation_path(&MINTER_ACCOUNT) (or pulling the master key from state) so the message and signers are consistent.
(DEFI-2670) Add timer task that resubmits all expired submitted transactions with a new blockhash.
Deterministic integration tests similar to the ones for deposit consolidation task added in #60 will be added in a follow-up PR.