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

Check that collateral inputs must be signed #2277

Merged
merged 2 commits into from
May 12, 2021
Merged

Check that collateral inputs must be signed #2277

merged 2 commits into from
May 12, 2021

Conversation

nc6
Copy link
Contributor

@nc6 nc6 commented May 11, 2021

This PR overrides the witsVKeyNeeded function in Alonzo, in line with the updated Alonzo spec.

This brings the code into line with the updated spec, which overrides
this function. It also addresses CAD-2937, and ensures that signers must
be present for all collateral inputs.
GenDelegs (Crypto era) ->
WitHashes (Crypto era)
witsVKeyNeeded utxo' tx genDelegs =
WitHashes $
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you defined witsVKeyNeeded from scratch, rather than unioning the collateral input key set with the Shelley definition of witsVKeyNeeded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My policy here has always been to follow the spec, in order to make it easier to check we're doing the same thing. In this case, the spec overrides the whole witsVKeyNeeded function, so I did the same.

That having been said, I'm totally fine with doing this in a different way.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a good policy. It is indeed that way in the spec, but only because I thought it would be weird to either rename the function to something else or to say something like ShelleyMA.witsVKeyNeeded in the spec. Do you think that is a better idea to rename it to a new function, and do it that way in both code and spec?

Copy link
Contributor

@polinavino polinavino May 11, 2021

Choose a reason for hiding this comment

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

I am fine leaving it as is now, too

Note that this also changes the `validatingTxBody` in order to remove
the required signers. These were not part of any existing test, and
removing them allowed me to reuse this body in the collateral test.
Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @nc6 !

@nc6 nc6 merged commit 02aff33 into master May 12, 2021
@iohk-bors iohk-bors bot deleted the nc/fee-signers branch May 12, 2021 08:08
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.

3 participants