fix: identity & token screen safety and validation#563
Conversation
… in Identity Create
…n in query_tokens.rs
📝 WalkthroughWalkthroughConvert several panic/unwrap cases to propagated errors or guarded early returns; add logging for previously ignored DB results; validate signing-key and identity selection in multiple token/UI flows; and enforce detailed validation when converting identity keys to public key maps. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens identity/key handling across the UI and backend by removing panic/unwrap paths, adding user-facing validation for missing signing keys in token flows, and improving resilience/logging around identity/contact persistence.
Changes:
- Add “no signing key selected” validation to multiple token action screens and the token contract creator flow.
- Replace panic/expect-based error handling with
Resultpropagation in identity key mapping and token query construction. - Make identity refresh more resilient to deleted/missing identities and start logging previously-silenced DB errors in identity deletion and DashPay contacts persistence.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/tokens/unfreeze_tokens_screen.rs | Adds selected-key validation before dispatching unfreeze task. |
| src/ui/tokens/transfer_tokens_screen.rs | Adds selected-key validation before dispatching transfer task. |
| src/ui/tokens/tokens_screen/token_creator.rs | Validates identity + signing key before creating token registration tasks. |
| src/ui/tokens/resume_tokens_screen.rs | Adds selected-key validation before dispatching resume task. |
| src/ui/tokens/pause_tokens_screen.rs | Adds selected-key validation before dispatching pause task. |
| src/ui/tokens/mint_tokens_screen.rs | Adds selected-key validation before dispatching mint task. |
| src/ui/tokens/freeze_tokens_screen.rs | Adds selected-key validation before dispatching freeze task. |
| src/ui/tokens/destroy_frozen_funds_screen.rs | Adds selected-key validation before dispatching destroy-frozen-funds task. |
| src/ui/tokens/claim_tokens_screen.rs | Adds selected-key validation before dispatching claim task. |
| src/ui/identities/withdraw_screen.rs | Avoids panic on refresh when identity no longer exists locally. |
| src/ui/identities/transfer_screen.rs | Avoids panic on refresh when identity no longer exists locally; refreshes known identities list. |
| src/ui/identities/identities_screen.rs | Logs DB deletion failures instead of silently ignoring them. |
| src/ui/identities/add_new_identity_screen/mod.rs | Adds ENCRYPTION/DECRYPTION purposes and enforces MEDIUM security level when selected. |
| src/ui/dashpay/contacts_list.rs | Logs DB failures for clear/save contact operations instead of silently ignoring them. |
| src/backend_task/tokens/query_tokens.rs | Replaces expect() on DocumentQuery::new with error propagation. |
| src/backend_task/identity/register_identity.rs | Propagates identity key mapping errors via ?. |
| src/backend_task/identity/mod.rs | Changes to_public_keys_map() to return Result and removes panic on unsupported key types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn confirmation_ok(&mut self) -> AppAction { | ||
| if self.selected_key.is_none() { | ||
| self.error_message = Some("No signing key selected".into()); | ||
| self.status = FreezeTokensStatus::ErrorMessage("No key selected".into()); |
There was a problem hiding this comment.
The displayed error message is sourced from FreezeTokensStatus::ErrorMessage(...), but this branch sets it to "No key selected" while error_message is "No signing key selected". Consider aligning these so the user sees the intended message.
| self.status = FreezeTokensStatus::ErrorMessage("No key selected".into()); | |
| self.status = FreezeTokensStatus::ErrorMessage("No signing key selected".into()); |
| if let Some(refreshed) = self | ||
| .app_context | ||
| .load_local_qualified_identities() | ||
| .unwrap() | ||
| .unwrap_or_default() | ||
| .into_iter() |
There was a problem hiding this comment.
refresh() uses load_local_qualified_identities().unwrap_or_default(), which will silently treat a load failure the same as “no identities” and hide the underlying error. Consider handling the Err case explicitly (e.g., if let Err(e) = ... { tracing::warn!(...); }) so DB/load issues are visible while still avoiding a panic.
|
|
||
| if self.selected_key.is_none() { | ||
| self.error_message = Some("No signing key selected".into()); | ||
| self.status = ResumeTokensStatus::ErrorMessage("No key selected".into()); |
There was a problem hiding this comment.
The displayed error message is sourced from ResumeTokensStatus::ErrorMessage(...), but this branch sets it to "No key selected" while error_message is "No signing key selected". Consider making these consistent (or only setting the rendered one) so the user sees the intended message.
| self.status = ResumeTokensStatus::ErrorMessage("No key selected".into()); | |
| self.status = | |
| ResumeTokensStatus::ErrorMessage("No signing key selected".into()); |
| fn confirmation_ok(&mut self) -> AppAction { | ||
| if self.selected_key.is_none() { | ||
| self.error_message = Some("No signing key selected".into()); | ||
| self.status = MintTokensStatus::ErrorMessage("No key selected".into()); |
There was a problem hiding this comment.
The displayed error message is sourced from MintTokensStatus::ErrorMessage(...), but this branch sets it to "No key selected" while error_message is "No signing key selected". Consider using one consistent message (and/or only setting the field that’s rendered).
| self.status = MintTokensStatus::ErrorMessage("No key selected".into()); | |
| self.status = MintTokensStatus::ErrorMessage("No signing key selected".into()); |
| fn confirmation_ok(&mut self) -> AppAction { | ||
| if self.selected_key.is_none() { | ||
| self.error_message = Some("No signing key selected".into()); | ||
| self.status = DestroyFrozenFundsStatus::ErrorMessage("No key selected".into()); |
There was a problem hiding this comment.
The displayed error message is sourced from DestroyFrozenFundsStatus::ErrorMessage(...), but this branch sets it to "No key selected" while error_message is "No signing key selected". Consider using a single consistent string to avoid ambiguity.
| self.status = DestroyFrozenFundsStatus::ErrorMessage("No key selected".into()); | |
| self.status = DestroyFrozenFundsStatus::ErrorMessage("No signing key selected".into()); |
|
|
||
| if self.selected_key.is_none() { | ||
| self.error_message = Some("No signing key selected".into()); | ||
| self.status = ClaimTokensStatus::ErrorMessage("No key selected".into()); |
There was a problem hiding this comment.
The displayed error message is sourced from ClaimTokensStatus::ErrorMessage(...), but this branch sets it to "No key selected" while error_message is "No signing key selected". Consider making these consistent so the user sees a clear error.
| self.status = ClaimTokensStatus::ErrorMessage("No key selected".into()); | |
| self.status = ClaimTokensStatus::ErrorMessage("No signing key selected".into()); |
| if let Err(e) = self | ||
| .app_context | ||
| .db | ||
| .delete_local_qualified_identity(&identity_id, &self.app_context) | ||
| .ok(); | ||
| { | ||
| tracing::warn!("Failed to delete identity from database: {}", e); | ||
| } |
There was a problem hiding this comment.
The warning log here doesn’t include which identity ID failed to delete, which can make troubleshooting harder (especially if multiple deletions happen). Consider including identity_id (and voter_identity_id below) in the log fields/message.
| let identities = self | ||
| .app_context | ||
| .load_local_qualified_identities() | ||
| .unwrap() | ||
| .into_iter() | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
refresh() uses load_local_qualified_identities().unwrap_or_default(), which discards load errors and can make DB problems hard to diagnose. Consider matching on the result and logging the error before falling back, so “identity deleted” and “load failed” aren’t conflated.
| fn confirmation_ok(&mut self) -> AppAction { | ||
| if self.selected_key.is_none() { | ||
| self.error_message = Some("No signing key selected".into()); | ||
| self.status = UnfreezeTokensStatus::ErrorMessage("No key selected".into()); |
There was a problem hiding this comment.
The UI error text shown to the user comes from UnfreezeTokensStatus::ErrorMessage(...), but this branch sets that message to "No key selected" while error_message is set to "No signing key selected". Consider using a single, consistent string (and/or only setting the field that is actually rendered) to avoid confusing/ambiguous errors.
| self.status = UnfreezeTokensStatus::ErrorMessage("No key selected".into()); | |
| self.status = UnfreezeTokensStatus::ErrorMessage("No signing key selected".into()); |
|
|
||
| if self.selected_key.is_none() { | ||
| self.error_message = Some("No signing key selected".into()); | ||
| self.status = PauseTokensStatus::ErrorMessage("No key selected".into()); |
There was a problem hiding this comment.
The displayed error message is sourced from PauseTokensStatus::ErrorMessage(...), but this branch sets it to "No key selected" while error_message is "No signing key selected". Consider making these consistent (or only setting the rendered one) to avoid confusing UX.
| self.status = PauseTokensStatus::ErrorMessage("No key selected".into()); | |
| self.status = PauseTokensStatus::ErrorMessage("No signing key selected".into()); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/identities/add_new_identity_screen/mod.rs (1)
671-715:⚠️ Potential issue | 🟠 MajorAuto-set ECDSA_SECP256K1 and required contract bounds for ENCRYPTION/DECRYPTION.
When ENCRYPTION or DECRYPTION purposes are selected, the code must auto-set the key type to ECDSA_SECP256K1 and apply SingleContractDocumentType bounds with "contactRequest" document type (per DIP-15 requirements). Currently, these constraints are not enforced, allowing users to register keys with wrong key types or missing bounds, which causes platform validation failures.
🔧 Suggested fix
- for (i, ((key, _), key_type, purpose, security_level, _contract_bounds)) in + for (i, ((key, _), key_type, purpose, security_level, contract_bounds)) in self.identity_keys.keys_input.iter_mut().enumerate() { @@ if *purpose != prev_purpose { match *purpose { Purpose::TRANSFER => { *security_level = SecurityLevel::CRITICAL; } Purpose::ENCRYPTION | Purpose::DECRYPTION => { *security_level = SecurityLevel::MEDIUM; + *key_type = KeyType::ECDSA_SECP256K1; + if contract_bounds.is_none() { + *contract_bounds = Some( + ContractBounds::SingleContractDocumentType { + id: self.app_context.dashpay_contract.id(), + document_type_name: "contactRequest".to_string(), + }, + ); + } }Also applies to lines 744–748 where ENCRYPTION/DECRYPTION security levels are locked to MEDIUM—extend to also prevent key type changes and validate contract bounds exist.
🤖 Fix all issues with AI agents
In `@src/ui/identities/transfer_screen.rs`:
- Around line 504-515: Replace the use of unwrap_or_default() on
self.app_context.load_local_qualified_identities() with explicit Result
handling: call load_local_qualified_identities(), match on Ok(identities) to use
the returned Vec, and on Err(err) log a warning that includes the error (so
failures are diagnosable) and set identities to an empty Vec as a graceful
fallback; then continue using identities to update self.identity,
self.max_amount and self.known_identities as before (referencing
load_local_qualified_identities, identities, self.identity, self.max_amount, and
self.known_identities).
🧹 Nitpick comments (3)
src/ui/tokens/freeze_tokens_screen.rs (2)
253-257: Guard is good, but.unwrap()on line 304 still leaves a latent panic.The early return at lines 253–257 ensures
selected_keyisSomebefore reaching line 304 in the current flow, but a future refactor could break that invariant. Since the PR's explicit goal is eliminating panics, consider using a pattern that doesn't rely on.unwrap():Suggested improvement
At line 304, replace the
.unwrap()with a safe extraction that's consistent with the guard:- signing_key: self.selected_key.clone().unwrap(), + signing_key: match self.selected_key.clone() { + Some(key) => key, + None => { + self.error_message = Some("No signing key selected".into()); + self.status = FreezeTokensStatus::ErrorMessage("No key selected".into()); + return AppAction::None; + } + },Or more concisely, restructure
confirmation_okto bind the key from the guard:- if self.selected_key.is_none() { - self.error_message = Some("No signing key selected".into()); - self.status = FreezeTokensStatus::ErrorMessage("No key selected".into()); - return AppAction::None; - } + let signing_key = match self.selected_key.clone() { + Some(key) => key, + None => { + self.error_message = Some("No signing key selected".into()); + self.status = FreezeTokensStatus::ErrorMessage("No key selected".into()); + return AppAction::None; + } + };Then use
signing_keydirectly at line 304, eliminating the.unwrap()entirely.Based on learnings: "Error handling refactoring is needed across the Dash-EVO-Tool (DET) codebase, particularly to avoid panics with
.expect()and instead propagate errors properly."Also applies to: 304-304
267-272: Sameis_err()+.unwrap()pattern — prefermatchorlet-else.Minor nit, same idea as the key guard above. This can be tightened:
Suggested improvement
- if parsed.is_err() { - self.error_message = Some("Please enter a valid identity ID.".into()); - self.status = FreezeTokensStatus::ErrorMessage("Invalid identity".into()); - return AppAction::None; - } - let freeze_id = parsed.unwrap(); + let freeze_id = match parsed { + Ok(id) => id, + Err(_) => { + self.error_message = Some("Please enter a valid identity ID.".into()); + self.status = FreezeTokensStatus::ErrorMessage("Invalid identity".into()); + return AppAction::None; + } + };src/backend_task/identity/mod.rs (1)
149-219: Add inline tests for new error paths.Since unsupported key types now return
Err, add#[test]coverage to lock this behavior in.💡 Example inline test scaffold
+#[cfg(test)] +mod tests { + use super::*; + use dash_sdk::dpp::key_wallet::bip32::DerivationPath; + + #[test] + fn to_public_keys_map_rejects_unsupported_key_type() { + let keys = IdentityKeys { + master_private_key: None, + master_private_key_type: KeyType::BLS12_381, // unsupported + keys_input: vec![], + }; + assert!(keys.to_public_keys_map().is_err()); + } +}As per coding guidelines: “Unit tests should be inline in source files using
#[test]attribute.”
Audit SummaryReviewed by: Claude Code with a 3-agent team:
Overall AssessmentThe PR is well-structured and achieves its stated goals. It systematically replaces 10+ panic/unwrap/expect paths with proper error handling across 17 files. No critical issues found. The changes significantly improve application resilience. Cross-checked against PR #562 ( Findings
Pre-existing Issues (outside diff, for awareness)
Positive Observations
|
…er screen refresh Log identity-load failures with tracing::warn instead of silently swallowing them via unwrap_or_default(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/ui/identities/transfer_screen.rs`:
- Around line 82-85: The constructor TransferScreen::new currently calls
app_context.load_local_qualified_identities().expect("Identities not loaded")
which can still panic; change new (the TransferScreen::new function) to avoid
expect by either returning a Result<TransferScreen, Error> and propagating the
load error (use ? on load_local_qualified_identities), or handle the error
non-panically by logging the failure and using a safe default (e.g., empty
known_identities Vec) before constructing Self — mirror the non-panicking
approach used in refresh() to keep behavior consistent.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/ui/identities/transfer_screen.rs`: - Around line 82-85: The constructor TransferScreen::new currently calls app_context.load_local_qualified_identities().expect("Identities not loaded") which can still panic; change new (the TransferScreen::new function) to avoid expect by either returning a Result<TransferScreen, Error> and propagating the load error (use ? on load_local_qualified_identities), or handle the error non-panically by logging the failure and using a safe default (e.g., empty known_identities Vec) before constructing Self — mirror the non-panicking approach used in refresh() to keep behavior consistent.src/ui/identities/transfer_screen.rs (1)
82-85: Pre-existingexpect()innew()remains.
load_local_qualified_identities().expect("Identities not loaded")on Line 85 can still panic. This is outside the current diff, but given the PR's goal of replacing panic paths, it's worth noting for a follow-up — especially since the same pattern was already fixed inrefresh().🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/ui/identities/transfer_screen.rs` around lines 82 - 85, The constructor TransferScreen::new currently calls app_context.load_local_qualified_identities().expect("Identities not loaded") which can still panic; change new (the TransferScreen::new function) to avoid expect by either returning a Result<TransferScreen, Error> and propagating the load error (use ? on load_local_qualified_identities), or handle the error non-panically by logging the failure and using a safe default (e.g., empty known_identities Vec) before constructing Self — mirror the non-panicking approach used in refresh() to keep behavior consistent.
- Replace guard-then-unwrap with match/let-else in all 8 token screens - Move identity UI removal after successful DB deletion - Add backend security level validation for identity key purposes - Show user-facing error message on identity deletion failure
- Replace guard-then-unwrap with match/let-else in all 8 token screens - Move identity UI removal after successful DB deletion - Add backend security level validation for identity key purposes - Show user-facing error message on identity deletion failure Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/ui/tokens/pause_tokens_screen.rs (1)
96-115:⚠️ Potential issue | 🟡 MinorPre-existing copy-paste: error messages say "Burning" / "burn" instead of "Pausing" / "pause".
Lines 98, 106, and 114 reference "Burning" and "burn" but this is the Pause screen. These appear to have been copied from
destroy_tokens_screen.rsand never updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/pause_tokens_screen.rs` around lines 96 - 115, Error messages in the pause_tokens_screen logic still refer to "Burning"/"burn" instead of "Pausing"/"pause"; update the strings in the match arms for AuthorizedActionTakers::NoOne, ::ContractOwner, and ::Identity(identifier) so they say "Pausing is not allowed on this token", "You are not allowed to pause this token. Only the contract owner is.", and "You are not allowed to pause this token" respectively, modifying the error_message assignments that reference identity_token_info and AuthorizedActionTakers to use the correct wording and casing.src/ui/tokens/resume_tokens_screen.rs (1)
96-115:⚠️ Potential issue | 🟡 MinorPre-existing copy-paste: error messages say "Burning" / "burn" instead of "Resuming" / "resume".
Same issue as
pause_tokens_screen.rs— Lines 97, 105, and 113 reference "Burning" / "burn" but this is the Resume screen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/resume_tokens_screen.rs` around lines 96 - 115, The error strings in the AuthorizedActionTakers match arms still refer to "Burning"/"burn" on the Resume screen; update the messages in the NoOne, ContractOwner, and Identity(identifier) arms to say "Resuming"/"resume" (e.g., replace "Burning is not allowed on this token" with "Resuming is not allowed on this token", and "You are not allowed to burn this token" with "You are not allowed to resume this token"), keeping the same logic around identity_token_info and AuthorizedActionTakers checks.src/ui/tokens/transfer_tokens_screen.rs (2)
232-237:⚠️ Potential issue | 🟡 MinorDouble
expect()on data contract lookup can also panic.If the data contract is somehow missing or the DB query fails, both
.expect()calls on Lines 235-236 will panic. This is pre-existing (not changed in this PR), but it's on the same code path that was improved above and was flagged in the audit (finding#3).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/transfer_tokens_screen.rs` around lines 232 - 237, The code currently calls self.app_context.get_unqualified_contract_by_id(&self.identity_token_balance.data_contract_id).expect(...).expect(...), which can panic on both the Result and Option; change this to handle both error cases explicitly: call get_unqualified_contract_by_id(...), match or use ? to propagate the Result error (or convert it into a user-facing error), then handle the Option (None) with a controlled error path (e.g., return Err, show a UI error, or log and early-return) instead of using expect; update the creation of data_contract (Arc::new(...)) to use the safely-unwrapped value after these checks and reference the symbols data_contract, get_unqualified_contract_by_id, and identity_token_balance.data_contract_id to locate the code to modify.
276-294:⚠️ Potential issue | 🟠 Major
refresh()still contains double.unwrap()and can panic if the identity is deleted.Lines 281 and 284 both call
.unwrap()— if the DB load fails or the identity was deleted while this screen is open, the app panics. This was identified in the PR audit (finding#1) and is inconsistent with the graceful handling added to other screens (e.g.,mint_tokens_screen.rsLine 381 usesif let Ok(...)+.find()). Consider the same pattern here:🐛 Proposed fix
fn refresh(&mut self) { - self.identity = self - .app_context - .load_local_qualified_identities() - .unwrap() - .into_iter() - .find(|identity| identity.identity.id() == self.identity.identity.id()) - .unwrap(); + if let Ok(all_identities) = self.app_context.load_local_qualified_identities() + && let Some(updated_identity) = all_identities + .into_iter() + .find(|identity| identity.identity.id() == self.identity.identity.id()) + { + self.identity = updated_identity; + } let token_balances = self .app_context .dbBased on learnings, error handling refactoring is needed across the codebase to avoid panics with
.expect()and instead propagate errors properly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/transfer_tokens_screen.rs` around lines 276 - 294, The refresh() method currently uses two .unwrap() calls that can panic if loading identities fails or the identity was deleted; change it to gracefully handle errors: call self.app_context.load_local_qualified_identities() with if let Ok(identities) and then use .into_iter().find(|i| i.identity.id() == self.identity.identity.id()) to update self.identity only if found (otherwise return early or set an appropriate fallback), and replace the expect on self.app_context.db.get_identity_token_balances(&self.app_context) with if let Ok(token_balances) (or handle Err) and then compute self.max_amount by finding the matching balance and mapping with Amount::from, defaulting to Amount::default() when absent; ensure no unwrap()/expect() remain in refresh().src/backend_task/identity/mod.rs (1)
107-145:⚠️ Potential issue | 🟠 MajorInconsistency in
to_key_storage: ECDSA_HASH160 key data not handled liketo_public_keys_map.
to_key_storage(line 126) always stores keys usingprivate_key.public_key(&secp).to_bytes().into()regardless ofkey_type. However,to_public_keys_map(lines 224-231) correctly usespubkey_hash().to_byte_array()forECDSA_HASH160keys. This causes a data mismatch: if anECDSA_HASH160key is registered viato_public_keys_map, thedatafield stored byto_key_storagewill not match the on-chain key. The same issue exists for the master key (line 88 vs lines 159-166). SinceECDSA_HASH160is the default key type in the application, this should be fixed into_key_storageto match the behavior ofto_public_keys_map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/mod.rs` around lines 107 - 145, The code in to_key_storage constructs IdentityPublicKey::V0 using private_key.public_key(&secp).to_bytes().into() unconditionally, which mismatches to_public_keys_map for KeyType::ECDSA_HASH160; update to_key_storage (and the master key creation path that mirrors it) to check for KeyType::ECDSA_HASH160 and set data = private_key.pubkey_hash().to_byte_array().into() for that case, otherwise continue using private_key.public_key(&secp).to_bytes().into(); ensure you modify the IdentityPublicKey::V0 construction sites (the per-key loop producing qualified_identity_public_key and the master key creation) and use the same pubkey_hash() logic as to_public_keys_map to keep on-chain and stored data consistent.
🧹 Nitpick comments (1)
src/backend_task/identity/mod.rs (1)
210-222: Nit: Considermatches!for readability.♻️ Suggested simplification
- Purpose::AUTHENTICATION => { - if *security_level != SecurityLevel::CRITICAL - && *security_level != SecurityLevel::HIGH - && *security_level != SecurityLevel::MEDIUM - { + Purpose::AUTHENTICATION => { + if !matches!( + security_level, + SecurityLevel::CRITICAL | SecurityLevel::HIGH | SecurityLevel::MEDIUM + ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/mod.rs` around lines 210 - 222, Replace the chained inequality checks in the Purpose::AUTHENTICATION match arm with a matches! expression for readability: in the block handling Purpose::AUTHENTICATION (around the match on purpose/security checks), use matches!(security_level, SecurityLevel::CRITICAL | SecurityLevel::HIGH | SecurityLevel::MEDIUM) and invert the condition to return the same Err message when it does not match; keep the existing Err(...) text and the id and security_level references intact so behavior and error content remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/backend_task/identity/mod.rs`:
- Around line 107-145: The code in to_key_storage constructs
IdentityPublicKey::V0 using private_key.public_key(&secp).to_bytes().into()
unconditionally, which mismatches to_public_keys_map for KeyType::ECDSA_HASH160;
update to_key_storage (and the master key creation path that mirrors it) to
check for KeyType::ECDSA_HASH160 and set data =
private_key.pubkey_hash().to_byte_array().into() for that case, otherwise
continue using private_key.public_key(&secp).to_bytes().into(); ensure you
modify the IdentityPublicKey::V0 construction sites (the per-key loop producing
qualified_identity_public_key and the master key creation) and use the same
pubkey_hash() logic as to_public_keys_map to keep on-chain and stored data
consistent.
In `@src/ui/tokens/pause_tokens_screen.rs`:
- Around line 96-115: Error messages in the pause_tokens_screen logic still
refer to "Burning"/"burn" instead of "Pausing"/"pause"; update the strings in
the match arms for AuthorizedActionTakers::NoOne, ::ContractOwner, and
::Identity(identifier) so they say "Pausing is not allowed on this token", "You
are not allowed to pause this token. Only the contract owner is.", and "You are
not allowed to pause this token" respectively, modifying the error_message
assignments that reference identity_token_info and AuthorizedActionTakers to use
the correct wording and casing.
In `@src/ui/tokens/resume_tokens_screen.rs`:
- Around line 96-115: The error strings in the AuthorizedActionTakers match arms
still refer to "Burning"/"burn" on the Resume screen; update the messages in the
NoOne, ContractOwner, and Identity(identifier) arms to say "Resuming"/"resume"
(e.g., replace "Burning is not allowed on this token" with "Resuming is not
allowed on this token", and "You are not allowed to burn this token" with "You
are not allowed to resume this token"), keeping the same logic around
identity_token_info and AuthorizedActionTakers checks.
In `@src/ui/tokens/transfer_tokens_screen.rs`:
- Around line 232-237: The code currently calls
self.app_context.get_unqualified_contract_by_id(&self.identity_token_balance.data_contract_id).expect(...).expect(...),
which can panic on both the Result and Option; change this to handle both error
cases explicitly: call get_unqualified_contract_by_id(...), match or use ? to
propagate the Result error (or convert it into a user-facing error), then handle
the Option (None) with a controlled error path (e.g., return Err, show a UI
error, or log and early-return) instead of using expect; update the creation of
data_contract (Arc::new(...)) to use the safely-unwrapped value after these
checks and reference the symbols data_contract, get_unqualified_contract_by_id,
and identity_token_balance.data_contract_id to locate the code to modify.
- Around line 276-294: The refresh() method currently uses two .unwrap() calls
that can panic if loading identities fails or the identity was deleted; change
it to gracefully handle errors: call
self.app_context.load_local_qualified_identities() with if let Ok(identities)
and then use .into_iter().find(|i| i.identity.id() ==
self.identity.identity.id()) to update self.identity only if found (otherwise
return early or set an appropriate fallback), and replace the expect on
self.app_context.db.get_identity_token_balances(&self.app_context) with if let
Ok(token_balances) (or handle Err) and then compute self.max_amount by finding
the matching balance and mapping with Amount::from, defaulting to
Amount::default() when absent; ensure no unwrap()/expect() remain in refresh().
---
Nitpick comments:
In `@src/backend_task/identity/mod.rs`:
- Around line 210-222: Replace the chained inequality checks in the
Purpose::AUTHENTICATION match arm with a matches! expression for readability: in
the block handling Purpose::AUTHENTICATION (around the match on purpose/security
checks), use matches!(security_level, SecurityLevel::CRITICAL |
SecurityLevel::HIGH | SecurityLevel::MEDIUM) and invert the condition to return
the same Err message when it does not match; keep the existing Err(...) text and
the id and security_level references intact so behavior and error content remain
unchanged.
Summary
Test plan
Summary by CodeRabbit
Bug Fixes
Improvements