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 Uninitialized from ConnectionEnd state #1042

Open
5 tasks
soareschen opened this issue Jun 3, 2021 · 2 comments
Open
5 tasks

Remove Uninitialized from ConnectionEnd state #1042

soareschen opened this issue Jun 3, 2021 · 2 comments
Milestone

Comments

@soareschen
Copy link
Contributor

Crate

ibc

Summary of Bug

This is a follow up on #875. When inspecting the ibc-go source code, we can see that the Uninitialized state is not really used anywhere. In fact the name of the state has been renamed to STATE_UNINITIALIZED_UNSPECIFIED.

The main reason the field is being there is because of the quirk of protobuf3, which removes the notion of required field and effectively making all fields optional. When no result is returned from ibc-go, the res.val buffer in query_connection() is in fact empty, but the deserialization from empty buffer into the RawConnectionEnd struct still succeeds because of proto3 semantics.

To fix this, I have created PR #1041 to remove the Uninitialized connection state, and return an error instead when no connection is found. However the MBT test is failing as it still models the Uninitialized state, and that would need to be updated as well.

An alternative design is to change query_connection() to return Option<ConnectionEnd> instead of Connection. However checking it would require proper refactoring of the decoding trait implementations so that the decoders should return Option values when any of the "required" fields are missing.

Acceptance Criteria

  • The relayer no longer has Uninitialized connection end state.
  • The relayer returns either an error or an Option value when a connection ID is not found.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ancazamfir
Copy link
Collaborator

An alternative design is to change query_connection() to return Option<ConnectionEnd> instead of Connection. However checking it would require proper refactoring of the decoding trait implementations so that the decoders should return Option values when any of the "required" fields are missing.

Could we discuss this alternative also?

@soareschen
Copy link
Contributor Author

Sure. For this we may have to cover the corner cases of protobuf3, and have well defined behavior on whether the decoder should return error or None. This may also apply to all other decoding of protobuf3 payloads, so we may also want to be consistent on that.

@adizere adizere added this to the Backlog milestone Jun 9, 2021
@adizere adizere assigned soareschen and unassigned soareschen Jun 9, 2021
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 a pull request may close this issue.

3 participants