refactor: improve tx waiting, fees, and compatibility

[ethan-crypto]

Summary

This PR improves transaction submission behavior, fee handling, and runtime compatibility checks across the Quantus CLI.

The main goal is to make transaction flow semantics clearer and safer:

• sends can now return immediately after submission by default, or wait for inclusion/finalization when requested
• amount and tip parsing now uses exact decimal handling based on chain decimals
• fee and balance checks are more accurate for both single-send and batch flows
• compatibility checks now require both runtime spec_version and transaction_version to match supported values

What changed

Transaction lifecycle / waiting behavior

• Clarifies transaction execution semantics across CLI and library usage
• Distinguishes between:
  • submitted
  • included
  • finalized
• Improves post-submit behavior so commands do not imply stronger confirmation than what was actually awaited
• Updates docs/examples to reflect the default submit-and-return flow, plus --wait-for-transaction and --finalized-tx

Fee estimation and balance safety

• Improves transfer safety by validating balances against the actual amount, tip, and estimated fee requirements
• Adds better fee-aware checks for batch transfers
• Falls back safely when fee estimation is unavailable instead of failing in a misleading way
• Improves error messages so users can see how much was needed vs available

Exact amount / tip parsing

• Tightens parsing for --amount and --tip using exact decimal conversion based on configured chain decimals
• Rejects malformed, negative, over-precise, or effectively-zero values instead of silently accepting bad input

Batch transfer behavior

• Clarifies that batch JSON file amounts are raw smallest-unit integers
• Uses safer accumulation / overflow-aware accounting for total batch amounts
• Aligns batch command logging and success reporting with the new transaction stage model

Compatibility and metadata regeneration

• Tightens compatibility checks to require a supported (spec_version, transaction_version) pair
• Updates regeneration guidance to remind maintainers to refresh the compatibility pair after metadata updates
• Refreshes README / library usage docs to match the new behavior

Cleanup

• Removes unnecessary wallet debug output
• Rejects duplicate developer wallet creation paths

[n13]

Comment: We definitely need to fix #4 below - there should be no default tip. We currently don't expose tips to end users because they're confusing and useless for end users as long as the chain doesn't have congestion. For context - the wallet doesn't have a tip.

I believe tip is something we inherited from substrate, and it has some limited usefulness for the blockchain later on when we have millions of users but for now, it's not needed at all.

TL;DR: Default tip amount should be 0 (zero) on all transactions.

==========================================

AI Review

This is a substantial, well-motivated PR (10 files, +807/-230) that addresses real correctness issues: float-based amount parsing, a hardcoded fee estimate, misleading "confirmed" messages after submit-only, and a spec-version-only compatibility check. The new TransactionStage state machine and exact decimal parser are clear improvements.

That said, I found a few items worth flagging.

---

Issues

1. DRY violation in src/config/mod.rs -- the three constants can desync

The PR introduces three separate constants that encode the same data:

pub const COMPATIBLE_RUNTIME_VERSIONS: &[u32] = &[127];

/// List of transaction versions that this CLI is compatible with.
pub const COMPATIBLE_TRANSACTION_VERSIONS: &[u32] = &[2];

/// Supported runtime / transaction version pairs for the checked-in metadata snapshot.
pub const COMPATIBLE_RUNTIMES: &[CompatibleRuntime] =

Only COMPATIBLE_RUNTIMES is used for the actual check. The other two are only used for display in handle_compatibility_check. If someone adds a new pair to COMPATIBLE_RUNTIMES but forgets to update the flat arrays, the display output will lie. These should be derived from COMPATIBLE_RUNTIMES or the flat arrays should be removed and the display loop should use COMPATIBLE_RUNTIMES everywhere.

2. should_refresh_post_submit_state() is an identity delegate

pub fn should_refresh_post_submit_state(self) -> bool {
    self.should_watch_transaction()
}

This adds a layer of indirection with no behavioral difference. If it's reserving a slot for future logic, a brief doc comment would help -- otherwise it's dead abstraction.

3. build_batch_transfer_call is called twice per batch send

