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

some better UTXOW errors #1458

Merged
merged 2 commits into from May 18, 2020
Merged

some better UTXOW errors #1458

merged 2 commits into from May 18, 2020

Conversation

JaredCorduan
Copy link
Contributor

This PR adds better errors for InvalidWitnessesUTXOW and MissingVKeyWitnessesUTXOW.

I hope to be able to add robust error types for all the ledger failures in the near future. But please let me know if there are certain failures that you would like me to prioritize.

closes #1456

Copy link
Contributor

@erikd erikd left a comment

Choose a reason for hiding this comment

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

That certainly looks like an improvement.

Bool
verifiedWits (Tx txbody wits _ _) =
all (verifyWitVKey $ hashWithSerialiser toCBOR txbody) wits
[VKey 'Witness crypto]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very confusing! The function verifiedWits returns the list of witnesses that didn't verify?

In such a case, I think it's better to use Either [VKey 'Witness crypto] () (or at least a heavy comment flagging the unexpected behaviour!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thank you, that's way better

@@ -160,10 +171,11 @@ utxoWitnessed =
?! MissingScriptWitnessesUTXOW

-- check VKey witnesses
verifiedWits tx ?! InvalidWitnessesUTXOW
let failedWits = verifiedWits tx
failedWits == mempty ?! InvalidWitnessesUTXOW failedWits
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that Either [..] () will let you use ?!: :-D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's like Christmas in May! 🎄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(seriously, that's great)


let needed = witsVKeyNeeded utxo tx genDelegs
needed `Set.isSubsetOf` witsKeyHashes ?! MissingVKeyWitnessesUTXOW
needed `Set.isSubsetOf` witsKeyHashes ?! MissingVKeyWitnessesUTXOW (needed `Set.difference` witsKeyHashes)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this operation is on small enough sets that we don't care too much about repeating work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, I thought it read nicer without the let binding, and that the extra work in the error case was fine. but maybe that's silly.

@JaredCorduan JaredCorduan merged commit e4d609c into master May 18, 2020
@iohk-bors iohk-bors bot deleted the jc/utxow-better-errors branch May 18, 2020 15:06
@intricate
Copy link
Contributor

Thanks @JaredCorduan

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.

Include missing witnesses with MissingVKeyWitnessesUTXOW error
4 participants