[Context] Issues caused by improper output signatures of provable store readers

[Farhad-Shabani]

Problem Statement

There is a discrepancy between the output signature/type of methods (under ValidationContext) that retrieve datagrams from a provable store, e.g.:

• For the next_consensus_state and prev_consensus_state interfaces we have Result<Option<Output>, ContextError>
• Other places, reader methods use Result<Output, ContextError> type

Meaning sometimes we denote unavailability with None and other times as an Error

---

Why does this matter and should a consistent approach be taken? and clearly differentiate cases where an output represents (1) existence/absence of a state in storage and any other errors that show (2) failure scenarios (like broken storage, etc) when performing a query?

• Looking into the acceptable absences
  There are places we operate based on a lack of datagrams and treat them as an expected/acceptable execution route, like:

  • No-op case when calling get_packet_commitment
  • Ack must not exist upon calling get_packet_acknowledgment
  • If a relayer hasn't yet relayed a packet (lack of receipt) from a get_packet_receipt call, we should act afterward.
  • The case arose in Check if ClientStatePath is empty during client creation #605, where the ClientStateNotFound is a valid case, shouldn't be treated as an error. In contrast, ClientStateAlreadyExists correctly represents the existence error.

• Needing self-contained approach
  The correctness of our operations relies on implementors. Hoping they select the right error variant and do not use any other errors under the ContextError enum, too. E.g. here

• Moving towards errors with concerns separated
  This is consistent with the view presented here splitting errors into categories of protocol-specific and host-related errors, where:

  1. The first will be responsible for handling (1) Option<Output> concerned only with the IBC implementation
  2. The latter deals with (2) propagating errors of the Result<Option<Output>, Error> type that occurred in hosts' boundary

• Avoiding side effects
  And, with the current signature, our error system becomes useless and fails to propagate the correct failure reason in a bunch of places, as we eventually map them to our assumed/desired errors like here & here

Proposal

• Leaving with the Result<Option<Output>, Error> signature for the reader methods (It might not apply to all, but seems like it should)
• Why not Result<Result<Output, ProtocolError> HostError> instead?
  1. As mentioned, some places treat the absence of a datagram as a reasonable/expected behavior
  2. Requires implementors to introduce an additional ProtocolError everywhere, which must be exactly the same as the one we defined to represent that non-existence case. (means they have no other options). This shouldn't be their concern and better to be handled internally.

Remark

• Separated from Make core Error enums less specific #270 due to critical issues associated with this and needs to be fixed before that. We are unsure how different hosts have handled errors, and some of their error scenarios may bypass the cases mentioned above, particularly during packet processings.