Msg processors for ICS03 Ack and Confirm messages #228
Conversation
Codecov Report
@@ Coverage Diff @@
## master #228 +/- ##
=========================================
+ Coverage 13.6% 40.8% +27.1%
=========================================
Files 69 81 +12
Lines 3752 5778 +2026
Branches 1374 1986 +612
=========================================
+ Hits 513 2359 +1846
- Misses 2618 3227 +609
+ Partials 621 192 -429
Continue to review full report at Codecov.
|
| if claimed_height > ctx.chain_current_height() { | ||
| return Err(Kind::InvalidConsensusHeight | ||
| .context(claimed_height.to_string()) | ||
| .into()); | ||
| } | ||
|
|
||
| // Fail if the consensus height is too old (outside of trusting period). | ||
| if claimed_height.value() | ||
| // Fail if the consensus height is too advanced. | ||
| Err(Kind::InvalidConsensusHeight(claimed_height).into()) | ||
| } else if claimed_height.value() | ||
| < (ctx.chain_current_height().value() - ctx.chain_consensus_states_history_size() as u64) | ||
| { | ||
| return Err(Kind::StaleConsensusHeight | ||
| .context(claimed_height.to_string()) | ||
| .into()); | ||
| // Fail if the consensus height is too old (outside of trusting period). | ||
| Err(Kind::StaleConsensusHeight(claimed_height).into()) | ||
| } else { | ||
| // Height check is within normal bounds, check passes. | ||
| Ok(()) |
There was a problem hiding this comment.
Curious on the Rust idiomatic way for this. In other languages this is frowned upon:
if <expr1> {
return <err1>
} else if <expr2> {
return <err2>
...
} else {
return <err99>
}
And instead:
if <expr1> {
return <err1>
}
if <expr2> {
return <err2>
}
...
return <err99>
I also prefer the latter.
There was a problem hiding this comment.
I would say that in Rust the former is more idiomatic since in Rust if statements are expressions of their own, but both are fine by me.
There was a problem hiding this comment.
https://doc.rust-lang.org/rust-by-example/flow_control/if_else.html but I have no preference.
There was a problem hiding this comment.
But if this were happening in the middle of the function (for eg. an early return with explicit return statements, then yeah I would also prefer the latter.
There was a problem hiding this comment.
https://doc.rust-lang.org/rust-by-example/flow_control/if_else.html but I have no preference.
yes but there is no return in the if arms
There was a problem hiding this comment.
I am asking because it is very typical that we start a function body with a number of checks where we return errors if they fail. If we have a lot of checks we end up with a long if-then-elseif-... and I find that harder to read.
There was a problem hiding this comment.
More importantly though I would like to have consistency in our code base. So I will follow whatever we decide :)
There was a problem hiding this comment.
Yeah for such checks I agree that the version with return is better.
Closes: #223
ready for review!
For contributor use:
docs/) and code commentsFiles changedin the Github PR explorer