feat(sequencer)!: implement RecoverClient action based on sequencer-v1.0.0
#2001
Conversation
noot
requested review from
SuperFluffy,
Fraser999,
a team and
joroshiba
as code owners
github-actions
bot
added
proto
| @@ -22,6 +22,7 @@ message Action { | |||
| // IBC user actions are defined on 21-30 | |||
| astria_vendored.penumbra.core.component.ibc.v1.IbcRelay ibc = 21; | |||
| Ics20Withdrawal ics20_withdrawal = 22; | |||
| RecoverClient recover_client = 23; | |||
There was a problem hiding this comment.
This should probably be under the POA sudo actions?
| .address_bytes(); | ||
| ensure!( | ||
| from == state | ||
| .get_sudo_address() |
There was a problem hiding this comment.
Maybe utilize the ibc_sudo instead of the overall sudo? The authority that controls ibc access gates this as well?
| } | ||
| } | ||
|
|
||
| message IbcSudoChange { | ||
| astria.primitive.v1.Address new_address = 1; | ||
| } | ||
|
|
||
| message RecoverClient { |
There was a problem hiding this comment.
Should we note this is IBC client call ie RecoverIbcClient?
Can we add a note above on what the exepcted outcome of the action is?
| // the client identifier for the client to be updated | ||
| string subject_client_id = 1; |
There was a problem hiding this comment.
I know we copied this from cosmos, but their wording seems odd to me?
Propose we make this fieldclient_id to matches every other message in the client space which utilizes this as the field which action modifies.
| // the client identifier for the client to be updated | |
| string subject_client_id = 1; | |
| // the client which action is applying to, and is being recovered | |
| string client_id = 1; |
There was a problem hiding this comment.
don't really like just client_id as it's too ambiguous imo and will be more confusing reading the code. could do client_id_to_replace and substitute_client_id, or subject_client_id and replacement_client_id?
| string subject_client_id = 1; | ||
| // the substitute client identifier for the client which will replace the subject | ||
| // client | ||
| string substitute_client_id = 2; |
There was a problem hiding this comment.
We could also change this to replacement_client_id? But I'm not super opinionated on this.
Main thing I find subject substitute requires to much brain thought for which is which. (Both start with sub, in other conversation you might say sub[stituted] out vs sub in ).
I think the client_id is the thing being recovered the replacement is very clearly the thing which is being put in its place.
| .get_block_timestamp() | ||
| .await | ||
| .wrap_err("failed to get timestamp")?; | ||
| let subject_client_status = state |
There was a problem hiding this comment.
similar nits as in proto on naming here just too many things starting with sub makes it a slower read?
| let subject_client_status = state | ||
| .get_client_status(&self.subject_client_id, timestamp) | ||
| .await; | ||
|
|
There was a problem hiding this comment.
nit: maybe new line not needed between the fetch/verification of object
| let substitute_client_status = state | ||
| .get_client_status(&self.substitute_client_id, timestamp) | ||
| .await; | ||
|
|
| )?; | ||
|
|
||
| subject_client_state.latest_height = substitute_client_state.latest_height; | ||
| subject_client_state.chain_id = substitute_client_state.chain_id; |
There was a problem hiding this comment.
Should we actually have an ensure block on this? To make it so that recovery can never change the chain being acted on?
I suppose there may be a usecase here were there was a hardfork in chain and client expired but assets are considered the same?
Chain id changing hardforks are rare, nearly all the time we probably want to replace with a client of the same chain id and not doing so would be a safety failure? I'd be ok just not supporting that usecase at this time, and having to evaluate what to do if it does come up?
There was a problem hiding this comment.
the spec allows the chain ID to be changed - https://ibc.cosmos.network/architecture/adr-026-ibc-client-recovery-mechanisms/ although i'm not sure exactly why. this might be for the case you said where the counterparty forks and changes their chain ID, but we didn't do a client upgrade in time, which would invalidate the client.
noot
changed the title
RecoverClient actionRecoverClient action based on sequencer-v1.0.0
Summary
implement
RecoverClientaction which replaces an expired or otherwise inactive IBC client.this PR is based off the sequencer-v1.0.0 release tag.
this is based off
MsgRecoverClientimplemented in ibc-go v8: https://github.com/cosmos/ibc-go/blob/a71577cc1bf242f8c99360777106b1321503b17f/proto/ibc/core/client/v1/tx.proto#L126Background
there is no way to otherwise replace an expired IBC client without creating a new one and creating a governance proposal on the counterparty chain to change the client there, which is a lot more hassle than just being able to replace an expired client on one chain.
Changes
RecoverClientactionTesting
not yet, will be done on devnet
Changelogs
Ensure all relevant changelog files are updated as necessary. See
keepachangelog for change
categories. Replace this text with e.g. "Changelogs updated." or "No updates
required." to acknowledge changelogs have been considered.
Breaking Changelist