Rotate parent network key takes 2 steps #1018
Conversation
…te-parent-network-key-2-steps
JesseAbram
added
Bump `spec_version`
| // TODO: should this be a two step process? see # https://github.com/entropyxyz/entropy-core/issues/968 | ||
| if app_state.kv_store.kv().exists(&network_parent_key).await? { | ||
| app_state.kv_store.kv().delete(&network_parent_key).await? | ||
| let next_network_parent_key = hex::encode(NEXT_NETWORK_PARENT_KEY); |
There was a problem hiding this comment.
Why does this need to be encoded as hex?
There was a problem hiding this comment.
the type that the kv takes needs it, but tbh should probably make the const that
| serialize(&aux_info_after_rotate).unwrap() | ||
| ); | ||
|
|
||
| // calling twice doesn't do anything |
HCastano
changed the title
|
Will take a look at this on Monday |
Co-authored-by: peg <[email protected]>
…te-parent-network-key-2-steps
There was a problem hiding this comment.
So I don't necessarily think that a two phase approach is bad, but I'm also not sure if we should be adding an extra OCW flow just yet.
I'd either want to see us explore other approaches (e.g maybe just having the TSS subscribe to an on-chain event post-confirmation before deleting), or see where this actually ends up failing in practice with the current implementation.
| let latest_block_number = rpc | ||
| .chain_get_header(None) | ||
| .await? | ||
| .ok_or_else(|| ValidatorErr::OptionUnwrapError("Failed to get block number".to_string()))? | ||
| .number; |
There was a problem hiding this comment.
Here it makes sense to check using the finalized header
There was a problem hiding this comment.
no , I think that would fail as this needs to match when the OCW is fired and that isn't on finalized but on block creation
| /// Key Rotate Message passed to validators | ||
| /// parameters. [BlockNumberFor<T>] |
There was a problem hiding this comment.
| /// Key Rotate Message passed to validators | |
| /// parameters. [BlockNumberFor<T>] | |
| /// Key Rotate Message passed to validators parameters. [SOMETHING_MORE_DESCRIPTIVE] |
| let network_parent_key_heading = hex::encode(NETWORK_PARENT_KEY); | ||
| let next_network_parent_key_heading = hex::encode(NEXT_NETWORK_PARENT_KEY); | ||
|
|
||
| let new_parent_key = app_state.kv_store.kv().get(&next_network_parent_key_heading).await?; | ||
|
|
||
| if app_state.kv_store.kv().exists(&network_parent_key_heading).await? { | ||
| app_state.kv_store.kv().delete(&network_parent_key_heading).await?; | ||
| }; | ||
|
|
||
| app_state.kv_store.kv().delete(&next_network_parent_key_heading).await?; | ||
|
|
||
| let reservation = app_state.kv_store.kv().reserve_key(network_parent_key_heading).await?; | ||
| app_state.kv_store.kv().put(reservation, new_parent_key).await?; |
There was a problem hiding this comment.
Does this mean we should get rid of the is_signer_or_delete_parent_key step in the other flow? We want to wait for this to be called before deleting the parent key
There was a problem hiding this comment.
mmm yes that is a good point, that should be moved to the second step
HCastano
removed
the
Bump `impl_version`
personally Im more in favour of pushing then pulling from the TSS, plus I mean that would break convention here, and it is like we right now always have a flow that the chain pushes to the TSS but randomly we set up one long running listener, I know the ocw code is a little more verbose but I feel pretty strongly about keeping only push methods |
…te-parent-network-key-2-steps
Co-authored-by: Hernando Castano <[email protected]>
Closes #968