Check if ClientStatePath is empty during client creation
#605
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #605 +/- ##
=======================================
Coverage 72.88% 72.88%
=======================================
Files 126 126
Lines 15699 15703 +4
=======================================
+ Hits 11442 11445 +3
- Misses 4257 4258 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| ClientError::ClientIdentifierConstructor { | ||
| client_type: client_state.client_type(), | ||
| counter: id_counter, | ||
| validation_error: e, | ||
| } | ||
| })?; | ||
|
|
||
| let stored_client_state = ctx.client_state(&client_id); | ||
| match stored_client_state { | ||
| Err(ContextError::ClientError(ClientError::ClientStateNotFound { client_id: _ })) => (), |
There was a problem hiding this comment.
I wouldn't rely on users actually returning ClientStateNotFound. As we do elsewhere, we should just check is_err() here. I would wait for my suggested error system redesign to be implemented before we match on specific errors returned from users
There was a problem hiding this comment.
I see, but I'd like to point out that relying on the is_err() still leaves the validation step dependent on the users. wouldn't be much better. If client_state() fails due to any other reasons such as "broken storage" or "unable to fetch from storage", etc... the validation will wrongly pass, which is why I created issue #607.
I think, this is a significant one needing to be addressed even before we begin to refactor our errors.
There was a problem hiding this comment.
Note that using is_err() achieves the exact same behavior from ibc-go. Being more specific about which error occurred would be good for sure, but I consider using is_err() as at least following the same standard as ibc-go.
There was a problem hiding this comment.
Yes, though IBC-go's situation is better than ours. The nil is handled internally, users' preferences in error picking do not affect that. Plus, the storage is in a more known and clearer position with respect to the entire IBC implementation, which makes it less susceptible to errors, but it still suffers from this problem.
np, I went with the is_err approach for this PR (c9eba3e) Though, the discussed issue remains there.
* Check if ClientStatePath is empty during client creation * Rewrite ClientState present check with is_ok
Closes: #604
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.