Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
b574ccf
docs(rs-platform-wallet): test case specification for e2e harness
lklimek Apr 29, 2026
085734a
feat(rs-platform-wallet): public accessors for fee, sync watermark, i…
lklimek Apr 29, 2026
5dbbb8d
feat(rs-platform-wallet/e2e): identity signer + register_identity_fro…
lklimek Apr 29, 2026
66ca7ee
feat(rs-platform-wallet/e2e): setup_with_n_identities helper
lklimek Apr 29, 2026
e7b885c
feat(rs-platform-wallet/e2e): test-only utility helpers
lklimek Apr 29, 2026
471e399
style(rs-platform-wallet/e2e): rustfmt break-up in lookup_identity_se…
lklimek Apr 29, 2026
4e3a4b1
refactor(rs-platform-wallet/e2e): use dash_sdk address_inputs helpers…
lklimek Apr 29, 2026
491f65c
fix(rs-platform-wallet/e2e): rename DEFAULT_ACCOUNT_INDEX → _PUB afte…
lklimek Apr 29, 2026
7546a1e
Merge commit 'ffe107cc2c1fe5a09638b1e1b0337c00839c3d60' into worktree…
lklimek Apr 30, 2026
4a04786
Merge remote-tracking branch 'origin/feat/rs-platform-wallet-e2e' int…
lklimek Apr 30, 2026
895fa93
docs(rs-platform-wallet/e2e): expand TEST_SPEC with edge-case coverag…
lklimek Apr 30, 2026
5015e65
docs(rs-platform-wallet/e2e): add Found-NNN bug-pin cases from src audit
lklimek Apr 30, 2026
511d28c
docs(rs-platform-wallet/e2e): pin Found-019 — SeedBackedIdentitySigne…
lklimek Apr 30, 2026
2dea99c
fix(rs-platform-wallet/e2e): correct MultiIdentitySetupGuard cleanup …
lklimek May 4, 2026
000d645
fix(rs-platform-wallet/e2e): refresh wallet state after identity setu…
lklimek May 4, 2026
29598fd
fix(rs-platform-wallet): rename fee_paid -> estimated_min_fee with ho…
lklimek May 4, 2026
361d771
fix(rs-platform-wallet): correct fee comment about default strategy […
lklimek May 4, 2026
a2371a7
fix(rs-platform-wallet/e2e): branch on key type for identity-key sign…
lklimek May 4, 2026
ee895d0
fix(rs-platform-wallet): correct merge-fee comment to match append-su…
lklimek May 4, 2026
696ae3d
fix(rs-platform-wallet/e2e): cap wait sleeps against deadline [CMT-007]
lklimek May 4, 2026
d18fd4d
Merge branch 'feat/rs-platform-wallet-e2e' into feat/rs-platform-wall…
lklimek May 4, 2026
140a028
docs(rs-platform-wallet): clarify sync_watermark None semantics inclu…
lklimek May 4, 2026
18cebcc
docs(rs-platform-wallet/e2e): correct register_identity_from_addresse…
lklimek May 4, 2026
96c5177
docs(rs-platform-wallet/e2e): correct transfer_capturing_st_bytes byt…
lklimek May 4, 2026
b112616
fix(rs-platform-wallet/e2e): Σ-balanced explicit inputs in transfer_c…
lklimek May 4, 2026
d5fdc70
fix(rs-platform-wallet/e2e): filter sub-floor inputs and skip unecono…
lklimek May 4, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions packages/rs-platform-wallet/src/changeset/changeset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,36 @@ pub struct PlatformAddressChangeSet {
/// Last block height with recent address changes (compaction marker).
/// `None` means "no change".
pub last_known_recent_block: Option<u64>,
/// Lower-bound static fee estimate for the transfer that produced
/// this changeset, in credits. `0` for changesets not produced by
/// `transfer()` (e.g. sync-only changesets). See
/// [`Self::estimated_min_fee`].
pub fee: Credits,
}

impl PlatformAddressChangeSet {
/// Lower-bound static fee estimate for the transfer that produced
/// this changeset, in credits.
///
/// Returns `0` for changesets that didn't originate from a
/// `transfer()` call — e.g. sync-only changesets, or changesets
/// constructed via `Default::default()`. The value is the raw
/// `AddressFundsTransferTransition::estimate_min_fee(input_count,
/// output_count, version)` result captured at submit time — it is
/// **NOT** the actual on-chain fee and is **NOT** adjusted by the
/// `fee_strategy`.
///
/// `estimate_min_fee` only models the static
/// `state_transition_min_fees` floor; chain-time fees include
/// storage + processing costs that scale with the operation set
/// (~6.5M static vs ~14.94M observed real for 1in/1out at the time
/// of writing). Tests asserting on the actual chain-time debit
/// must read the post-broadcast balance delta directly, not this
/// value. See platform issue #3040 for the open ticket on
/// upgrading `estimate_min_fee` to a chain-time-accurate estimate.
pub fn estimated_min_fee(&self) -> Credits {
self.fee
}
}
Comment thread
lklimek marked this conversation as resolved.
Comment thread
lklimek marked this conversation as resolved.

impl Merge for PlatformAddressChangeSet {
Expand All @@ -606,13 +636,20 @@ impl Merge for PlatformAddressChangeSet {
.map_or(r, |existing| existing.max(r)),
);
}
// Fee: append-sum via `saturating_add`. Sync-only merges
// (`fee == 0`) are a no-op so a transfer's recorded fee
// survives untouched; merging two transfer changesets sums
// the per-operation fees so the merged total reflects the
// "total fee paid across operations in this batch" intent.
self.fee = self.fee.saturating_add(other.fee);
Comment on lines 585 to +644
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: fee field name, estimated_min_fee() accessor doc, and merge() semantics are mutually inconsistent

Three layers disagree on what this number means:

  1. Field (line 589): pub fee: Credits — public, named fee, suggesting "the fee for this transfer".
  2. Accessor doc (lines 593–614): explicitly warns this is a state_transition_min_fees lower-bound floor only — NOT the on-chain fee, NOT adjusted by fee_strategy, ~6.5M static vs ~14.94M observed for 1in/1out. Phrased per-transfer ("the transfer that produced this changeset", singular).
  3. Merge (lines 639–644): self.fee = self.fee.saturating_add(other.fee) with a comment that the intent is "total fee paid across operations in this batch".

A caller reading only the field name reasonably writes a balance-delta assertion against cs.fee, expecting the actual debit. A caller reading the accessor doc reasonably treats estimated_min_fee() as a single-transfer estimate. Either of those callers, if a merge() runs in between, gets a value that means neither — it is the sum of static lower-bound estimates across N transfers in the batch.

Fix one of:

  • Rename the field to match the accessor (pub estimated_min_fee: Credits) and rewrite the merge comment + accessor doc to consistently describe "sum of static lower-bound fee estimates across all transfers contributing to this changeset".
  • Make the field pub(crate) so all reads go through the accessor, and clarify in the accessor doc that the value is per-batch (post-merge) rather than per-transfer.

source: ['claude-rust-quality', 'codex-general']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] lines 585-644: `fee` field name, `estimated_min_fee()` accessor doc, and `merge()` semantics are mutually inconsistent
  Three layers disagree on what this number means:

