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

Remove Router::has_route #503

Closed
plafer opened this issue Mar 7, 2023 · 1 comment · Fixed by #740
Closed

Remove Router::has_route #503

plafer opened this issue Mar 7, 2023 · 1 comment · Fixed by #740
Labels
A: breaking Admin: breaking change that may impact operators A: good-first-issue Admin: good for newcomers A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews
Milestone

Comments

@plafer
Copy link
Contributor

plafer commented Mar 7, 2023

We currently use has_route() in ExecutionContext::execute(), but has_route() typically is implemented as

fn has_route(&self, module_id: &ModuleId) -> bool {
    self.get_route(module_id).is_some()
}

However, get_route() (immutable) success doesn't imply get_route_mut() (mutable) will succeed. Our has_route() call is redundant with the later calls to get_route() anyways.

@plafer plafer added A: good-first-issue Admin: good for newcomers A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews labels Mar 7, 2023
@Farhad-Shabani Farhad-Shabani moved this to 📥 To Do in ibc-rs Mar 7, 2023
@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Mar 7, 2023
@PanGan21 PanGan21 mentioned this issue Jul 1, 2023
7 tasks
@PanGan21
Copy link
Contributor

PanGan21 commented Jul 1, 2023

Hey @plafer i hope pr #740 is what is meaning to be done in this issue. If I misunderstood it please let me know.

@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs Jul 5, 2023
@Farhad-Shabani Farhad-Shabani added this to the v0.42.0 milestone Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: breaking Admin: breaking change that may impact operators A: good-first-issue Admin: good for newcomers A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants