Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
104 changes: 102 additions & 2 deletions src/backend_task/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,29 @@ use dash_sdk::platform::{
};
use dash_sdk::query_types::IndexMap;

/// Returns the first identifier in `identifiers` that appears earlier in the
/// same slice, or `None` if every entry is unique.
///
/// Used as a task-boundary duplicate guard for [`ContractTask::FetchContracts`].
fn first_duplicate_id(identifiers: &[Identifier]) -> Option<Identifier> {
identifiers
.iter()
.enumerate()
.find_map(|(index, id)| identifiers[..index].contains(id).then_some(*id))
}

/// Returns the first identifier in `requested` that is also present in
/// `existing`, or `None` when none of the requested IDs are already loaded.
fn first_already_loaded_id(
requested: &[Identifier],
existing: &[Identifier],
) -> Option<Identifier> {
requested
.iter()
.find(|identifier| existing.contains(identifier))
.copied()
}

#[derive(Debug, Clone, PartialEq)]
pub enum ContractTask {
FetchContracts(Vec<Identifier>),
Expand All @@ -47,6 +70,28 @@ impl AppContext {
) -> Result<BackendTaskSuccessResult, crate::backend_task::error::TaskError> {
match task {
ContractTask::FetchContracts(identifiers) => {
// Task-boundary duplicate / already-loaded enforcement.
//
// The `AddContractsScreen` UI also performs these checks, but
// duplicating them here protects non-UI callers (e.g. MCP tools)
// and closes the TOCTOU window between the screen check and
// the network fetch / persistence below.
if let Some(duplicate) = first_duplicate_id(&identifiers) {
return Err(
crate::backend_task::error::TaskError::DuplicateContractInRequest {
contract_id: duplicate,
},
);
}
let existing_ids = self.loaded_contract_ids()?;
if let Some(already_loaded) = first_already_loaded_id(&identifiers, &existing_ids) {
return Err(
crate::backend_task::error::TaskError::ContractAlreadyLoaded {
contract_id: already_loaded,
},
);
}

match DataContract::fetch_many(sdk, identifiers).await {
Ok(data_contracts) => {
let mut results = vec![];
Expand Down Expand Up @@ -208,9 +253,30 @@ impl AppContext {
}
ContractTask::RemoveContract(identifier) => self
.remove_contract(&identifier)
.map(|_| BackendTaskSuccessResult::RemovedContract)
.map_err(crate::backend_task::error::TaskError::from),
.map(|_| BackendTaskSuccessResult::RemovedContract),
ContractTask::SaveDataContract(data_contract, alias, insert_tokens_too) => {
// Task-boundary enforcement: the local DB layer silently
// skips system contract IDs and `INSERT OR IGNORE` makes a
// duplicate user-contract insert a no-op, so without this
// check non-UI callers could appear to "save" a contract
// when the database state did not actually change.
let contract_id = data_contract.id();
if self.is_system_contract_id(&contract_id) {
return Err(
crate::backend_task::error::TaskError::SystemContractImmutable {
contract_id,
},
);
}
let existing_ids = self.loaded_contract_ids()?;
if existing_ids.contains(&contract_id) {
return Err(
crate::backend_task::error::TaskError::ContractAlreadyLoaded {
contract_id,
},
);
}
Comment on lines +271 to +278

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🔴 Blocking: SaveDataContract guard breaks importing a new token from an already-loaded contract

The new SaveDataContract guard rejects every contract ID returned by loaded_contract_ids() before calling insert_contract_if_not_exists. AddTokenByIdScreen (src/ui/tokens/add_token_by_id_screen.rs:147) dispatches SaveDataContract(contract, None, SomeTokensShouldBeAdded(...)) precisely to add a token to an existing contract — the DB helper handles this case correctly via INSERT OR IGNORE on the contract row followed by token insertion at src/database/contracts.rs:59–93. With this guard in place, importing a token from a contract the user already has saved now errors with ContractAlreadyLoaded and the token is never inserted. The guard should not fire when the caller passed InsertTokensToo::SomeTokensShouldBeAdded (or, more generally, when there is still token work to do on an existing contract row).

Suggested change
let existing_ids = self.loaded_contract_ids()?;
if existing_ids.contains(&contract_id) {
return Err(
crate::backend_task::error::TaskError::ContractAlreadyLoaded {
contract_id,
},
);
}
let existing_ids = self.loaded_contract_ids()?;
if existing_ids.contains(&contract_id)
&& !matches!(
&insert_tokens_too,
InsertTokensToo::SomeTokensShouldBeAdded(_)
)
{
return Err(
crate::backend_task::error::TaskError::ContractAlreadyLoaded {
contract_id,
},
);
}

source: ['codex']


self.db.insert_contract_if_not_exists(
&data_contract,
alias.as_deref(),
Expand All @@ -222,3 +288,37 @@ impl AppContext {
}
}
}

#[cfg(test)]
mod tests {
use super::*;

fn id(byte: u8) -> Identifier {
Identifier::from_bytes(&[byte; 32]).expect("32 bytes is a valid identifier")
}

#[test]
fn first_duplicate_id_returns_none_for_unique_inputs() {
assert!(first_duplicate_id(&[id(1), id(2), id(3)]).is_none());
}

#[test]
fn first_duplicate_id_returns_first_repeat() {
assert_eq!(
first_duplicate_id(&[id(1), id(2), id(1), id(3)]),
Some(id(1))
);
}

#[test]
fn first_already_loaded_id_returns_first_match_in_request_order() {
let requested = [id(7), id(2), id(9)];
let existing = [id(9), id(2)];
assert_eq!(first_already_loaded_id(&requested, &existing), Some(id(2)));
}

#[test]
fn first_already_loaded_id_returns_none_when_no_overlap() {
assert!(first_already_loaded_id(&[id(1), id(2)], &[id(3), id(4)]).is_none());
}
}
38 changes: 38 additions & 0 deletions src/backend_task/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use dash_sdk::dpp::consensus::state::state_error::StateError;
use dash_sdk::dpp::dashcore;
use dash_sdk::dpp::dashcore::Network;
use dash_sdk::dpp::platform_value::string_encoding::Encoding;
use dash_sdk::dpp::prelude::Identifier;
use thiserror::Error;

/// Dash Core RPC error code: wallet file not specified (multi-wallet node).
Expand Down Expand Up @@ -458,6 +459,43 @@ pub enum TaskError {
)]
DataContractNotFound,

/// A user-driven mutation targeted a built-in system contract. System
/// contracts (DPNS, DashPay, withdrawals, token history, keyword search)
/// are managed by the application and cannot be modified or removed.
#[error(
"Contract {contract_id} is a built-in system contract and cannot be modified or removed. \
Use a different contract."
)]
SystemContractImmutable {
/// Identifier of the system contract whose mutation was rejected.
/// Rendered as Base58 in the user-facing message via `Identifier`'s
/// `Display` implementation.
contract_id: Identifier,
},

/// The same contract identifier appeared more than once in a single
/// add-contracts request.
#[error(
"Contract {contract_id} was entered more than once. Remove the duplicate entry before adding contracts."
)]
DuplicateContractInRequest {
/// Identifier of the duplicated contract. Rendered as Base58 in the
/// user-facing message via `Identifier`'s `Display` implementation.
contract_id: Identifier,
},

/// An add-contracts request referenced a contract that is already loaded
/// (either persisted in the local database or one of the built-in system
/// contracts).
#[error(
"Contract {contract_id} is already loaded. Select it from the existing contracts list or enter a different contract ID."
)]
ContractAlreadyLoaded {
/// Identifier of the already-loaded contract. Rendered as Base58 in
/// the user-facing message via `Identifier`'s `Display` implementation.
contract_id: Identifier,
},
Comment on lines +462 to +497

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

