Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Missing store_connection_channels after ExecutionContext refactor #479

Closed
kevinji opened this issue Feb 28, 2023 · 2 comments · Fixed by #484
Closed

Missing store_connection_channels after ExecutionContext refactor #479

kevinji opened this issue Feb 28, 2023 · 2 comments · Fixed by #484
Assignees
Labels
A: breaking Admin: breaking change that may impact operators A: good-first-issue Admin: good for newcomers O: usability Objective: aims to enhance user experience (UX) and streamline product usability
Milestone

Comments

@kevinji
Copy link
Contributor

kevinji commented Feb 28, 2023

Bug Summary

The function

fn store_connection_channels(
    &mut self,
    connection_id: ConnectionId,
    port_id: PortId,
    channel_id: ChannelId,
) -> Result<(), ContextError>;

got removed during the ExecutionContext refactor, but

fn connection_channels(
    &self,
    connection_id: &ConnectionId,
) -> Result<Vec<(PortId, ChannelId)>, ContextError>;

remains in ValidationContext.

Details

Since store_connection_channels got removed from ExecutionContext, I think there isn't a function any more that ties connections to (port, channel) pairs anymore, meaning connection_channels would always return an error.

Version

ibc-rs v0.29.0 onwards.

@plafer plafer added A: good-first-issue Admin: good for newcomers O: usability Objective: aims to enhance user experience (UX) and streamline product usability labels Feb 28, 2023
@Farhad-Shabani
Copy link
Member

It can still be retrieved by querying for all channels using ChannelEndPath and matching the passed connection_id with the connection_hops field of each found ChannelEnd. (like here in basecoin-rs).
Though indeed, it is not an efficient way and in some implementations can end up with the error.

@Farhad-Shabani Farhad-Shabani moved this to 📥 To Do in ibc-rs Feb 28, 2023
@plafer
Copy link
Contributor

plafer commented Feb 28, 2023

I think the main point though is that it is no longer used by the core handlers, and so serves no purpose. So IIUC, we should just remove ValidationContext::connection_channels().

@Farhad-Shabani Farhad-Shabani self-assigned this Feb 28, 2023
@Farhad-Shabani Farhad-Shabani added this to the v0.31.0 milestone Feb 28, 2023
@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs Feb 28, 2023
@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Feb 28, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A: breaking Admin: breaking change that may impact operators A: good-first-issue Admin: good for newcomers O: usability Objective: aims to enhance user experience (UX) and streamline product usability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants