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

Rework ibc::ics04_channel::events module #86

Closed
5 tasks done
romac opened this issue Mar 1, 2021 · 3 comments
Closed
5 tasks done

Rework ibc::ics04_channel::events module #86

romac opened this issue Mar 1, 2021 · 3 comments
Labels
O: new-feature Objective: aims to add new feature

Comments

@romac
Copy link
Member

romac commented Mar 1, 2021

Crate

ibc

Summary

The various channel events defined in the ibc::ics04_channel::events module are all defined as newtypes over the same Attributes which has some optional fields which are expected to always be defined for specific events. As such, this forces us to unwrap these values in some places. We should refactor this module to perform this check when parsing the source event, and perhaps refactor the parsing code a little bit while we're at it.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@romac romac added the O: new-feature Objective: aims to add new feature label Mar 1, 2021
@romac romac self-assigned this Mar 1, 2021
@romac romac removed their assignment Dec 1, 2021
@Wizdave97
Copy link

Wizdave97 commented Feb 7, 2022

@romac @adizere I'll like to work on this, I'll need a bit more detail on your idea of what th solution should look like.

@romac
Copy link
Member Author

romac commented Feb 7, 2022

The main idea is to go from this:

https://github.com/informalsystems/ibc-rs/blob/e16d5f64fdf7bbd6e0fa9449a567ad9ee1b3c180/modules/src/core/ics04_channel/events.rs#L510-L541

to a bare struct with only the necessary fields made public:

#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq)]
pub struct CloseInit {
    pub height: Height,
    pub port_id: PortId,
    pub channel_id: ChannelId,
    pub counterparty_port_id: PortId,
    pub counterparty_channel_id: Option<ChannelId>,
}

Note that now we statically ensure that CloseInit has a channel_id whereas we were previously forced to unwrap the Option<ChannelId> contained in the Attributes.

@Wizdave97
Copy link

Wizdave97 commented Feb 8, 2022

@romac @adizere I've made a draft implementation here https://github.com/Wizdave97/ibc-rs/blob/david/rework-channel-events/modules/src/core/ics04_channel/events.rs, I want to know if that's on track, especially the use of TryFrom<Attributes> instead of From<Attributes> since the other events still use optional channel_id values, or do I enforce existence of channel_id on all other events?

@hu55a1n1 hu55a1n1 transferred this issue from informalsystems/hermes Sep 29, 2022
hu55a1n1 pushed a commit that referenced this issue Sep 29, 2022
* made Msg trait global

* MsgConnectionOpenTry implementation

* Connection msgs skeleton

* channel msgs skeleton

* validation fixes

* Added tests for connection and channel msgs

* fix clippy error

* fix further errors

* review suggestions
livelybug pushed a commit to octopus-network/ibc-rs that referenced this issue Oct 14, 2022
* made Msg trait global

* MsgConnectionOpenTry implementation

* Connection msgs skeleton

* channel msgs skeleton

* validation fixes

* Added tests for connection and channel msgs

* fix clippy error

* fix further errors

* review suggestions
shuoer86 pushed a commit to shuoer86/ibc-rs that referenced this issue Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: new-feature Objective: aims to add new feature
Projects
None yet
Development

No branches or pull requests

2 participants