🟡 Suggestion: New TaskError variants store Identifier as Base58 String, erasing the typed value

SystemContractImmutable, DuplicateContractInRequest, and ContractAlreadyLoaded each carry contract_id: String. Every construction site (contract_token_db.rs:304/320, contract.rs:83/91/268/276) re-runs identifier.to_string(Encoding::Base58), so the typed Identifier is erased before the variant is built. Downstream consumers (future MCP tools, scripting, structural matching) cannot recover the original ID. Storing Identifier directly and formatting via Display in the #[error(...)] template (or a small wrapper) preserves the type, centralises Base58 encoding, and keeps the user-visible string identical. Storing typed IDs is consistent with the project rule allowing Base58 IDs in user-facing messages — the concern is the type erasure, not the encoding.

source: ['claude']

🤖 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 `src/backend_task/error.rs`:
- [SUGGESTION] lines 461-492: New TaskError variants store Identifier as Base58 String, erasing the typed value
  SystemContractImmutable, DuplicateContractInRequest, and ContractAlreadyLoaded each carry contract_id: String. Every construction site (contract_token_db.rs:304/320, contract.rs:83/91/268/276) re-runs identifier.to_string(Encoding::Base58), so the typed Identifier is erased before the variant is built. Downstream consumers (future MCP tools, scripting, structural matching) cannot recover the original ID. Storing Identifier directly and formatting via Display in the #[error(...)] template (or a small wrapper) preserves the type, centralises Base58 encoding, and keeps the user-visible string identical. Storing typed IDs is consistent with the project rule allowing Base58 IDs in user-facing messages — the concern is the type erasure, not the encoding.


// ──────────────────────────────────────────────────────────────────────────
// Serialization errors
// ──────────────────────────────────────────────────────────────────────────
Expand Down
7 changes: 2 additions & 5 deletions src/backend_task/update_data_contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,7 @@ impl AppContext {

match state_transition.broadcast_and_wait(sdk, None).await {
Ok(returned_contract) => {
self.db
.replace_contract(data_contract.id(), &returned_contract, self)?;
self.replace_contract(data_contract.id(), &returned_contract)?;
let fee_result = FeeResult::new(estimated_fee, estimated_fee);
Ok(BackendTaskSuccessResult::UpdatedContract(fee_result))
}
Expand Down Expand Up @@ -148,9 +147,7 @@ impl AppContext {
_ => sleep(Duration::from_secs(10)).await,
}
if let Ok(Some(contract)) = DataContract::fetch(sdk, id).await {
self.db
.replace_contract(contract.id(), &contract, self)
.ok();
self.replace_contract(contract.id(), &contract)?;

return Ok(BackendTaskSuccessResult::ContractSavedAfterProofError);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Expand Down
Loading
Loading