-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement verify_upgrade_and_update_state
for Tendermint Client
#349
Changes from 15 commits
9061610
73e87c1
d63fa4d
5b04088
869d8de
025a366
c968ee7
5f51c7c
a5b3f05
d0c050e
5dcf609
8ebf323
9ea805a
4155c75
a9b3198
5bc1a04
cba4c94
efff958
355cb1d
83de030
3d76bfc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
- Implement `verify_upgrade_and_update_state` method for Tendermint clients | ||
([#19](https://github.com/cosmos/ibc-rs/issues/19)). |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,12 +63,7 @@ pub trait ClientState: | |
/// Helper function to verify the upgrade client procedure. | ||
/// Resets all fields except the blockchain-specific ones, | ||
/// and updates the given fields. | ||
fn upgrade( | ||
&mut self, | ||
upgrade_height: Height, | ||
upgrade_options: &dyn UpgradeOptions, | ||
chain_id: ChainId, | ||
); | ||
fn zero_custom_fields(&mut self); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming we need this (see previous comments), I don't think this should be in the ICS-2 interface. Can't it just be a private function in the tendermint client module? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Considering that in any chain and client state struct there will be some relayer customizable fields that are needed to be zeroed out for the upgrade process. I think it makes sense to keep it here |
||
|
||
/// Convert into a boxed trait object | ||
fn into_box(self) -> Box<dyn ClientState> | ||
|
@@ -112,11 +107,30 @@ pub trait ClientState: | |
misbehaviour: Any, | ||
) -> Result<Box<dyn ClientState>, ContextError>; | ||
|
||
fn verify_upgrade_and_update_state( | ||
/// Verify the upgraded client and consensus states and validate proofs | ||
/// against the given root. | ||
/// | ||
/// NOTE: proof heights are not included as upgrade to a new revision is | ||
/// expected to pass only on the last height committed by the current | ||
/// revision. Clients are responsible for ensuring that the planned last | ||
/// height of the current revision is somehow encoded in the proof | ||
/// verification process. This is to ensure that no premature upgrades | ||
/// occur, since upgrade plans committed to by the counterparty may be | ||
/// cancelled or modified before the last planned height. | ||
fn verify_upgrade_client( | ||
&self, | ||
consensus_state: Any, | ||
upgraded_client_state: Any, | ||
upgraded_consensus_state: Any, | ||
proof_upgrade_client: MerkleProof, | ||
proof_upgrade_consensus_state: MerkleProof, | ||
root: &CommitmentRoot, | ||
) -> Result<(), ClientError>; | ||
|
||
// Update the client state and consensus state in the store with the upgraded ones. | ||
fn update_state_with_upgrade_client( | ||
&self, | ||
upgraded_client_state: Any, | ||
upgraded_consensus_state: Any, | ||
) -> Result<UpdatedState, ClientError>; | ||
|
||
/// Verification functions as specified in: | ||
|
@@ -175,7 +189,7 @@ pub trait ClientState: | |
expected_client_state: Any, | ||
) -> Result<(), ClientError>; | ||
|
||
/// Verify a `proof` that a packet has been commited. | ||
/// Verify a `proof` that a packet has been committed. | ||
#[allow(clippy::too_many_arguments)] | ||
fn verify_packet_data( | ||
&self, | ||
|
@@ -190,7 +204,7 @@ pub trait ClientState: | |
commitment: PacketCommitment, | ||
) -> Result<(), ClientError>; | ||
|
||
/// Verify a `proof` that a packet has been commited. | ||
/// Verify a `proof` that a packet has been committed. | ||
#[allow(clippy::too_many_arguments)] | ||
fn verify_packet_acknowledgement( | ||
&self, | ||
|
@@ -258,8 +272,6 @@ pub fn downcast_client_state<CS: ClientState>(h: &dyn ClientState) -> Option<&CS | |
h.as_any().downcast_ref::<CS>() | ||
} | ||
|
||
pub trait UpgradeOptions: AsAny {} | ||
|
||
pub struct UpdatedState { | ||
pub client_state: Box<dyn ClientState>, | ||
pub consensus_state: Box<dyn ConsensusState>, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that we're not building the right path, comparing with ibc-go. Can you confirm/disconfirm this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite understandable. Do you think we can keep this checked with issue #385?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If were that unsure about the current implementation, we should then put it behind a feature flag (similar to
val_exec_ctx
), which will allow us to test it with basecoin-rs, and at the same time, signal to users that this feature shouldn't be used yet.