Apply expiration checks in PacketData classes #8186
Conversation
Signed-off-by: Matilda Clerke <[email protected]>
Signed-off-by: Matilda Clerke <[email protected]>
Signed-off-by: Matilda Clerke <[email protected]>
Signed-off-by: Matilda Clerke <[email protected]>
Signed-off-by: Matilda Clerke <[email protected]>
| private static void validateParameters(final long expiration, final Clock clock) { | ||
| checkArgument(expiration >= 0, "expiration cannot be negative"); | ||
| checkArgument( | ||
| expiration >= clock.instant().getEpochSecond(), "expiration cannot be in the past"); |
There was a problem hiding this comment.
Why do we need to plumb Clock UTC everywhere, is it a testing concern? AFAIK Instant.now().toEpochMilli() is always in UTC.
If it is a testing concern about wanting to use a static clock, couldn't we spy and mock a non-static validateParameters method, or move this as a default method into PacketData for example? and then just directly test the real method in isolation. It should be the same coverage without the need to add clock to constructors
There was a problem hiding this comment.
It is a testing concern, yes. In many tests, we're using old hard-coded packets encoded in RLP, so the only way to ensure they're considered "valid" is to compare them against an appropriate instant, now provided by the supplied clock. Ideally, dependency injection of the clock into a PacketDataFactory, would remove this from the method parameters we see here, but that's a refactoring out of scope for this PR.
I suppose we could use a spy to mock calls against a non-static validateParameters method, but truthfully, I'd rather refactor the code and maybe utilise dagger. It'll be much easier to maintain moving forward. Since you're the third person to mention it this morning, I'll branch off and work on the refactoring now.
PR description
Apply expiration checks in PacketData classes