Remove old registration flow #1030
Conversation
I thought this had already been removed, guess I missed it
We don't pre-populate registered accounts anymore. We could do this again in the future, but the setup is a little tricker since we need to ensure that the network is jumpstarted before registration can happen.
I want to remove it to see where errors pop up, but still keep the old code visible as a reference.
HCastano
added
Breaks API
Bump `spec_version`
| @@ -176,7 +176,6 @@ impl AppState { | |||
| pub fn app(app_state: AppState) -> Router { | |||
There was a problem hiding this comment.
Can you pls also remove the documentation for the /user/new endpoint from the top of this file. Line 75 or so.
| @@ -303,22 +303,6 @@ pub fn development_genesis_config( | |||
| "imOnline": ImOnlineConfig { keys: vec![] }, | |||
| "authorityDiscovery": AuthorityDiscoveryConfig { keys: vec![], ..Default::default() }, | |||
| "grandpa": GrandpaConfig { authorities: vec![], ..Default::default() }, | |||
| "registry": RegistryConfig { | |||
There was a problem hiding this comment.
I'm not sure if your intention was to remove all traces of these pre-registered accounts in this PR. But if so we are still putting their keyshares in the kvdb here:
And there is quite possibly a bunch of other places where they pop up. Maybe something for another PR.
There was a problem hiding this comment.
def at least an issue maybe should also tag js team here incase their tests rely on these
There was a problem hiding this comment.
Yeah, so there are some tests that still depend on this, e.g test_proactive_refresh.
So probably best done in a follow-up
| let (validators_info, mut signature_request, validator_ips_and_keys) = | ||
| get_sign_tx_data(&entropy_api, &rpc, hex::encode(PREIMAGE_SHOULD_SUCCEED), with_parent_key) | ||
| .await; | ||
| get_sign_tx_data(&entropy_api, &rpc, hex::encode(PREIMAGE_SHOULD_SUCCEED)).await; | ||
|
|
||
| // This verifying key doesn't have a program registered with it | ||
| signature_request.block_number = rpc.chain_get_header(None).await.unwrap().unwrap().number; |
There was a problem hiding this comment.
Im kinda surprised the check below (line 227) still passes without dave being a pre-registered account anymore - i would expect the error to be something like "User not registered" and not "No program pointer defined for account". As i understand it, it should no longer be possible to get into a state where we have a registered account with no associated programs.
There was a problem hiding this comment.
Very nice, you were spot on 🔍
thread 'user::tests::test_signature_requests_fail_on_different_conditions' panicked at crates/threshold-signature-server/src/user/tests.rs:222:9:
assertion `left == right` failed
left: "Substrate: User is not registered on-chain"
right: "No program pointer defined for account"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test user::tests::test_signature_requests_fail_on_different_conditions ... FAILED
failures:
failures:
user::tests::test_signature_requests_fail_on_different_conditions
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 30 filtered out; finished in 143.00s
Looks like the "build and step" test of the CI didn't run yesterday, which is why the pipeline is all green.
Agreed, we shouldn't be able to get into this state anymore so I'm gonna remove the test.
Yep, a lot of the docs will need updating |
After having cleaned up a lot of code and tests in previous PRs it's finally time to get
rid of the old off-chain registration flow 🥳
Note that there are quite a few breaking changes in this PR since I've removed:
extrinsics, events, errors from the Registry pallet and a public HTTP API (
/new/user)from the TSS.
Hopefully I didn't miss anything, but there may be small clean up PRs that will be opened
over the next few weeks if I did.