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

ChainEndpoint trait refactor (first phase) #1001

Closed
5 of 8 tasks
adizere opened this issue May 26, 2021 · 1 comment
Closed
5 of 8 tasks

ChainEndpoint trait refactor (first phase) #1001

adizere opened this issue May 26, 2021 · 1 comment
Labels
A: good-first-issue Admin: good for newcomers I: logic Internal: related to the relaying logic O: code-hygiene Objective: cause to improve code hygiene
Milestone

Comments

@adizere
Copy link
Member

adizere commented May 26, 2021

Crate

ibc

Summary

We would like to make the methods in the Chain trait more consistent. Refactoring this trait will probably take multiple rounds of work. This issue covers a first phase, which comprises two elements that we'd like to visit:

  1. Fix some of the returned values for query_* methods, to make them consistent in the way in which they report non-existent values versus how they report a runtime/connection error.
  2. Ensure consistency across methods with respect to their input arguments.

Both of these are described in more detail below.

Problem Definition

The Chain trait is a major interface in the ibc-relayer library. The intention behind this trait is to capture all the dependencies which Hermes has towards any kind of chain (be it Cosmos SDK or something else). As it is a major connecting point between relaying logic and chain logic, this trait gets changed relatively often, and its design drifted to become less consistent over time.

Here are some examples of inconsistencies.

Return values for query methods

The query_connection method returns a domain type that is possibly in state Uninitialized, and this signifies that the Connection object does not exist on-chain. We currently have to do this domain type interpretation to report to the user correctly that an object does not exist, e.g.:
https://github.com/informalsystems/ibc-rs/blob/26ef2c6fcc2527998be1a206292ad870191b42c0/relayer-cli/src/commands/query/connection.rs#L52-L65

An alternative to returning an Uninitialized domain type is to return a value of type Result<Option<ConnectionEnd>, Error.
A returned Ok(None) indicates that the query suceedeed but nothing was found on-chain. It's not clear if this alternative design fits all queries, however, so the other queries methods should be visited in light of this proposal, and if the design is sound then we should modify the queries correspondingly.

Consistency of input arguments

Some improvements can be done for channel-related methods, specifically when both a ChannelId and a PortId are provided as input (for example here), and instead of these two types a single PortChannelId should be used.

The method query_packet_commitments can also be modified to accept the PortChannelId input, instead of QueryPacketCommitmentsRequest, but this is not clear. If the pagination field in QueryPacketCommitmentsRequest is used, then we should not change it.

Acceptance Criteria

  • The Chain trait should be more consistent across its method signatures.
  • low-hanging TODO that should also be fixed
  • follow-up work should be identified and an issue should be opened

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@adizere adizere added A: good-first-issue Admin: good for newcomers I: logic Internal: related to the relaying logic O: code-hygiene Objective: cause to improve code hygiene labels May 26, 2021
@adizere adizere added this to the 05.2021 milestone May 26, 2021
@adizere adizere modified the milestones: 05.2021, 06.2021 May 31, 2021
@ancazamfir ancazamfir modified the milestones: 06.2021, 07.2021 Jun 7, 2021
@adizere adizere modified the milestones: 07.2021, 08.2021 Jun 9, 2021
@adizere adizere modified the milestones: 08.2021, Backlog Aug 3, 2021
@adizere adizere mentioned this issue Nov 30, 2021
7 tasks
@adizere adizere changed the title Chain trait refactor (first phase) ChainEndpoint trait refactor (first phase) Jan 17, 2022
@adizere adizere modified the milestones: Backlog, v2 Apr 7, 2022
@adizere
Copy link
Member Author

adizere commented May 11, 2022

Closing as superseded by some of the work in #2192.

@adizere adizere modified the milestones: v2, v0.15.0 May 11, 2022
@adizere adizere moved this to Backlog in IBC-rs: the road to v1 May 11, 2022
@adizere adizere moved this from Backlog to Closed in IBC-rs: the road to v1 May 11, 2022
@adizere adizere closed this as completed May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: good-first-issue Admin: good for newcomers I: logic Internal: related to the relaying logic O: code-hygiene Objective: cause to improve code hygiene
Projects
No open projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

3 participants