search

entropyxyz / entropy-core

Protocol and cryptography development.
https://docs.entropy.xyz/
GNU Affero General Public License v3.0
9 stars 1 forks source link

Update TSS tests to use new registration flow #1026

Closed HCastano closed 1 month ago

HCastano commented 1 month ago

In #1021 I #[ignore]'d a few tests that dependend on the old registration flow. In this PR I bring (most of) those tests back and update them to use the new registration flow.

The main thing I wanted to achieve here was to not lose any test coverage switching to the new flow.

This was a pain to go through due to the slow run time of the tests (anywhere from 1-4 mins) and the jumbled nature of some tests (e.g many things being tested as a time).

To help with this I split out some test sections into their own tests. There's definitely still a lot of work to be done in cleaning up some of the tests (like test_sign_tx_no_chain, now test_signature_requests_fail_on_different_conditions), but that's something to address in later PRs.

After this PR I think we'll be good to remove the old registration flow completely.

HCastano commented 1 month ago

Great. Much clearer naming of the tests and comments.

🙏

There are still a few unused variable warnings in tests.

Yep fixed - I basically just pushed as soon as all the tests passed lol

As you mentioned, we could probably make this more concise if we had a helper which stored a program and registered a user.

Also, almost every time we call get_sign_tx_data, we change the verifying key in the signature request afterwards - so maybe this function should take the verifying key as an argument.

Maybe we can remove the pre-registered users from the chainspec in a follow-up, since we are no longer using them in these tests.

Agreed with all of these. Let's leave them for follow ups though.