Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix-2512 | rusk-wallet: Remove all panics in the codebase #3005

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

welf
Copy link
Contributor

@welf welf commented Nov 18, 2024

  • Remove all unwraps (some unwraps are left in tests)
  • Properly handle all expects
  • Remove From<f64> for Dusk implementation and replace it with TryFrom<f64> for Dusk implementation
  • Implement From<GraphQLError> for Error and From< InquireError> for Error
  • Remove MAX_PROFILES constant and replace it with get_max_profiles function call to handle possible errors properly

Unrelated to panic handling

  • Replace PasswordDisplayMode::Hidden with PasswordDisplayMode::Masked in password prompts to improve user experience when entering passwords

Future improvements (for a separate PR)

  • Enhance error handling in the inquire menu. For non-critical errors, provide users with options to retry the failed operation or return to the previous menu, rather than always exiting with an error message.

@welf welf requested review from Daksh14 and moCello November 18, 2024 13:17
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

You seem to have some changes to rusk and the stake contract in here which don't belong to the PR.
Apart from that I only some small comments

rusk-wallet/src/bin/io/prompt.rs Outdated Show resolved Hide resolved
rusk-wallet/src/bin/config.rs Outdated Show resolved Hide resolved
@welf welf force-pushed the bugfix-2512-rusk-wallet-remove-all-panics branch 2 times, most recently from b15dff0 to 0511b0d Compare November 18, 2024 14:52
moCello
moCello previously approved these changes Nov 18, 2024
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

LGTM

@welf welf force-pushed the bugfix-2512-rusk-wallet-remove-all-panics branch from c475ee9 to 923789f Compare November 18, 2024 17:03
@welf welf requested a review from moCello November 18, 2024 17:42
@welf welf added fix:bug Something isn't working module:rusk-wallet Issues related to rusk wallet labels Nov 18, 2024
// poison error and return the guard to the caller.
match state {
Ok(guard) => Arc::clone(&guard),
Err(poisoned) => Arc::clone(&poisoned.into_inner()),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a crate fn, if its called more than once we'll just deadlock anyways

The lock is also dropped once the function returns so im not sure if we need the match here. The function is also synchronous it won't be in a multithreaded environment

Copy link
Contributor

@Daksh14 Daksh14 left a comment

Choose a reason for hiding this comment

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

one nit otherwise LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix:bug Something isn't working module:rusk-wallet Issues related to rusk wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants