informalsystems / hermes

IBC Relayer in Rust
https://hermes.informal.systems
Apache License 2.0
434 stars 319 forks source link

Remove Uninitialized from ConnectionEnd state #1042

Open soareschen opened 3 years ago

soareschen commented 3 years ago

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


For Admin Use

ancazamfir commented 3 years ago

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 commented 3 years ago

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.