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

Another multisig test #72

Merged
merged 2 commits into from
Apr 20, 2023
Merged

Another multisig test #72

merged 2 commits into from
Apr 20, 2023

Conversation

JoshOrndorff
Copy link
Contributor

When I was recently preparing the Substrate update in #67, I was briefly confused about the tests on the ThresholdMultisig. Once I read the tests carefully, I understood them and quickly got them passing.

However, I noticed that there was not a test for the case where the redeemer contains more than enough valid signatures. This PR adds a single test for when there is a 2/3 multisig, and the redeemer contains valid sigs for all three signatories even though only two are required. I expected this case to still successfully verify, but it seems it is not.

@coax1d is this the behavior you expected?

Copy link
Contributor

@coax1d coax1d left a comment

Choose a reason for hiding this comment

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

Think there was just a small glitch if I understand correctly

tuxedo-core/src/verifier.rs Outdated Show resolved Hide resolved
Co-authored-by: Andrew <andrewburger1991@gmail.com>
@JoshOrndorff JoshOrndorff marked this pull request as ready for review April 20, 2023 17:44
@coax1d
Copy link
Contributor

coax1d commented Apr 20, 2023

Looks good tho good test to add!

Copy link
Contributor

@coax1d coax1d left a comment

Choose a reason for hiding this comment

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

LGTM!

@JoshOrndorff JoshOrndorff merged commit fd7b07b into main Apr 20, 2023
@JoshOrndorff JoshOrndorff deleted the extra-multisig-test branch October 18, 2023 02:54
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.

2 participants