In handle_batch_send_command (batch.rs), build_batch_transfer_call is invoked once for fee estimation, then batch_transfer (send.rs) calls it again for actual submission. Each invocation re-resolves every address via resolve_address. For a batch of 500 transfers this is noticeable. Consider passing the already-built call through, or caching the resolved addresses.

4. Default tip always added to balance check even when user omits --tip

effective_tip_amount returns DEFAULT_PRIORITY_TIP (10 billion units) when no tip is specified. The pre-send balance check uses this, so a user who doesn't pass --tip still needs amount + 10B + fee in their account. This is likely intentional (to prevent "temporarily banned" errors), but it's worth a short note in the CLI help or README so users aren't surprised when a send fails despite seemingly having enough balance.

5. Repeated error string in batch overflow checks

The string "Batch amount total is too large to represent" appears three times across batch.rs. A shared constant or helper (you already have checked_add in send.rs) would be cleaner.

6. InBestBlock execution-success is checked twice when waiting for finalization

When target_stage == Finalized, the loop checks check_execution_success on InBestBlock, then continues. When InFinalizedBlock arrives, it checks check_execution_success again for the same extrinsic. The second check is redundant (a tx that succeeded in a best block won't fail when that block finalizes). Not a bug, but unnecessary RPC call.

7. parse_amount_with_decimals -- rejecting + prefix feels overly strict

The function rejects +1.0 as invalid. Depending on your audience, this could be surprising -- +1.0 is standard decimal notation. If this is intentional to keep the surface area tight, that's fine, but worth a note.

---

Nits / minor

• regenerate_metadata.sh: The switch from cargo +nightly fmt to cargo fmt could break if the generated code uses nightly-only syntax. If the codebase currently requires nightly for formatting, this should stay.
• LIBRARY_USAGE.md version 1.3.3: Is this the actual published crates.io version? If not, it's misleading in a doc example.
• seed_output_probe subprocess test: Creative approach to assert no debug output leaks, but it's fragile (depends on binary path, exact test name). A simpler alternative: just assert the debug println! calls are removed (which this PR already does). The test guards against regression, which is nice -- but it will break if the test binary name changes or test harness evolves.

---

What looks great

• The exact integer decimal parser (parse_amount_with_decimals) is a big step up from float multiplication. The u128 boundary tests are thorough.
• The TransactionStage / WatchedTxEvent / WatchDecision state machine cleanly separates lifecycle semantics from display logic.
• Real fee estimation via estimate_transaction_partial_fee with a graceful fallback (log + proceed) is exactly the right pattern.
• Removing the debug println! in wallet code and adding the no-leak regression test is a good security hygiene move.
• The duplicate developer wallet guard is a straightforward correctness fix.
• set -euo pipefail in the shell script is a welcome improvement.

Overall this is solid work -- the items above are mostly about keeping the codebase DRY and avoiding a few subtle foot-guns.

[n13]

Now I have a complete picture. Here's my review of PR #101.

PR #101 Review: refactor: improve tx waiting, fees, and compatibility

Overall Assessment

Solid, well-motivated work. The author has cleanly addressed every item from the prior review I posted on Apr 24 ([address Nikolaus PR review comments](#commit-889c1c8)), including the core blocker from n13 about removing the default tip. This is ready to merge after a few small follow-ups (none are blockers).

What's already addressed from the previous round

• Default tip is now 0 (n13's blocker). effective_tip_amount(None) → 0, positive_tip_amount(None) → None, and submit_transaction only attaches a tip when Some(>0) is passed. The hardcoded 10_000_000_000 is gone from both transfer_with_nonce and batch_transfer.
• Compatibility constants DRY-ed up. The duplicated COMPATIBLE_RUNTIME_VERSIONS / COMPATIBLE_TRANSACTION_VERSIONS flat arrays were removed. Display in handle_compatibility_check now iterates COMPATIBLE_RUNTIMES directly — single source of truth.
• should_refresh_post_submit_state removed (was identity delegate to should_watch_transaction).
• Batch double-build fixed. build_batch_transfer_call is now built once in handle_batch_send_command and threaded through submit_prebuilt_batch_transfer_call. Address resolution happens once per batch send.
• Repeated batch overflow string extracted to BATCH_AMOUNT_TOTAL_TOO_LARGE + batch_amount_overflow_error() helper.
• Double check_execution_success avoided via execution_success_checked_for: Option<H256> + should_check_execution_success (with unit test).
• +1.0 is now accepted by parse_amount_with_decimals, with new tests for both the happy path and +, ++1, +0 rejection.
• regenerate_metadata.sh keeps cargo +nightly fmt with a comment explaining why. Good call.
• LIBRARY_USAGE.md 1.3.3 example now has a "replace with the latest published version" hint.

Other things I like

• Tip parsing failures no longer silently swallowed. Old code did parse_amount_with_decimals(tip_str, decimals).ok(), hiding malformed tips. New code uses parse_amount(...).await? and propagates the error. Aligns with the project's "fail early" philosophy.
• TransactionStage / WatchedTxEvent / WatchDecision cleanly separates lifecycle semantics from display logic and is easy to test in isolation. Tests cover both successful inclusion/finalization paths and all error variants.
• Real fee estimation via partial_fee_estimate with a graceful "log + proceed" fallback when estimation is unavailable. The pattern handles the "with-tip" vs "no-tip" messaging cleanly.
• u128 boundary tests for parse_amount_with_decimals (u128::MAX / factor + remainder, plus an overflow case) are thorough.
• Removed wallet debug println! calls + IdentifyAccount unused import + serial_test dependency that nothing references anymore.
• Duplicate developer wallet creation guard + the test_developer_wallet_duplicate_rejected test.
• set -euo pipefail in the regen script.
• --finalized-tx doc now says "Implies --wait-for-transaction".

Items worth considering (none are blockers)

1. Inconsistent address resolution between single-send and batch builders

build_transfer_call in src/cli/send.rs (line ~256) assumes the address is already resolved:

fn build_transfer_call(resolved_address: &str, amount: u128) -> Result<impl subxt::tx::Payload> {
	let (to_account_id_sp, _) = SpAccountId32::from_ss58check_with_version(resolved_address)
		.map_err(|e| {
			crate::error::QuantusError::NetworkError(format!("Invalid destination address: {e:?}"))
		})?;

	let to_account_id_bytes: [u8; 32] = *to_account_id_sp.as_ref();
	let to_account_id = subxt::ext::subxt_core::utils::AccountId32::from(to_account_id_bytes);

	Ok(quantus_subxt::api::tx().balances().transfer_allow_death(
		subxt::ext::subxt_core::utils::MultiAddress::Id(to_account_id),
		amount,
	))
}

…while build_batch_transfer_call calls resolve_address itself for each entry (line ~280). They also use different SS58 parsers (from_ss58check_with_version vs from_ss58check). It would be cleaner if both used a single shared helper like resolve_to_subxt_account_id(address) -> Result<AccountId32> that always does resolve_address + SS58 parse + byte conversion. That same helper would also clean up duplication in get_account_data, recovery.rs, multisig.rs, etc., where the same three-line dance appears.

2. Batch path doesn't validate addresses before the balance error

In handle_batch_send_command, validate_batch_transfer_request only checks emptiness and chain limits — it never tries to parse the destination addresses. The first address parse happens later in build_batch_transfer_call, which is after the exact-balance check at line 171. So a user with both insufficient balance AND a typo'd address sees "Insufficient balance", fixes the balance, and only then sees "Invalid destination address". Easy fix: have validate_batch_transfer_request (or a new validate_addresses helper) call resolve_address for each entry up front.

3. Duplicated fee-aware balance check between handle_send_command and handle_batch_send_command

Both follow the same shape (exact_required = amount + tip → estimate_transaction_partial_fee → if balance < estimated_total, build error message with breakdown). The error-message construction is also nearly identical between the two with-tip / no-tip branches. Worth extracting something like:

async fn ensure_balance_covers_call(
    client: &QuantusClient,
    keypair: &QuantumKeyPair,
    call: &impl subxt::tx::Payload,
    balance: u128,
    exact_required: u128,
    submit_tip: Option<u128>,
    label: &str, // e.g. "send" or "batch"
) -> Result<()>

This is the largest remaining DRY violation in the PR.

4. wait_for_transaction || cli.finalized_tx in main.rs is redundant

	let execution_mode = cli::common::ExecutionMode {
		finalized: cli.finalized_tx,
		wait_for_transaction: cli.wait_for_transaction || cli.finalized_tx,
	};

should_watch_transaction() already returns true whenever transaction_stage() != Submitted, which is true if either finalized or wait_for_transaction is set. Setting wait_for_transaction when finalized is set is harmless but redundant. Either drop the || cli.finalized_tx or add a brief comment that it's defensive for any code that reads the field directly.

5. wait_tx_inclusion is getting structurally awkward

The function is now ~140 lines with two different decision paths: the InBestBlock and InFinalizedBlock arms call describe_watched_tx_event inline (because they need different success/failure spinner messages), while every other status maps to a WatchedTxEvent and falls through to the bottom-of-loop dispatcher. Extracting per-status handlers (e.g. handle_in_best_block(spinner, ...) -> ControlFlow<Result<()>>) would make the control flow clearer and the function shorter. The "❌ Transaction failed in block" / "❌ Transaction failed in finalized block" spinner blocks are also nearly identical and could share a helper.

6. resolve_address called twice on the single-send path

handle_send_command resolves to resolved_address, then calls transfer_with_nonce(&resolved_address, ...), which immediately calls resolve_address(to_address) again on the same already-resolved string. Cheap operation (just an SS58 parse round-trip), but worth either dropping one resolve or documenting that transfer_with_nonce is the public boundary that re-resolves for safety.

7. positive_tip_amount(Some(0)) == None is silent coercion

pub(crate) fn positive_tip_amount(tip: Option<u128>) -> Option<u128> {
	tip.filter(|tip_amount| *tip_amount > 0)
}

A library caller passing Some(0) to transfer() will get the same behavior as None. That's the right choice given the "no tip by default" semantics, but the public transfer / transfer_with_nonce doc comments don't mention it. A one-line note ("Pass Some(0) or None to omit a tip") would prevent confusion.

8. Minor: checked_add in send.rs and the inline try_fold(...).ok_or_else(batch_amount_overflow_error)? in batch.rs are doing the same job

The send path uses checked_add(lhs, rhs, "required send balance")? and the batch path uses acc.checked_add(*amount).ok_or_else(batch_amount_overflow_error)?. Both are fine in isolation, but a shared checked_add(lhs, rhs, context: &str) exported from send.rs (or moved to error.rs) would let batch use the same helper.

Nits

• Cargo.lock includes the unrelated rand 0.9.2 → 0.9.4 bump from the *fix cargo audit issue commit. Worth calling out in the PR description so reviewers don't wonder where that came from.
• The 📝 Encoded call size: {} bytes line in submit_transaction uses log_print! (always shown), while almost everything else around it uses log_verbose!. Probably should be verbose.
• src/cli/common.rs: transaction_stage() returns Finalized when finalized: true regardless of wait_for_transaction, so a caller could construct ExecutionMode { finalized: true, wait_for_transaction: false } and it would Just Work — but main.rs ORs them anyway. If you keep both, a doc comment on ExecutionMode explaining "constructors should ensure wait_for_transaction is true whenever finalized is true" would help library users.

Recommendation

LGTM with optional follow-ups. The blocker (default tip) is resolved, all prior review feedback is addressed cleanly, the new state machine + tests give the lifecycle semantics a much firmer foundation, and the bug fixes (overflow-safe accumulation, exact decimal parsing, real fee estimation, correct (spec, tx_version) compat check) are real correctness wins. The remaining items above are about tightening DRY and UX polish — fine as a follow-up PR.

---

Want me to draft inline GitHub review comments for any of items 1–8, or open a separate issue tracking the codebase-wide SS58→subxt AccountId32 duplication? (Per your rules I won't post anything to GitHub without your go-ahead.)

[n13]

GTG as I am concerned

The first item above could maybe be fixed but up to you

[n13]

Agree, let's merge!