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

Conversation

@jmacato
Copy link
Collaborator

commented Sep 2, 2019

Fixes #2203

cc: @NicolasDorier

Related: #2193

@jmacato jmacato requested a review from nopara73 Sep 2, 2019

@nopara73
Copy link
Collaborator

left a comment

Can you create a TryParse pattern to the PSBT parsing? (Helpers/NBitcoinHelpers) If false, then try the hex, if true, then try the finalization, etc, and let them throw their own exceptions.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2019

This code seems fine, but as @nopara73 said, even better is to use the TryParse.

@jmacato

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

@nopara73 I'm not sure if I got the logic right but i added the TryParse methods in NBitcoinHelpers.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2019

@jmacato NBitcoin already has a PSBT.TryParse method normally...

@jmacato

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 3, 2019

@NicolasDorier it didnt seem to show up on intellisense.. will double check

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2019

@jmacato you are right one sec will make new version fixing this.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2019

You have PSBT.TryFinalize as well, making a new NBitcoin release in 15min, I run the tests and publish.

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2019

@jmacato I pushed version 4.2.6 on nuget

@molnard
Copy link
Collaborator

left a comment

LGTM

jmacato added 2 commits Sep 3, 2019

@jmacato jmacato changed the title Handle PSBTExceptions in TransactionBroadcaster. Handle PSBTExceptions in TransactionBroadcaster + NBitcoin dependency update. Sep 3, 2019

}
else
{
throw new FormatException($"Transaction string is invalid or cannot be finalized.");

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Sep 4, 2019

Collaborator

A transaction can't be "finalized" only a PSBT is.

{
transaction = new SmartTransaction(Transaction.Parse(TransactionString, Global.Network ?? Network.Main), WalletWasabi.Models.Height.Unknown);
if (Transaction.TryParse(TransactionString, Global.Network, out var txn))

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Sep 4, 2019

Collaborator

I think this is fine, but another alternative is to just use Transaction.Parse that will also have a FormatException anyway, that would save you the else block.

@jmacato

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

@NicolasDorier i only realized now that PSBT.TryParse still throws an exception (thought it only returns a boolean whatever happens.). And the PSBTException makes the exception message handler throw a huge chunk of error text to the view. I wonder if catching the PSBTException and displaying a generic PSBT cannot be finalized message would do?

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

I wonder if catching the PSBTException

I think it makes sense, or maybe just show the first error in the list.

@jmacato

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2019

@NicolasDorier just did a bit of both, hopefully this looks okay :)

@NicolasDorier

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

Look good to me!

@nopara73 nopara73 merged commit 9eaa395 into zkSNACKs:master Sep 9, 2019

3 of 4 checks passed

Wasabi.Linux #20190904.3 failed
Details
CodeFactor No issues found.
Details
Wasabi.Osx #20190904.3 succeeded
Details
Wasabi.Windows #20190904.3 succeeded
Details

@jmacato jmacato deleted the jmacato:fix-2203 branch Sep 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.