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

Fix Wasabi crash if exception happens after PreviewTx screen #11338

Merged
merged 2 commits into from
Aug 24, 2023

Conversation

Szpoti
Copy link
Collaborator

@Szpoti Szpoti commented Aug 21, 2023

Addresses #11191
Addresses #10171
and probably more in the future.

Already suggested this fix in the past for different problems, not sure why it was not accepted.

At this point, if an exception does occur somehow even before trying to finalize and send, just when we build the tx, we should raise the exception and throw out a message because something is deffinetly not ok, and we can prevent the wallet crash as well.

This PR just widens the try/catch.

@adamPetho
Copy link
Collaborator

Already suggested this fix in the past for different problems, not sure why it was not accepted.

I think it was not accepted before because this approach is just hides the problem and wraps it inside a try-catch block.
Instead of fixing the real issues that can come up in this part of send workflow.

I think showing an error dialog is a tiny bit better than crashing the software, but still the issue is there just as before, and we don't present a solution to the user how to overcome the issue.

@Szpoti
Copy link
Collaborator Author

Szpoti commented Aug 21, 2023

but still the issue is there just as before, and we don't present a solution to the user how to overcome the issue.

I absolutely agree with this, the main problem should be found & fixed at the deeper level.
But I still think this way is better as we deflect any crash that can happen. What we can also do to improve, is to create a more detailed try/catch block here, and catch and show various exceptions differently.

@Szpoti
Copy link
Collaborator Author

Szpoti commented Aug 23, 2023

To note

This PR doesn't fix the main, deep problem of #11191 and #10171, this just fixes the wallet crashing because of these kind of errors.

var transaction = await Task.Run(() => TransactionHelpers.BuildTransaction(_wallet, _info));

TransactionHelpers.BuildTransaction() doesnt have a try/catch inside, because every other call of this function is wraped in a try/catch outside. Everywhere, but here, causing the crash.

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

tACK

@soosr soosr merged commit 82fa2fb into WalletWasabi:master Aug 24, 2023
6 of 7 checks passed
@molnard molnard deleted the fixCrashAfterPreview branch August 29, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants