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

[ABW-3454] iOS - Display proper error messages for Sargon errors #168

Conversation

danvleju-rdx
Copy link
Contributor

@danvleju-rdx danvleju-rdx commented Jun 19, 2024

Jira ticket: ABW-3454

  • Make SargonError conform to LocalizedError to ensure a proper localizedDescription is displayed in the iOS wallet.
  • Added a new UniFFI export function is_safe_to_show_error_message for hosts to determine if the error_message can be displayed to users. For now, the only error we consider "safe" is FailedToDeserializeJSONToValue

@danvleju-rdx danvleju-rdx added the Swift 🍏 Changes in Swift Sargon label Jun 19, 2024
Copy link
Contributor

@matiasbzurovski matiasbzurovski left a comment

Choose a reason for hiding this comment

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

solution makes sense to me but not sure if Matt wants to show that specific error message (Failed to deserialize JSON with #319 bytes of Data) or something more generic for now (e.g. Something went wrong, please contact support with code 10070).

We could also remove the "with code 10070" part, and in iOS/Android side handle Sargon Errors so that we show a Contact Support button that when tapped, the email preloads "Error code 10070" in the body.


extension SargonError: LocalizedError {
public var errorDescription: String? {
"\(errorMessage)\nCode: \(errorCode)"
Copy link
Contributor

Choose a reason for hiding this comment

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

did we conclude that we wanted to show errorMessage, it is very much a "programmer error" written by me... I think Matt would prefer to not show it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, we could use something similar to what Matias suggested. I'll check with Matt.

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.8%. Comparing base (7440f6e) to head (6ad076f).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #168   +/-   ##
=====================================
  Coverage   98.8%   98.8%           
=====================================
  Files        756     756           
  Lines      11347   11357   +10     
  Branches      27      27           
=====================================
+ Hits       11213   11223   +10     
  Misses       132     132           
  Partials       2       2           
Flag Coverage Δ
kotlin 99.7% <ø> (ø)
rust 98.5% <100.0%> (+<0.1%) ⬆️
swift 99.7% <100.0%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ns/Swiftified/Prelude/SargonError+Swiftified.swift 100.0% <100.0%> (ø)
src/core/error/common_error.rs 100.0% <100.0%> (ø)

@GhenadieVP
Copy link
Contributor

solution makes sense to me but not sure if Matt wants to show that specific error message (Failed to deserialize JSON with #319 bytes of Data) or something more generic for now (e.g. Something went wrong, please contact support with code 10070).

We could also remove the "with code 10070" part, and in iOS/Android side handle Sargon Errors so that we show a Contact Support button that when tapped, the email preloads "Error code 10070" in the body.

As you suggest we could extend the Error alert to have a contact support button, which in the email body can preload the full error message with the code along. Just the code is not very helpful.

@matiasbzurovski
Copy link
Contributor

matiasbzurovski commented Jun 19, 2024

Just the code is not very helpful.

We could replace the code at OverlayWindowClient for something like this:

errorQueue.errors().map { error in
	if let sargonError = error as? SargonError {
		return Item.alert(.init(
			title: { TextState(L10n.Common.errorAlertTitle) },
			actions: { .init(action: .contactSupport(sargonError.errorCode), label: { TextState("Contact Support") }) },
			message: { TextState("Oopss something went wrong") }
		))
	} else {
		return Item.alert(.init(
			title: { TextState(L10n.Common.errorAlertTitle) },
			message: { TextState(error.localizedDescription) }
		))
	}
}

and then handle that new AlertAction.contactSupport(Int) to redirect to the email witht such code.


EDIT: I just realise you mean the error code by itself isn't very helpful, not our code 🤦 Anyway, snippet could help. We can include the entire error message as you say, since that would include particular details (when there is a badValue available on the error).

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

LGTM !

Copy link
Contributor

@matiasbzurovski matiasbzurovski left a comment

Choose a reason for hiding this comment

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

One Swift failing test, otherwise looks good to me.


func test_localized_description_for_non_sensitive_error() {
let sut = SUT.FailedToDeserializeJsonToValue(jsonByteCount: 100, typeName: "TypeName")
XCTAssertEqual(sut.localizedDescription, "Error code: 10070\nError message: Failed deserialize JSON with #100 bytes to value of type TypeName")
Copy link
Contributor

Choose a reason for hiding this comment

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

This test will fail since the error message was updated:

"Failed deserialize JSON with #{json_byte_count} bytes to value of type {type_name} with error: \"{serde_message}\""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to update after merge. Thanks

extension SargonError: LocalizedError {
public var errorDescription: String? {
let errorCodeFormatted = "Error code: \(errorCode)"
let errorMessageFormatted: String? = isSafeToShowErrorMessageFromError(error: self) ? "Error message: \(errorMessage)" : nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I would opt to show the full error message in Debug mode.

@danvleju-rdx danvleju-rdx merged commit 1df72e1 into main Jun 20, 2024
10 checks passed
@danvleju-rdx danvleju-rdx deleted the ABW-3454-i-os-display-proper-error-messages-for-sargon-errors branch June 20, 2024 11:25
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Swift 🍏 Changes in Swift Sargon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants