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

refactor: detailed error messages for users #3427

Conversation

therustmonk
Copy link
Contributor

Description

This refactoring lets us improve the user experience if a node failed.

Motivation and Context

Related to #3423. These changes help us to fix the following issues.

Duplications like this:
applications/tari_app_utilities/src/identity_management.rs#L58

error!(
    target: LOG_TARGET,
    "Node identity information not found. {}. You can update the configuration file to point to a \
     valid node identity file, or re-run the node with the --create-id flag to create a new \
     identity.",
    e
);
return Err(ExitCodes::ConfigError(format!(
    "Node identity information not found. {}. You can update the configuration file to point to a \
     valid node identity file, or re-run the node with the --create-id flag to create a new \
     identity.",
    e
)));

Avoid logging in converting methods and let us derive From automatically:
applications/tari_app_utilities/src/utilities.rs#L130

impl From<tari_common::ConfigError> for ExitCodes {
    fn from(err: tari_common::ConfigError) -> Self {
        error!(target: LOG_TARGET, "{}", err); 
        Self::ConfigError(err.to_string())
    }
}

Handle potential panics:
applications/tari_app_utilities/src/identity_management.rs#L201

pub fn save_as_json<P: AsRef<Path>, T: MessageFormat>(path: P, object: &T) -> Result<(), String> {
    let json = object.to_json().unwrap();
    if let Some(p) = path.as_ref().parent() {

Not everything can be converted to JSON, and if we change the format of an object (and will have numbers as keys of maps, etc.), we can have panic there.

Later we can also implement the standard Termination trait (not stabilized yet) for this:
applications/tari_app_utilities/src/utilities.rs#L86

impl ExitCodes {
    pub fn as_i32(&self) -> i32 {
        match self {
            Self::ConfigError(_) => 101,
            Self::UnknownError => 102,
            Self::InterfaceError => 103,

And use ExitCodes as returned Error of the main function.

Improve specific errors checking:
applications/tari_base_node/src/main.rs#L220

// todo: find a better way to do this
if boxed_error.to_string().contains("Invalid force sync peer") {

It's the potential issue if we will change the error's message and forget to sync the text above.

How Has This Been Tested?

Not tested yet 🙃 (to be updated...)

Specific error types let us keep all the details of an error
and give detailed messages to users.
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Looks good!

@sdbondi
Copy link
Member

sdbondi commented Oct 12, 2021

Suggest that LoadFromJsonError and SaveToJsonError can become one error type for the module.

@stringhandler
Copy link
Collaborator

Already implemented in a different way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants