Don't restore from storage after reboot #50
Conversation
| @@ -26,8 +26,20 @@ class TimeStorageTests: XCTestCase { | |||
| var storage = TimeStorage(storagePolicy: .standard) | |||
| let sampleFreeze = TimeFreeze(offset: 5000.32423) | |||
| storage.stableTime = sampleFreeze | |||
|
|
|||
There was a problem hiding this comment.
Yea; letting it breathe a bit ;)
| @@ -10,7 +10,7 @@ struct TimeFreeze { | |||
| private let offset: TimeInterval | |||
|
|
|||
| /// The stable timestamp adjusted by the most acurrate offset known so far. | |||
| var adjustedTimestamp: TimeInterval? { | |||
| var adjustedTimestamp: TimeInterval { | |||
There was a problem hiding this comment.
surprised this doesn't cause build failures, shouldn't making this non-optional also require us to remove some optional chaining somewhere?
There was a problem hiding this comment.
It's just a coincidence this didn't break anything. Is because we are using it on an optional chaining such as self.whatever?.adjustedTimestamp that as a whole returns an optional regardless
| func testRetrievingTimeFreezeAfterReboot() { | ||
| let sampleFreeze = TimeFreeze(offset: 5000.32423) | ||
| var storedData = sampleFreeze.toDictionary() | ||
| storedData["Uptime"] = storedData["Uptime"]! + 10 |
There was a problem hiding this comment.
+1 for liberally using ! in tests
|
What are your thoughts on making this a configurable option? Aka have a way to configure Kronos to either return nil or try best-effort in the case where we restore from storage after reboot. |
|
I'm indifferent to whether you merge this or not. However, I'd just like to explain why we did this in the first place. The system clock cannot be trusted because the user might be doing all sorts of weird things, like setting it to the wrong time zone, or purposely setting it nine minutes forward to keep themselves on time. However, the assumption that this is making is that the weird offset of the system clock is going to be the same between reboots. If that is the case, then the existing code guesses right. The phone that's always nine minutes fast will work for this code. Obviously the tradeoff is that we could be wrong, and I just wanted to give the reasoning from last year. |
|
I'll add that I'm also indifferent, although I think we should at least consider whether it makes sense to make this a configurable option in Kronos. |
|
@craffert0 Thanks for the context; that's what I assumed. I think as a library, the contract of giving a trustworthy clock or nothing is very powerful and one I wish we could keep. Hence why I think this change will be beneficial on the accuracy side. Now I understand that most of the times not returning nil will still return an accurate estimation but my stance is that most might not be enough |
|
FYI Leo is ok with this change on Android as well so we can go forward. I still think making this configurable on Kronos is the best answer in terms of this PR, but am also OK with landing this as-is. |
If the device clock changes after reboot before Kronos is updated, the "trusted" time will be wrong since we'll apply the offset to the wrong ground time. This breaks the "trusted" guarantee.
If the device clock changes after reboot before Kronos is updated, the "trusted" time will be wrong since we'll apply the offset to the wrong ground time. This breaks the "trusted" guarantee.