Followup on 149 and 151 #155
Conversation
- don't use refs for time and duration - simplify is_within_trust_period according to suggestion in: #149 (comment) - simply Kind::DurationOutOfRange as we don't need to wrap a SystemTimeError anymore
tendermint/src/lite/verifier.rs
Outdated
| let header_time: SystemTime = last_header.bft_time().into(); | ||
| let expires_at = header_time.add(trusting_period); | ||
| // Ensure now > expires_at. | ||
| // TODO: shouldn't this be <= instead below: |
There was a problem hiding this comment.
See discussion: #149 (comment) cc @ebuchman
There was a problem hiding this comment.
The spec mentions return header.Time + trustedPeriod > now (for the non-error'ing case) and the go code also behaves accordingly. So this should indeed be <=!
There was a problem hiding this comment.
Changed accordingly in 301958f
Codecov Report
@@ Coverage Diff @@
## master #155 +/- ##
==========================================
- Coverage 36.74% 35.38% -1.37%
==========================================
Files 91 91
Lines 3233 3188 -45
Branches 500 505 +5
==========================================
- Hits 1188 1128 -60
- Misses 1775 1776 +1
- Partials 270 284 +14
Continue to review full report at Codecov.
|
|
I don't understand why this seems to decrease test-coverage? |
| trusting_period: &Duration, | ||
| now: &SystemTime, | ||
| trusting_period: Duration, | ||
| now: SystemTime, |
There was a problem hiding this comment.
Both SytemTime and Duration implement the Copy trait. Hence, these references where not needed.
| #[error("duration error {:?}", _0)] | ||
| DurationOutOfRange(#[from] SystemTimeError), | ||
| #[error("trusted header time is too far in the future")] | ||
| DurationOutOfRange, |
There was a problem hiding this comment.
Maybe we should rename this error? Note this is different from the NonIncreasingTime one introduced in #142, namely:
https://github.com/interchainio/tendermint-rs/blob/0b75eee8c88c4c012670c9e526742ae66ce5598c/tendermint/src/lite/error.rs#L27-L29
(or the Expired error)
See:
https://github.com/interchainio/tendermint-rs/blob/a6f7c64e8214d5fbf32adee5ee3b6cbac09ee9da/tendermint/src/lite/verifier.rs#L35-L49
|
Looks good to me! 👍 |
|
Thanks for the review @Shivani912 👍 |
closes #154