Update TSS tests to use new registration flow #1026
Conversation
The `register_and_sign` integration test covers the same functionality now.
This isn't relevant in the context of registration anymore. The functionality of this test was mostly moved to `test_jumpstart_network` anyways.
Not sure if we still want this, or tbh how to fix it
This better reflects what the variable actually is
| let add_parent_key_to_kvdb = true; | ||
| let (_validator_ips, _validator_ids) = spawn_testing_validators(add_parent_key_to_kvdb).await; | ||
|
|
||
| // Here we need to use `--chain=integration-tests` and force authoring otherwise we won't be | ||
| // able to get our chain in the right state to be jump started. | ||
| let force_authoring = true; | ||
| let substrate_context = test_node_process_testing_state(force_authoring).await; | ||
| let entropy_api = get_api(&substrate_context.ws_url).await.unwrap(); | ||
| let rpc = get_rpc(&substrate_context.ws_url).await.unwrap(); | ||
|
|
||
| // We first need to jump start the network and grab the resulting network wide verifying key | ||
| // for later | ||
| jump_start_network(&entropy_api, &rpc).await; |
There was a problem hiding this comment.
Since this pattern is so common we may want to put it in its own function, but maybe future work
| clean_tests(); | ||
| } | ||
|
|
||
| #[ignore] | ||
| #[tokio::test] | ||
| #[serial] | ||
| async fn test_program_with_config() { | ||
| async fn test_fails_to_sign_if_non_signing_group_participants_are_used() { |
There was a problem hiding this comment.
The diff is a bit confusing here, but this test was split out from the old test_sign_tx_no_chain test.
@ameba23 do you think this is still needed in its current form? If so, can you maybe take a look at it for me, it's failing and tbh I'm not sure how to fix it 🙏
There was a problem hiding this comment.
This functionality is covered in register_and_sign.rs
There was a problem hiding this comment.
Great. Much clearer naming of the tests and comments.
There are still a few unused variable warnings in tests.
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.
| request_author: signature_request_account.clone(), | ||
| }); | ||
| let verifying_key = { | ||
| let registration_request = put_new_register_request_on_chain( |
There was a problem hiding this comment.
If we are not specifically testing registering here, it would be less verbose to use the register function from entropy-client
There was a problem hiding this comment.
Yep, I've updated all the tests except the registration one to use the test client instead
|
|
||
| // Test: We check that an account with a program succeeds in submiting a signature request | ||
|
|
||
| // The account we registered does have a program pointer, so this should succeed | ||
| update_programs( |
There was a problem hiding this comment.
Why do we need to update programs here? We set a valid program when we registered above.
To make it clearer I would move this lower down to where we test aux data for multiple programs
There was a problem hiding this comment.
You're right, I've moved it.
This goes to show my point - the subtest setup within the test is jumbled 😄
| DAVE_VERIFYING_KEY, | ||
| &one.pair(), | ||
| verifying_key.as_slice().try_into().unwrap(), | ||
| &two.pair(), | ||
| OtherBoundedVec(vec![ | ||
| OtherProgramInstance { program_pointer: program_hash, program_config: vec![] }, |
There was a problem hiding this comment.
Unrelated to this PR, but it looks to me like ProgramInstance and OtherProgramInstance are the same type. BoundedVec and OtherBoundedVec are also the same.
There was a problem hiding this comment.
Yeah I noticed that too. A bit strange and I would like to look into it at some point since its probably unnecessary
…itte (#1028) * Update error message for unexpected participant * Fix test
🙏
Yep fixed - I basically just pushed as soon as all the tests passed lol
Agreed with all of these. Let's leave them for follow ups though. |
In #1021 I
#[ignore]'d a few tests that dependend on the old registration flow. In thisPR 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, nowtest_signature_requests_fail_on_different_conditions), butthat's something to address in later PRs.
After this PR I think we'll be good to remove the old registration flow completely.