1. Field (line 589): `pub fee: Credits` — public, named `fee`, suggesting "the fee for this transfer".
2. Accessor doc (lines 593–614): explicitly warns this is a `state_transition_min_fees` lower-bound floor only — NOT the on-chain fee, NOT adjusted by `fee_strategy`, ~6.5M static vs ~14.94M observed for 1in/1out. Phrased per-transfer ("the transfer that produced this changeset", singular).
3. Merge (lines 639–644): `self.fee = self.fee.saturating_add(other.fee)` with a comment that the intent is "total fee paid across operations in this batch".

A caller reading only the field name reasonably writes a balance-delta assertion against `cs.fee`, expecting the actual debit. A caller reading the accessor doc reasonably treats `estimated_min_fee()` as a single-transfer estimate. Either of those callers, if a `merge()` runs in between, gets a value that means neither — it is the sum of static lower-bound estimates across N transfers in the batch.

Fix one of:
- Rename the field to match the accessor (`pub estimated_min_fee: Credits`) and rewrite the merge comment + accessor doc to consistently describe "sum of static lower-bound fee estimates across all transfers contributing to this changeset".
- Make the field `pub(crate)` so all reads go through the accessor, and clarify in the accessor doc that the value is per-batch (post-merge) rather than per-transfer.

}

