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

Handle PSBTExceptions in TransactionBroadcaster + NBitcoin dependency update. #2204

Merged
merged 7 commits into from Sep 9, 2019
@@ -13,6 +13,7 @@
using System.Threading.Tasks;
using WalletWasabi.Gui.Models;
using WalletWasabi.Gui.ViewModels;
using WalletWasabi.Helpers;
using WalletWasabi.Models;

namespace WalletWasabi.Gui.Controls.WalletExplorer
@@ -165,20 +166,20 @@ private async Task OnDoTransactionBroadcastAsync()
ButtonText = "Broadcasting Transaction...";

SmartTransaction transaction;
try
{
var signedPsbt = PSBT.Parse(TransactionString, Global.Network ?? Network.Main);

if (PSBT.TryParse(TransactionString, Global.Network ?? Network.Main, out var signedPsbt))
This conversation was marked as resolved by nopara73

This comment has been minimized.

Copy link
@lontivero

lontivero Sep 6, 2019

Collaborator

Can really Global.Network be null here? I am pretty sure it cannot.

This comment has been minimized.

Copy link
@jmacato

jmacato Sep 6, 2019

Author Collaborator

It was there before but i do think that's a good failsafe. even if it's redundant.

This comment has been minimized.

Copy link
@nopara73

nopara73 Sep 6, 2019

Collaborator

It cannot be null.

This comment has been minimized.

Copy link
@nopara73

nopara73 Sep 6, 2019

Collaborator

I'm ok with both approach.

{
if (!signedPsbt.IsAllFinalized())
{
signedPsbt.Finalize();
}

transaction = signedPsbt.ExtractSmartTransaction();
}
catch
else
{
transaction = new SmartTransaction(Transaction.Parse(TransactionString, Global.Network ?? Network.Main), WalletWasabi.Models.Height.Unknown);
transaction = new SmartTransaction(Transaction.Parse(TransactionString, Global.Network ?? Network.Main),
WalletWasabi.Models.Height.Unknown);
This conversation was marked as resolved by jmacato

This comment has been minimized.

Copy link
@nopara73

nopara73 Sep 4, 2019

Collaborator

Spacing is strange here.

}

MainWindowViewModel.Instance.StatusBar.TryAddStatus(StatusBarStatus.BroadcastingTransaction);
@@ -187,6 +188,10 @@ private async Task OnDoTransactionBroadcastAsync()
SetSuccessMessage("Transaction is successfully sent!");
TransactionString = "";
}
catch (PSBTException ex)
This conversation was marked as resolved by nopara73

This comment has been minimized.

Copy link
@nopara73

nopara73 Sep 4, 2019

Collaborator

Why this catch? Why not let the exception flow? Similar result. It's an advanced tab, no need to make the message stupid, that was the issue you are fixing in the first place.

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Sep 4, 2019

Collaborator

@nopara73 because if there is too much error, the PSBTException is fucking huge and unreadable.

This comment has been minimized.

Copy link
@jmacato

jmacato Sep 4, 2019

Author Collaborator

@nopara73 if I let the exception fall through, what happens is this:
image

So I had to handle and truncate that PSBTException.

This comment has been minimized.

Copy link
@nopara73

nopara73 Sep 5, 2019

Collaborator

Makes sense.

{
SetWarningMessage($"The PSBT cannot be finalized: {ex.Errors.FirstOrDefault()}");
}
catch (Exception ex)
{
OnException(ex);
@@ -21,7 +21,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="NBitcoin" Version="4.2.4" />
<PackageReference Include="NBitcoin" Version="4.2.6" />
<PackageReference Include="Microsoft.Win32.Registry" Version="4.5.0" />
</ItemGroup>

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.