fix: add cargo lock and pin rust version#191
Conversation
Adding based on this to make building deterministic: https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control
WalkthroughThe repository now tracks Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 20
🔭 Outside diff range comments (1)
src/backend_task/tokens/query_tokens.rs (1)
1-2:⚠️ Potential issueIncomplete file implementation
The file only contains a documentation comment but lacks actual implementation code. This seems incomplete or may be part of a larger change not visible in this review.
Check if the implementation is included in another file or if this is an oversight that needs to be addressed.
🧹 Nitpick comments (79)
src/ui/dpns/dpns_contested_names_screen.rs (1)
860-871: Consistent filtering implementation for owned names.The owned names filtering uses the same UI pattern but has a slightly different implementation that doesn't include the character substitutions. Consider updating this to match the other filters for consistency.
- let filter_lc = self.owned_filter_term.to_lowercase(); - name_infos.retain(|c| c.1.name.to_lowercase().contains(&filter_lc)); + let mut filter_lc = self.owned_filter_term.to_lowercase(); + // Convert o and O to 0 and l to 1 in filter_lc + filter_lc = filter_lc + .chars() + .map(|c| match c { + 'o' | 'O' => '0', + 'l' => '1', + _ => c, + }) + .collect(); + name_infos.retain(|c| c.1.name.to_lowercase().contains(&filter_lc));src/backend_task/identity/withdraw_from_identity.rs (1)
19-22: Ensure proper cleanup of the SDK clone.The code now correctly acquires a read lock for thread-safe access to the SDK, which is a good improvement. However, cloning the entire SDK guard might be resource-intensive depending on the SDK's size. Consider if a reference or a more targeted approach could be used instead of cloning the entire SDK.
src/backend_task/identity/load_identity.rs (1)
250-253: Consider error handling for poisoned lock.While acquiring a read lock for the SDK is good for thread safety, the code uses
unwrap()which will panic if the lock is poisoned. Consider handling this error case more gracefully, similar to how it's done in thetop_up_identity_screen/by_wallet_qr_code.rsfile.- let sdk_guard = { - let guard = self.sdk.read().unwrap(); - guard.clone() - }; + let sdk_guard = { + let guard = self.sdk.read().expect("SDK lock was poisoned"); + guard.clone() + };src/ui/identities/add_new_identity_screen/by_wallet_qr_code.rs (1)
55-58: Consider more graceful error handling for poisoned locksWhile the read lock is correctly applied, using
expect()will cause a panic if the lock is poisoned. In production code, it might be better to handle this error more gracefully.- .read() - .expect("Core client lock was poisoned") + .read() + .map_err(|e| format!("Failed to acquire core client lock: {}", e))?src/backend_task/contract.rs (1)
15-15: Introduction of RegisterDataContract variant.Adding
RegisterDataContract(DataContract, String, QualifiedIdentity, IdentityPublicKey)effectively expands task coverage for contract registrations. Overloading the variant with multiple parameters is acceptable but consider documenting each parameter’s purpose for clarity.src/backend_task/tokens/freeze_tokens.rs (1)
1-48: Freeze tokens functionality.
- Implementation parallels the burn logic, which enhances consistency.
- Error mapping is adequate, but you might improve debuggability by providing more contextual logs.
- A test case verifying the freeze operation’s success and failure paths could be beneficial.
src/backend_task/contested_names/query_ending_times.rs (1)
85-101: Document the removed path_query encoding.The code that was encoding the
path_queryhas been commented out andverification_path_query_bytesis now set to an empty vector. This change seems to be related to SDK changes but could benefit from documentation.Consider adding a comment explaining why this change was made and whether the empty vector is a temporary or permanent solution.
- // // Encode the path_query using bincode - // let verification_path_query_bytes = - // match bincode::encode_to_vec(&path_query, bincode::config::standard()) - // .map_err(|encode_err| { - // tracing::error!("Error encoding path_query: {}", encode_err); - // format!("Error encoding path_query: {}", encode_err) - // }) { - // Ok(encoded_path_query) => encoded_path_query, - // Err(e) => return Err(e), - // }; + // Path query is no longer needed in GroveDBError structure verification_path_query_bytes: vec![],src/ui/identities/identities_screen.rs (1)
123-131: Reordering mechanism is clear but might be costly at scale
The loop that repeatedly callsget_index_ofcould have O(n) lookups inside an O(n) iteration, leading to O(n^2) complexity if the map is large. If performance becomes a concern for significantly larger sets, consider a more direct approach. For typical usage with modest identity counts, this is acceptable.src/backend_task/mod.rs (1)
105-108: Good concurrency practice with read lock; consider handling poisoning gracefully.Using a read lock (
self.sdk.read()) is safer than direct cloning. However,unwrap()will panic if the lock is poisoned. An alternative is to handle the error or recover gracefully to prevent unwarranted application termination.-let guard = self.sdk.read().unwrap(); +let guard = match self.sdk.read() { + Ok(guard) => guard, + Err(poisoned) => { + // Attempt to recover or handle error + return Err("SDK read lock poisoned".to_string()); + } +};src/ui/identities/keys/key_info_screen.rs (1)
550-584: Confirmation dialog for key removal is clear and safe.The new
render_remove_private_key_confirmmethod:
- Prompts the user for confirmation.
- Cleans up the internal state if “Remove” is chosen.
- Persists changes by removing private key references.
Although functional, consider mandating an additional check (e.g., typed confirmation) if the operation is high-risk.
src/backend_task/identity/top_up_identity.rs (2)
64-65: Read lock usage oncore_clientlock.Calls
read().expect(...), which will panic if poisoned. Consider more robust error handling to avoid unexpected crashes.
106-114: Reloading UTXOs under write lock blocks concurrency.Releasing the lock earlier can improve concurrency if the UTXO loading is a time-consuming IO operation. Consider narrowing the lock scope to reduce blocking.
src/backend_task/register_contract.rs (3)
23-32: Ensure user feedback on empty alias.
Whenalias.is_empty()is true, you silently drop the alias by storingNone. Consider providing more explicit user feedback or logs, in case this behavior might confuse end users.
33-43: Validate error confidentiality.
Inserting the contract may fail due to various reasons (e.g., database constraints). Re-check that the error messages (format!("Error inserting contract...")) do not inadvertently expose sensitive details. If needed, provide a sanitized or less verbose error in user-facing logs.
44-50: Enhance success message.
Returning a simple string like"DataContract successfully registered"might be insufficient in certain contexts, especially if more insights (contract ID, alias, etc.) would benefit the user.src/backend_task/tokens/destroy_frozen_funds.rs (2)
25-30: Check correctness oftoken_position.
token_positionis used to identify which token is being destroyed. Consider validating or at least logging this value prior to building the transition, in case the provided position is out of range or not recognized.
32-37: Unused_senderparameter.
The_senderis passed into the function but never used. If it’s intentionally unused, consider removing it or documenting the reason for its presence..env.example (1)
19-27: Avoid committing sensitive credentials or placeholders.
TheseDEVNET_*lines contain default passwords. Ensure real deployments use environment-specific files or secret management to avoid leaking credentials.src/backend_task/tokens/unfreeze_tokens.rs (3)
14-24: Consider removing or utilizing_sender.The
_senderparameter is never used, which might lead to confusion. Either remove it if no longer needed, or handle it if it’s intended for post-processing, logging, or other feedback to the caller.
34-37: Use typed errors for clearer diagnostics.Currently, the error is converted into a string with limited contextual details. Consider returning a custom error type or an enum to provide more structured and actionable data to callers.
39-43: Handle or utilize the proof result.The
_proof_resultis not used after broadcasting. If this proof contains relevant information (such as confirmation of unfreeze completion), consider logging or returning it to confirm the success state.src/ui/components/tokens_subscreen_chooser_panel.rs (2)
7-24: Add failover or user feedback for missing settings.When
app_context.get_settings()is unavailable or invalid, the code uses a fallbackTokensSubscreen::MyTokens. Provide user feedback (e.g., a label or message) if settings are missing, so users know they’ve fallen back to the default screen.
39-68: Promote extensibility with a dynamic subscreen list.The code manually enumerates subscreens in a vector and iterates over them in the same scope. If more subscreens are added, consider a self-contained data structure or enum-to-string mapping that can dynamically generate the labels and actions, improving scalability.
src/context_provider.rs (1)
82-83: Avoid returning multiple known contracts in the future.Currently, if
data_contract_idmatches the DPNS or token history contract, the method returns that contract. If additional contract checks are added later, it could get unwieldy. Consider a more flexible approach, such as storing and indexing known contracts in a map by ID, to avoid repetitiveifchecks.src/backend_task/tokens/pause_tokens.rs (2)
14-23: Consider removing unused_senderparameter.The parameter
_senderis never used in this method. Removing inactive parameters can help reduce confusion and improve clarity.- _sender: mpsc::Sender<TaskResult>,
24-48: Abstract repeated logic for building, signing, and broadcasting.This pattern of creating the transition builder, signing, and broadcasting is duplicated across various token methods (e.g., pause, resume, transfer). Consider centralizing these steps into a shared helper to reduce code duplication and simplify future maintenance.
+// Example of a shared utility function to handle signing and broadcasting: +pub async fn handle_emergency_transition( + sdk: &Sdk, + signing_key: &IdentityPublicKey, + identity: &QualifiedIdentity, + builder: TokenEmergencyActionTransitionBuilder, + platform_version: u32, +) -> Result<(), String> { + let state_transition = builder + .sign(sdk, signing_key, identity, platform_version) + .await + .map_err(|e| format!("Error signing transition: {:?}", e))?; + + state_transition + .broadcast_and_wait::<StateTransitionProofResult>(sdk, None) + .await + .map_err(|e| format!("Error broadcasting transition: {}", e))?; + Ok(()) +}src/backend_task/tokens/transfer_tokens.rs (2)
15-26: Parametersenderis not utilized.The
senderparameter does not appear to be used. Consider removing it to reduce clutter, or add logic if it will eventually send transfer progress/results.- sender: mpsc::Sender<TaskResult>,
27-53: Consider consolidating repetitive transition workflow and adding test coverage.The steps for building, signing, and broadcasting the token transfer transition largely mirror those in other token operations. You might benefit from extracting this workflow into a shared method or trait for DRY code. Also, adding targeted tests will help validate both success and error paths.
+// Example: central utility function for transitions: +pub async fn handle_transfer_transition( + sdk: &Sdk, + builder: TokenTransferTransitionBuilder, + ... +) -> Result<(), String> { + let state_transition = builder + .sign(/* ... */) + .await + .map_err(|e| format!("Error signing: {:?}", e))?; + + state_transition.broadcast_and_wait::<StateTransitionProofResult>(sdk, None) + .await + .map_err(|e| format!("Error broadcasting: {}", e))?; + Ok(()) +}src/backend_task/tokens/resume_tokens.rs (2)
14-23: Revisit the_senderparameter usage.Here as well,
_senderis not actively used. Consider removing it or implementing status/result notifications if it’s intended for that purpose.- _sender: mpsc::Sender<TaskResult>,
24-49: Potential for shared emergency action handling logic.The structure of building, signing, and broadcasting the token resume transition is repeated in other files (pause, freeze, etc.). Centralize these steps in a helper function to minimize duplication and boost maintainability.
+// Potential aggregator for emergency-actions: +pub async fn handle_emergency_action( + sdk: &Sdk, + builder: TokenEmergencyActionTransitionBuilder, + identity: &QualifiedIdentity, + signing_key: &IdentityPublicKey, + platform_version: u32, +) -> Result<(), String> { + let st = builder + .sign(sdk, signing_key, identity, platform_version) + .await + .map_err(|e| format!("Error signing: {:?}", e))?; + + st.broadcast_and_wait::<StateTransitionProofResult>(sdk, None) + .await + .map_err(|e| format!("Error broadcasting: {}", e))?; + Ok(()) +}src/backend_task/identity/transfer.rs (1)
19-22: Consider robust error handling forunwrap()usage when acquiring the SDK read lockUsing
unwrap()may cause the program to panic if the lock is poisoned. If the data is critical, it might be better to handle errors gracefully to improve reliability.src/model/qualified_identity/qualified_identity_public_key.rs (1)
44-47: Handle invalid public key slices without panicking
expect("Expected valid public key")will panic on malformed data. Consider returning an error or using a graceful fallback to avoid crashing at runtime.src/backend_task/identity/register_identity.rs (3)
121-124: Usingunwrap()on the SDK read lock may propagate panicsIf lock poisoning occurs, the code will panic. Consider using a custom error or handling strategy to improve robustness.
187-194: Reloading UTXOs on failureWhile reloading UTXOs if initial transaction creation fails is practical, it might be cleaner to encapsulate retry logic in a helper function or method to reduce repetition.
287-289: Potential long-running operations within a read lockExecuting network or disk I/O while holding a read lock can block other readers unnecessarily. Consider copying necessary data first, then releasing the lock before performing heavy I/O.
src/backend_task/tokens/query_my_token_balances.rs (1)
15-95: Expand error handling inquery_my_token_balances
- A single failure in token balance fetching aborts processing for all identities. Consider returning partial results or continuing.
- Ensure no data races occur if other tasks update token balances concurrently.
- Provide more granular logs for troubleshooting in multi-token scenarios.
src/app_dir.rs (4)
10-10: Use a more descriptive constant name.
CORE_APPLICATIONis clear, but consider making it even more explicit (e.g.,DASH_CORE_APPLICATION) if you anticipate other Core-like applications in the future.
12-20: Validate the returned paths before using them.The
user_data_dir_pathfunction relies onProjectDirs::from. If this function returnsNone, you handle it with an error. However, validating the resulting path (e.g., checking if it's writable) might be beneficial.fn user_data_dir_path(app: &str) -> Result<PathBuf, std::io::Error> { let proj_dirs = ProjectDirs::from(QUALIFIER, ORGANIZATION, app).ok_or_else(|| { std::io::Error::new( std::io::ErrorKind::NotFound, "Failed to determine project directories", ) })?; - Ok(proj_dirs.config_dir().to_path_buf()) + let path = proj_dirs.config_dir().to_path_buf(); + // Potential extended validation snippet: + // if !path.exists() || !path.is_dir() { ... } + Ok(path) }
22-24: Keep naming consistent for clarity.
app_user_data_dir_path()callsuser_data_dir_path(APPLICATION). Consider an inline function or a constant forAPPLICATIONusage instead, ensuring consistent naming and limiting extra indirection.
26-44: Adopt a cross-platform strategy or environment variable for the DashCore directory.The approach of returning
~/.dashcoreon Linux andProjectDirson other platforms is reasonable. However, some advanced or containerized setups might want configuration overrides (e.g., an environment variable likeDASHCORE_PATH). Supporting that could improve flexibility.Do you want me to propose an alternative environment-variable-based approach for cross-platform compatibility?
src/ui/components/contract_chooser_panel.rs (4)
33-37: Consider capturing error details in logs beyond just printing.When
get_contracts(None, None)fails, you only print an error. Logging approaches (e.g., usingtracing::error!) might offer deeper insights or metrics for debugging.
39-51: Filtering logic is clear but consider fuzzy matching or advanced filtering.Your filtering only does a simple
containscheck. Depending on scale, consider more robust search or fuzzy matching (e.g.,regexor library-based).
64-72: Useful inlined search box.Your horizontal layout for the filter input is straightforward. Consider highlighting the search box or adding placeholders to help guide user input.
307-320: “Remove” button logic is straightforward but consider a confirmation step.One accidental click could remove the contract. A confirmation dialog or an “Undo” approach can help guard against user mistakes.
src/backend_task/core/mod.rs (3)
6-6: NetworkConfig usage is now more explicit.See if you need to handle missing credentials or partial configs. The existing approach of returning an error on missing config is clear but might benefit from more detailed logs.
50-51: Check for concurrency-related issues.You are reading
.expect("Core client lock was poisoned")on thecore_clientlock. If multiple tasks run in parallel and panic, consider a more graceful fallback than panicking.
111-153: Graceful fallback from cookie to user/password is a strong design choice.This approach ensures a robust connection flow. A few suggestions:
- Log the exact cause of the cookie auth failure for debugging.
- Possibly handle proxy environments or different auth flows if user/pass also fails.
match Client::new(&addr, Auth::CookieFile(cookie_path.clone())) { Ok(client) => Ok(client), Err(cookie_err) => { + tracing::debug!("Cookie authentication failed: {}", cookie_err); tracing::info!( "Failed to authenticate using .cookie file at {:?}, falling back to user/pass", cookie_path ); Client::new( &addr, Auth::UserPass(...), ) } }src/ui/tokens/burn_tokens_screen.rs (3)
47-49: Consider clarifying or removingerror_messageif always shadowed bystatus.
Currently, bothstatus(when set toErrorMessage(...)) anderror_messagestore error details. This duality can make error handling confusing. Consider consolidating these fields or always deriving one from the other for clarity.
155-161: Validate non-zero burn amounts.
Parsing foru64ensures the user cannot enter negative numbers, but it does not guard against a zero burn amount. If the logic requires a strictly positive number, add a check to improve the user feedback.
395-408: Consider more granular error messages.
When inBurnTokensStatus::ErrorMessage(msg), the UI just displays"Error: {}". For debugging or user clarity, you might highlight whether it's a backend or parsing error, or provide recommended next steps for the user.src/backend_task/tokens/mod.rs (4)
41-130: Consider splitting large enum variants for token tasks.
TheTokenTaskenum holds many distinct operations with numerous fields (e.g.,RegisterTokenContract). This can grow unwieldy. Consider modularizing sub-enums or using different data structures for clarity and maintainability.
132-203: Add logging or contextual metadata for contract registration.
When building and registering a data contract, adding logs or contextual info (e.g.,token_name,owner_id) can help trace issues if the operation fails. This improves observability and debug-friendliness..map_err(|e| format!("Error building contract V1: {e}"))?; + tracing::info!("Attempting to register token contract '{}', owner = {}", token_name, identity.identity.id()); // 2) Call your existing function...
204-368: Continue implementing placeholder queries or remove them.
Several query tasks (QueryTokensByKeyword,QueryTokensByKeywordPage) remain placeholders. If they are not immediately needed, consider removing them to maintain code clarity, or fully implement them to avoid dead code.
371-453: Refactor thebuild_data_contract_v1_with_one_tokenfunction for clarity.
This single function configures a large number of fields ontoken_config_v0. You may benefit from extracting logic into helper methods (e.g., a separate function for localizing the token name, additional checks formax_supply, etc.) to keep each part more maintainable.src/config.rs (5)
50-58: Return a specialized error for unknown networks.
When matching onNetworkinconfig_for_network,_ => &Nonesilently discards unknown networks. If new networks appear in the future, consider returning an error or logging a warning to avoid confusion.
60-65: Store confidential credentials securely.
The.envfile is stored in plain text. Consider guiding users about the sensitivity ofcore_rpc_password. Encourage secure secret management instead of checking.envinto version control (if that is a risk).
154-216: Revisit mandatory networks.
If your application genuinely supports only Mainnet and Testnet, you might remove the hard check that blocks loading when Devnet or Local are missing. If you genuinely require Devnet/Local for testing, consider explicitly failing early instead of merely warning.
244-259: Handle unexpected “custom” networks.
Inupdate_config_for_network, the_ => { ... }block only logs a warning. Decide whether to fail outright or store partial config. This can help avoid silent misconfiguration.
282-287: Document the builder-like approach.
Theupdate_core_rpc_passwordmethod returns a newNetworkConfig. Consider adding short doc comments clarifying that it clones or updates the struct for a functional style approach. This helps others use it more effectively.src/app.rs (2)
240-262: Devnet tokens setup
Handles creation of tokens screens for Devnet. Note that the repeated pattern for creating each set of screens across networks might benefit from shared helper methods to reduce duplication.
263-284: Regtest (Local) tokens setup
Similar repeated logic for local/regtest environment. A common function for building these screens by passing in the relevant network context would aid maintainability.src/ui/tokens/resume_tokens_screen.rs (2)
146-146: Consider usingInstantfor elapsed timing.When tracking how long a process has been waiting, using
std::time::Instantis often more precise and appropriate than storing a rawu64fromSystemTime::now().as_secs().- WaitingForResult(u64), + WaitingForResult(std::time::Instant),
12-14: Add more test coverage for the resume flow.The file primarily focuses on UI logic, leaving the backend task driving the core functionality. Consider adding one or more tests covering different statuses (e.g.,
NotStarted,WaitingForResult,Complete) to ensure the UI flow remains stable.src/context.rs (1)
153-195: Ensure safe re-initialization under concurrent usage.The
reinit_core_client_and_sdkmethod rebuilds both the RPC client and the SDK by acquiring multiple locks sequentially. If other threads are reading or writing these locks, partial updates could lead to inconsistencies. Consider whether additional synchronization or an atomic swap is needed to ensure the system remains stable.src/ui/tokens/mint_tokens_screen.rs (2)
172-177: Improve user feedback on invalid amount.When parsing an invalid amount, you immediately set an error message and close the confirmation popup, which might be slightly abrupt. Consider letting the user correct the value in place or highlight the field to enhance the user experience.
326-326: Consider validating zero or negative mint amounts.Currently, the code checks for a valid
u64but does not enforce a non-zero constraint. Decide whether zero or extremely large amounts should be supported, and if not, provide appropriate checks and messages.src/database/tokens.rs (3)
19-33: Consider adding an index for frequently queried columns.Currently, the table creation script does not define any secondary indexes. If queries frequently filter by
identity_idornetwork, adding appropriate indexes can significantly improve performance for large datasets.CREATE TABLE IF NOT EXISTS identity_token_balances ( token_id BLOB NOT NULL, identity_id BLOB NOT NULL, balance INTEGER NOT NULL, data_contract_id BLOB NOT NULL, token_position INTEGER NOT NULL, network TEXT NOT NULL, PRIMARY KEY(token_id, identity_id, network) ) +CREATE INDEX IF NOT EXISTS idx_identity_id_network + ON identity_token_balances (identity_id, network);
154-171: Consider a trace or warning when removing zero or non-existent balances.If the intended token balance doesn’t exist, this deletion call will succeed without actually removing any records. Depending on your domain logic, adding a debug log or check to confirm removal was performed might be useful for diagnosing issues.
216-249: Log or surface row parsing errors inload_token_order.Currently, invalid rows are simply skipped. Adding a small debug log for each parse failure can help identify data corruption or invalid row entries in the future.
src/ui/network_chooser_screen.rs (4)
42-50: Sanitize or securely store Dashmate passwords.Storing the dashmate password in a string variable and the config file might raise security considerations. Ensure that password logs or memory snapshots are protected if this password is sensitive.
254-265: Add an option to stop or restart Local network.This code displays a “Start” button for all networks except Regtest (Local environment). However, for local development, it might be useful to start or restart the local node. Consider adding a corresponding action for consistency and convenience.
268-314: Prompt user before overwriting local configuration.Saving the dashmate password overwrites the local config. Consider validating or asking for confirmation before overwriting to avoid accidental data loss if the user has a custom config.
334-340: Investigate synchronous design for chain lock checks.Chain lock status is polled at intervals, marked by booleans for each network. If these checks can block or cause UI slowdowns, investigating async solutions could improve responsiveness—especially if queries to devnet/local are slow or indefinite.
Also applies to: 343-351, 354-362
src/ui/tokens/transfer_tokens_screen.rs (2)
106-138: Block selection of inappropriate key types.You are allowing selection of any authentication keys that match certain
PurposeandSecurityLevel. Ensure that if multiple key types span these criteria (e.g., BLS, ECDSA), the user is choosing the correct one for token transfers. Clarifying these constraints in the UI or restricting the set further might be beneficial.
334-340: Improve user feedback while waiting.The code calculates elapsed time for a transfer in seconds, but if the transaction times out or fails silently, users might be stuck. Consider adding more feedback (logs, UI updates, or a progress spinner) and a timeout or retry method.
Also applies to: 343-383
src/ui/tokens/destroy_frozen_funds_screen.rs (4)
31-37: Consider adding more descriptive enum variant documentation.
While the purpose of each variant inDestroyFrozenFundsStatusis somewhat implied, providing short doc comments for each variant (e.g.,/// NotStarted – indicates ...,/// WaitingForResult(u64) – used when ..., etc.) can improve maintainability and clarity for future contributors.
39-67: Encapsulate internal fields that aren't exposed externally.
Consider making some of these struct fields private if they are not intended to be accessed directly outside the screen logic. This approach preserves encapsulation and helps avoid unintended modifications from external code.
153-232: Reinforce user awareness of destructive actions.
Destroying frozen funds is a critical operation. You might consider emphasizing this with an additional warning or requiring a second confirmation (e.g., typing the identity ID to confirm).
303-433: Enhance test coverage for UI state transitions and callbacks.
Though primarily a UI screen, adding integration tests or UI-driven tests would help ensure that state transitions (e.g., fromNotStartedtoWaitingForResulttoComplete) and the confirmation popup flow function correctly under various scenarios.Would you like help creating a test harness for verifying these UI interactions?
🛑 Comments failed to post (20)
.github/workflows/release.yml (1)
33-33: 💡 Verification agent
🧩 Analysis chain
Warning: GitHub Actions runner label might be invalid.
The label "ubuntu-22.04-arm" doesn't appear to be a standard GitHub-hosted runner label. The static analysis indicates this could cause workflow failures.
🏁 Script executed:
#!/bin/bash # Check if this might be a custom self-hosted runner by looking for runner setup in the repo echo "Checking for custom runners configuration..." grep -r "self-hosted" --include="*.yml" .github/ echo "Looking for ubuntu-22.04-arm label usage in other workflows..." grep -r "ubuntu-22.04-arm" --include="*.yml" .github/Length of output: 406
Action Required: Update Runner Label in Workflow
The runner label
"ubuntu-22.04-arm"used in.github/workflows/release.ymlat line 33 does not appear to be a standard GitHub-hosted runner label—and no custom self-hosted runner configuration was found in the repository. This mismatch may cause workflow failures. Please verify whether the intention is to use a custom self-hosted runner (in which case additional configuration is needed) or if you meant to use a standard label (such as"ubuntu-22.04").
- Location:
.github/workflows/release.yml(line 33)- Suggestion: Update the runner label to a valid GitHub-hosted runner label or ensure that proper self-hosted runner configurations are in place.
🧰 Tools
🪛 actionlint (1.7.4)
33-33: label "ubuntu-22.04-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
src/backend_task/contested_names/query_dpns_contested_resources.rs (1)
71-83: 🛠️ Refactor suggestion
Commented-out path_query code.
By commenting out the encoding and storage of
path_query, you lose potentially valuable data for debugging. If that detail is still needed, consider reintroducing it or retaining it behind a feature flag.src/database/contracts.rs (1)
35-67:
⚠️ Potential issueInserting token balances without robust error handling.
You’re calling
.map_err(...)but discarding the result (let _ = ...). Critical DB insertion failures could be silently ignored.-let _ = self - .insert_identity_token_balance(...) - .map_err(|e| { format!("Failed to insert token balance: {:?}", e) }); +if let Err(e) = self.insert_identity_token_balance(...) { + return Err(rusqlite::Error::UserFunctionError( + Box::new(e.into()) + )); +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Next, if the contract has tokens, add the tokens to identity_token_balances if !data_contract.tokens().is_empty() { for token in data_contract.tokens().iter() { let wallets = app_context.wallets.read().unwrap(); let identities = self.get_local_qualified_identities(app_context, &wallets)?; if let Some(token_id) = data_contract.token_id(*token.0) { for identity in identities { let balance = if data_contract.owner_id() == identity.identity.id() { token.1.base_supply() } else { 0 }; if let Err(e) = self.insert_identity_token_balance( &token_id, &identity.identity.id(), balance, &data_contract.id(), *token.0, app_context, ) { return Err(rusqlite::Error::UserFunctionError( Box::new(e.into()) )); } } } } }src/backend_task/tokens/destroy_frozen_funds.rs (1)
39-43: 🛠️ Refactor suggestion
Evaluate result-handling strategy.
While you broadcast and wait for a proof result, it might be beneficial to log transition details or persist them for auditing. Confirm whether any rollback or state-tracking logic is needed in case subsequent operations fail.src/context_provider.rs (1)
24-46:
⚠️ Potential issueValidate cookie format before splitting.
When reading the cookie file, the code automatically splits on “:”. If the file is malformed or empty,
cookie_parts[1]may be out-of-bounds. Adding a validation check before indexing ensures safer error handling.let cookie_parts: Vec<&str> = cookie.split(':').collect(); -if cookie_parts.len() < 2 { - // handle malformed cookie error -}Committable suggestion skipped: line range outside the PR's diff.
src/app_dir.rs (2)
1-4: 💡 Verification agent
🧩 Analysis chain
Consider adding fallback behavior for situations where user directories are unavailable.
While these imports and the
Networkusage look fine, you may want to handle cases whereProjectDirs::from(...)orUserDirs::new()cannot resolve home directories on non-standard OS setups or restricted environments. A fallback strategy (like defaulting to a temporary directory) could improve robustness.To confirm whether edge cases might break directory resolution, you could scan for all references to these path-handling functions and check if there's any existing fallback in place:
Please review the results to ensure no code path lacks a fallback.
🏁 Script executed:
#!/bin/bash fd --type f --exec rg -A 6 'user_data_dir_path|core_user_data_dir_path|core_cookie_path|ProjectDirs|UserDirs' {}Length of output: 5169
Action Required: Implement Fallback for Directory Resolution Failures
The current implementation in
src/app_dir.rs(e.g., in the functionsuser_data_dir_pathandcore_user_data_dir_path) simply returns an error when directory resolution fails. This behavior leaves the code without a recovery path in non-standard or restricted environments. It is recommended to add fallback behavior (such as defaulting to a temporary directory usingstd::env::temp_dir()) to improve robustness.
- Function
user_data_dir_path: Currently returns an error viaok_or_elseifProjectDirs::from(...)fails.- Function
core_user_data_dir_path: Similarly, returns an error whenUserDirs::new()does not yield a valid home directory.- Upstream implications: The absence of fallback logic in these functions means that subsequent operations (e.g., in
core_cookie_pathand elsewhere) might fail unexpectedly.Please consider introducing a fallback strategy that ensures a valid directory path is provided (or at least a safe default) even in edge cases where canonical user directories are unavailable.
46-57: 💡 Verification agent
🧩 Analysis chain
Consider verifying the
devnet_nameargument and network subdirectory.When constructing the
.cookiepath, ifdevnet_nameis empty or contains invalid characters, an unexpected path might be generated. Validating or sanitizingdevnet_namebefore joining could prevent path resolution issues or injection vulnerabilities.Here's a script to check if there's any path handling or input validation for
devnet_namein the codebase:Please confirm that no additional sanitization is needed.
🏁 Script executed:
#!/bin/bash fd --type f --exec rg -A 3 'devnet_name' {}Length of output: 2059
Sanitize and validate the
devnet_nameinputIt appears that in
core_cookie_path(src/app_dir.rs, lines 46–57) theNetwork::Devnetcase usesdevnet_name.as_deref().unwrap_or("")directly without any sanitization. A quick search for additional validation in the codebase (via the provided script) did not reveal any checks or cleansing routines for this parameter. Without explicit sanitization, there is a risk that an empty string or one containing directory separators or other invalid characters might lead to unintended path constructions or injection vulnerabilities.
- Action Needed: Introduce validation or sanitization for
devnet_name(for example, enforcing an allowed character whitelist or rejecting path traversal sequences) before it’s used to build file paths.- Double-check: Ensure similar validation is applied wherever
devnet_nameis processed (e.g., when writing to environment files).Please update the code accordingly.
src/ui/components/contract_chooser_panel.rs (1)
89-305: 🛠️ Refactor suggestion
Large nested collapsible UI block may benefit from modularization.
Nested collapsing blocks for contract info, document types, indexes, tokens, and JSON can become unwieldy. Extract sub-panels or sub-functions for each area to simplify maintenance and reusability.
fn add_contract_chooser_panel(...) -> AppAction { ... - ui.collapsing(contract_header_text, |ui| { - // document types - ... - // tokens - ... - // contract JSON - ... - }); + ui.collapsing(contract_header_text, |ui| { + render_document_types(ui, contract, ...); + render_tokens(ui, contract, ...); + render_contract_json(ui, contract, ...); + }); ... }Committable suggestion skipped: line range outside the PR's diff.
src/ui/tokens/burn_tokens_screen.rs (1)
110-135: 🛠️ Refactor suggestion
Gracefully handle missing key selections.
Ifself.selected_keyisNoneby the time you construct the burn task, the code calls.expect("Expected a key")(line 197), causing a panic. Consider handling or reporting this situation gracefully, especially in production contexts where a crash might be undesirable.- signing_key: self.selected_key.clone().expect("Expected a key"), + signing_key: match &self.selected_key { + Some(key) => key.clone(), + None => { + self.status = BurnTokensStatus::ErrorMessage("No selected key to sign burn".into()); + return AppAction::None; + } + },src/ui/tokens/resume_tokens_screen.rs (2)
154-154: 🛠️ Refactor suggestion
Avoid hard crash when data contract is not found.
Relying on
.expect("Data contract not found")can abruptly terminate the application if no matching contract is present. Instead, handle this case gracefully or display an error message to the user.
68-68:
⚠️ Potential issueGracefully handle missing identity instead of panicking.
Using
.expect("No local qualified identity found for this token’s identity.")here will cause a hard crash if no local identity is found. Consider providing a user-facing error message or an alternative fallback path to prevent panics.- .expect("No local qualified identity found for this token’s identity."); + .unwrap_or_else(|| { + // Show error message and return early or handle gracefully + panic!("No local qualified identity found for this token’s identity."); + })Committable suggestion skipped: line range outside the PR's diff.
src/context.rs (1)
537-543: 🛠️ Refactor suggestion
Validate removal of token balances for concurrency or partial data states.
While
remove_token_balancecan physically remove data, be mindful of potential race conditions if another thread is simultaneously reading or updating the same records. Ensure the calling code properly synchronizes around token operations.src/ui/tokens/mint_tokens_screen.rs (1)
75-75:
⚠️ Potential issueAvoid panicking on missing local identity.
Similar to other screens,
.expect("No local qualified identity found...")can abruptly terminate the application. Use graceful error handling to avoid unexpected crashes.src/database/tokens.rs (1)
189-214:
⚠️ Potential issueWrap or handle potential rollback gracefully.
While the transaction is explicit, ensure errors from
tx.commit()or insertion are surfaced to the caller. If insertion fails mid-loop, some partial inserts might occur before rollback. Confirm that partial states are acceptable or that you log partial failures for debugging.src/ui/tokens/transfer_tokens_screen.rs (2)
270-285: 🛠️ Refactor suggestion
Avoid overshadowing extraneous errors.
In
display_message, if an error occurs outside of the direct token transfer, it setstransfer_tokens_statustoErrorMessage(message.to_string()). This can overshadow other errors. A more fine-grained approach to error handling can help differentiate these states for better debugging.
59-75:
⚠️ Potential issueEnforce business rules for negative or zero transfer amounts.
While you determine
max_amountfrom thetoken_balances, consider validating thatamountis strictly positive and does not exceedmax_amount. Ifamountis zero or invalid, the user might unknowingly trigger an error.if self.amount.is_empty() || self.amount.parse::<u64>().unwrap_or(0) == 0 { - // current code might default to an error later + return Err("Transfer amount must be greater than zero".to_string()); }Committable suggestion skipped: line range outside the PR's diff.
src/ui/mod.rs (1)
294-316:
⚠️ Potential issueCritical screen mismatch when creating freeze/unfreeze/pause/resume screens.
All of these variants (FreezeTokensScreen,UnfreezeTokensScreen,PauseTokensScreen,ResumeTokensScreen) currently instantiateDestroyFrozenFundsScreen, which is likely unintended and prevents the correct UI from loading.Below is a suggested fix that replaces
DestroyFrozenFundsScreenwith the matching screen constructor for each variant:- ScreenType::FreezeTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + ScreenType::FreezeTokensScreen(identity_token_balance) => { + Screen::FreezeTokensScreen(FreezeTokensScreen::new( identity_token_balance.clone(), app_context, )) } - ScreenType::UnfreezeTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + ScreenType::UnfreezeTokensScreen(identity_token_balance) => { + Screen::UnfreezeTokensScreen(UnfreezeTokensScreen::new( identity_token_balance.clone(), app_context, )) } - ScreenType::PauseTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + ScreenType::PauseTokensScreen(identity_token_balance) => { + Screen::PauseTokensScreen(PauseTokensScreen::new( identity_token_balance.clone(), app_context, )) } - ScreenType::ResumeTokensScreen(identity_token_balance) => { - Screen::DestroyFrozenFundsScreen(DestroyFrozenFundsScreen::new( + ScreenType::ResumeTokensScreen(identity_token_balance) => { + Screen::ResumeTokensScreen(ResumeTokensScreen::new( identity_token_balance.clone(), app_context, )) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.ScreenType::FreezeTokensScreen(identity_token_balance) => { Screen::FreezeTokensScreen(FreezeTokensScreen::new( identity_token_balance.clone(), app_context, )) } ScreenType::UnfreezeTokensScreen(identity_token_balance) => { Screen::UnfreezeTokensScreen(UnfreezeTokensScreen::new( identity_token_balance.clone(), app_context, )) } ScreenType::PauseTokensScreen(identity_token_balance) => { Screen::PauseTokensScreen(PauseTokensScreen::new( identity_token_balance.clone(), app_context, )) } ScreenType::ResumeTokensScreen(identity_token_balance) => { Screen::ResumeTokensScreen(ResumeTokensScreen::new( identity_token_balance.clone(), app_context, ))src/ui/tokens/freeze_tokens_screen.rs (1)
64-104: 🛠️ Refactor suggestion
Avoid panicking on missing identity lookups.
The constructor uses several.expect(...)calls (e.g., “No local qualified identity found…”) that will panic if the identity cannot be fetched. This may degrade user experience. Consider returning an error and displaying it in the UI instead of panicking.- .expect("No local qualified identity found for this token"); + .unwrap_or_else(|| { + // Present a user-friendly error or fallback + // e.g., set self.status to FreezeTokensStatus::ErrorMessage(...) + panic!("No local qualified identity found for token"); + })Committable suggestion skipped: line range outside the PR's diff.
src/ui/tokens/unfreeze_tokens_screen.rs (1)
64-104: 🛠️ Refactor suggestion
Use graceful error handling rather than
expect(...).
Similar to the freeze screen, unfreezing heavily relies on.expect(...)calls that could lead to panics in legitimate edge cases (e.g., identity is missing). Prefer gracefully returning errors to the UI.src/ui/tokens/destroy_frozen_funds_screen.rs (1)
69-113: 🛠️ Refactor suggestion
Handle potential file I/O errors more gracefully.
In the constructornew,load_local_qualified_identities()could theoretically fail due to I/O or parsing errors. Currently, youexpectthe identity withexpect("No local qualified identity found ..."). Consider logging or displaying a contextual error message instead, improving user feedback and avoiding abrupt panics in production builds.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
2 participants
Adding based on this to make building deterministic: https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control. Current v0.9-dev (8d050b2) builds for me successfully with this.
Summary by CodeRabbit