fn is_empty(&self) -> bool {
self.addresses.is_empty()
&& self.sync_height.is_none()
&& self.sync_timestamp.is_none()
&& self.last_known_recent_block.is_none()
&& self.fee == 0
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,20 @@ impl IdentityManager {
.sum::<usize>()
}

/// Snapshot of every managed identity's `Identifier` across both
/// buckets. Order is unspecified — callers that need a stable
/// order should sort the returned `Vec`.
pub fn identity_ids(&self) -> Vec<Identifier> {
let mut out: Vec<Identifier> = Vec::with_capacity(self.identity_count());
out.extend(self.out_of_wallet_identities.keys().copied());
for inner in self.wallet_identities.values() {
for managed in inner.values() {
out.push(managed.identity.id());
}
}
out
}
Comment thread
lklimek marked this conversation as resolved.

/// `true` iff both buckets are empty.
pub fn is_empty(&self) -> bool {
self.out_of_wallet_identities.is_empty() && self.wallet_identities.is_empty()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,18 @@ impl PlatformPaymentAddressProvider {
self.last_known_recent_block = result.last_known_recent_block;
}

/// Current `last_known_recent_block` watermark.
///
/// Read-only mirror of the field used by the trait
/// implementation; exposed `pub` so wallet-level helpers
/// (notably [`super::wallet::PlatformAddressWallet::sync_watermark`])
/// can return the value to callers without going through the
/// `AddressProvider` trait. Monotonic non-decreasing across
/// `sync_finished` calls.
pub fn last_known_recent_block(&self) -> u64 {
self.last_known_recent_block
}

/// Restore incremental-sync watermark from persisted state.
pub(crate) fn set_stored_sync_state(
&mut self,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,32 +45,44 @@ impl PlatformAddressWallet {

let version = platform_version.unwrap_or(LATEST_PLATFORM_VERSION);

let address_infos = match input_selection {
// Capture (input_count, output_count) so we can compute the
// fee paid after broadcast for `PlatformAddressChangeSet::fee`.
// The output map is consumed by the SDK call below; the
// input map is materialized (`Auto`) or is the caller's
// (`Explicit*`).
let output_count = outputs.len();
let (address_infos, input_count) = match input_selection {
InputSelection::Explicit(inputs) => {
if inputs.is_empty() {
return Err(PlatformWalletError::AddressOperation(
"Transfer requires at least one input address".to_string(),
));
}
self.sdk
let n = inputs.len();
let infos = self
.sdk
.transfer_address_funds(inputs, outputs, fee_strategy, address_signer, None)
.await?
.await?;
(infos, n)
}
InputSelection::ExplicitWithNonces(inputs) => {
if inputs.is_empty() {
return Err(PlatformWalletError::AddressOperation(
"Transfer requires at least one input address".to_string(),
));
}
self.sdk
let n = inputs.len();
let infos = self
.sdk
.transfer_address_funds_with_nonce(
inputs,
outputs,
fee_strategy,
address_signer,
None,
)
.await?
.await?;
(infos, n)
}
InputSelection::Auto => {
// Auto-select supports `[DeductFromInput(0)]` and
Expand All @@ -89,12 +101,27 @@ impl PlatformAddressWallet {
let inputs = self
.auto_select_inputs(account_index, &outputs, &fee_strategy, version)
.await?;
self.sdk
let n = inputs.len();
let infos = self
.sdk
.transfer_address_funds(inputs, outputs, fee_strategy, address_signer, None)
.await?
.await?;
(infos, n)
}
};

// Lower-bound static estimate from `estimate_min_fee` —
// captures the `state_transition_min_fees` floor only, with
// no adjustment for the chosen `fee_strategy`. This crate
// ships transfers under both `[ReduceOutput(0)]` (the
// wallet-factory default) and `[DeductFromInput(0)]`; either
// way the chain-time fee scales with storage + processing
// costs and is typically larger than this value (see
// `PlatformAddressChangeSet::estimated_min_fee` for the
// honest doc and platform issue #3040).
let fee_paid =
AddressFundsTransferTransition::estimate_min_fee(input_count, output_count, version);

// Get the cached key source from the unified provider for gap
// limit maintenance.
let key_source = {
Expand All @@ -106,7 +133,10 @@ impl PlatformAddressWallet {

// Update balances in the ManagedPlatformAccount.
let mut wm = self.wallet_manager.write().await;
let mut cs = PlatformAddressChangeSet::default();
let mut cs = PlatformAddressChangeSet {
fee: fee_paid,
..Default::default()
};
if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) {
if let Some(account) = info
.core_wallet
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,28 @@ impl PlatformAddressWallet {
.unwrap_or_default()
}

/// Read the current incremental-sync watermark from the unified
/// platform-address provider.
///
/// Returns `None` when the provider hasn't been initialised yet
/// (no [`Self::initialize`] call) or when the provider has no stored
/// watermark (whether restored via [`Self::apply_sync_state`] or
/// produced by a previous sync). The value is monotonic non-decreasing
/// across [`Self::sync_balances`](super::sync) calls against the
/// same chain — a later sync can only advance the watermark, never
/// roll it back. A zero-valued watermark is reported as `None` to
/// match the "no stored watermark" convention used elsewhere in
/// the wallet (see [`Self::apply_sync_state`]).
Comment thread
lklimek marked this conversation as resolved.
pub async fn sync_watermark(&self) -> Option<u64> {
let guard = self.provider.read().await;
let raw = guard.as_ref().map(|p| p.last_known_recent_block())?;
if raw == 0 {
None
} else {
Some(raw)
}
}

/// Get total platform credits across all addresses.
///
/// Returns the sum of all cached balances.
Expand Down
Loading
Loading