Skip to content
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

Split MsgUpdateClient back into MsgUpdateClient and MsgSubmitMisbehaviour #643

Merged
merged 14 commits into from
Apr 21, 2023

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Apr 21, 2023

Closes: #628

cc @hdevalence


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

Patch coverage: 55.26% and project coverage change: -0.04 ⚠️

Comparison is base (9bb6327) 73.00% compared to head (858f8c0) 72.96%.

❗ Current head 858f8c0 differs from pull request most recent head 20f45dd. Consider uploading reports for the commit 20f45dd to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #643      +/-   ##
==========================================
- Coverage   73.00%   72.96%   -0.04%     
==========================================
  Files         125      126       +1     
  Lines       15663    15686      +23     
==========================================
+ Hits        11434    11446      +12     
- Misses       4229     4240      +11     
Impacted Files Coverage Δ
crates/ibc/src/core/handler.rs 74.64% <ø> (ø)
crates/ibc/src/core/ics02_client/client_state.rs 68.18% <0.00%> (-3.25%) ⬇️
crates/ibc/src/core/ics26_routing/msgs.rs 1.11% <0.00%> (ø)
...tes/ibc/src/core/ics02_client/msgs/misbehaviour.rs 4.54% <4.54%> (ø)
crates/ibc/src/core/context.rs 83.33% <50.00%> (-1.73%) ⬇️
...ibc/src/core/ics02_client/handler/update_client.rs 95.23% <82.60%> (-4.77%) ⬇️
...s/ibc/src/clients/ics07_tendermint/client_state.rs 68.38% <100.00%> (-0.06%) ⬇️
crates/ibc/src/core/ics02_client/msgs.rs 100.00% <100.00%> (ø)
...es/ibc/src/core/ics02_client/msgs/update_client.rs 100.00% <100.00%> (+40.42%) ⬆️
crates/ibc/src/mock/client_state.rs 90.22% <100.00%> (-0.05%) ⬇️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 42 to 44
let domain_msg =
<update_client::MsgUpdateClient as Protobuf<RawMsgUpdateClient>>::decode_vec(
&any_msg.value,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write update_client::MsgUpdateClient::decode_vec(&any_msg.value) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update_client::MISBEHAVIOUR_TYPE_URL => {
let domain_msg = <update_client::MsgUpdateClient as Protobuf<
misbehaviour::TYPE_URL => {
let domain_msg = <misbehaviour::MsgSubmitMisbehaviour as Protobuf<
RawMsgSubmitMisbehaviour,
>>::decode_vec(&any_msg.value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here misbehaviour::MsgSubmitMisbehaviour::decode_vec(&any_msg.value) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -34,8 +19,7 @@ pub enum UpdateKind {
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct MsgUpdateClient {
pub client_id: ClientId,
pub client_message: Any,
pub update_kind: UpdateKind,
pub header: Any,
pub signer: Signer,
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we re-add Msg implementation to be consistent with others?

impl Msg for MsgUpdateClient {
    type Raw = RawMsgUpdateClient;

    fn type_url(&self) -> String {
        TYPE_URL.to_string()
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Farhad-Shabani Farhad-Shabani merged commit 87f2f54 into main Apr 21, 2023
@Farhad-Shabani Farhad-Shabani deleted the plafer/fix-client-domain-types branch April 21, 2023 17:51
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
…behaviour` (#643)

* Add misbehaviour message back

* remove Protobuf<Misbehaviour> for UpdateClient message

* new `MsgUpdateOrMisbehaviour`

* Revert "new `MsgUpdateOrMisbehaviour`"

This reverts commit ff6a968.

* Fix ClientState::update_state's signature

* Update update_client::validate

* fix interface

* fix TYPE_URL

* Move UpdateKind

* changelog

* fmt

* simplify syntax

* impl Msg for MsgUpdateClient
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MsgUpdateClient domain type does not faithfully represent the IBC protocol messages
2 participants