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

connection msgs.rs split #268

Merged
merged 4 commits into from
Sep 30, 2020
Merged

connection msgs.rs split #268

merged 4 commits into from
Sep 30, 2020

Conversation

ancazamfir
Copy link
Collaborator

Closes: informalsystems/ibc-rs#112

Description

Split the ics03_connection/msgs.rs, added From for all connection messages and tests


For contributor use:

  • Unit tests written
  • Added test to CI if applicable
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer

@ancazamfir ancazamfir requested a review from romac as a code owner September 29, 2020 12:33
@ancazamfir ancazamfir self-assigned this Sep 29, 2020
Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Looks much cleaner like this! Looks good to me.

@@ -5,7 +5,7 @@ use crate::ics03_connection::error::{Error, Kind};
use crate::ics03_connection::handler::verify::{check_client_consensus_height, verify_proofs};
use crate::ics03_connection::handler::ConnectionEvent::ConnOpenTry;
use crate::ics03_connection::handler::ConnectionResult;
use crate::ics03_connection::msgs::MsgConnectionOpenTry;
use crate::ics03_connection::msgs::conn_open_try::MsgConnectionOpenTry;
Copy link
Member

Choose a reason for hiding this comment

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

In a separate, later PR, we can consider using the trick Greg used in ibc-proto, to shorten the import paths. So instead of

use crate::ics03_connection::msgs::conn_open_try::MsgConnectionOpenTry;

we could get

use crate::ics03_connection::msgs::MsgConnectionOpenTry;

by using in msgs.rs something like:

mod conn_open_try;
pub use conn_open_try::*;

This improvement could benefit multiple other parts of our codebase, not just msgs. I will open an issue for this later on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. I tried this briefly but couldn't make it work and ran out of time.

Copy link
Member

Choose a reason for hiding this comment

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

Ah in fact there is already an issue open for this: informalsystems/ibc-rs#186. We'll solve this later!

@ancazamfir ancazamfir merged commit ec9dafb into master Sep 30, 2020
@ancazamfir ancazamfir deleted the anca/conn_msg_split branch September 30, 2020 10:10
@adizere adizere modified the milestone: v0.0.4 Sep 30, 2020
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Split connection msgs

* Cleanup and added From impls and tests for all messages

* forgotten merge conflict resolution
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