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

Cleanup access to state handler #1860

Merged

Conversation

Wizdave97
Copy link
Contributor

Closes: informalsystems/ibc-rs#631

Description

Remove getters from client, connection and channel messages


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

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

@Wizdave97 Wizdave97 changed the title David/cleanup access to state handler Ceanup access to state handler Feb 9, 2022
@romac romac changed the title Ceanup access to state handler Cleanup access to state handler Feb 10, 2022
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Very nice, thanks a lot!

Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Can you please fix the clippy warnings?

@Wizdave97 Wizdave97 requested a review from romac February 11, 2022 07:20
@romac
Copy link
Member

romac commented Feb 11, 2022

@Wizdave97 Can you please either fix the two remaining warnings by applying this patch, or allow edits by maintainers for this PR?

diff --git a/modules/src/core/ics03_connection/handler/conn_open_confirm.rs b/modules/src/core/ics03_connection/handler/conn_open_confirm.rs
index b32605be..a2849217 100644
--- a/modules/src/core/ics03_connection/handler/conn_open_confirm.rs
+++ b/modules/src/core/ics03_connection/handler/conn_open_confirm.rs
@@ -141,7 +141,7 @@ mod tests {
                 ctx: context
                     .with_client(&client_id, Height::new(0, 10))
                     .with_connection(msg_confirm.connection_id.clone(), correct_conn_end),
-                msg: ConnectionMsg::ConnectionOpenConfirm(msg_confirm.clone()),
+                msg: ConnectionMsg::ConnectionOpenConfirm(msg_confirm),
                 want_pass: true,
             },
         ]
diff --git a/modules/src/core/ics04_channel/handler/chan_open_init.rs b/modules/src/core/ics04_channel/handler/chan_open_init.rs
index 81a01760..d37edc6d 100644
--- a/modules/src/core/ics04_channel/handler/chan_open_init.rs
+++ b/modules/src/core/ics04_channel/handler/chan_open_init.rs
@@ -146,8 +146,7 @@ mod tests {
                     .with_port_capability(
                         MsgChannelOpenInit::try_from(get_dummy_raw_msg_chan_open_init())
                             .unwrap()
-                            .port_id
-                            .clone(),
+                            .port_id,
                     ),
                 msg: ChannelMsg::ChannelOpenInit(msg_chan_init),
                 want_pass: true,

Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Thanks so much, @Wizdave97!

@romac romac merged commit 01c2983 into informalsystems:master Feb 11, 2022
@Wizdave97
Copy link
Contributor Author

Thanks so much, @Wizdave97!

My pleasure, @romac is there like an active platform where the contributors to this project meet and discuss technical details of the project, I would like to work on more technical tasks, but I my understanding is still limited, If there is a platform I'd love to join.

@Wizdave97 Wizdave97 deleted the david/cleanup-access-to-state-handler branch February 11, 2022 14:47
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.

2 participants