change TimerService to use branded Timestamps

[warner]

What is the Problem Being Solved?

Now that #5668 is complete and TimerBrand is available, one follow-on step is to include the brand in Timestamp and RelativeTime objects.

On the vat-timer side, we this change is pretty tiny: there are already toTimestamp() and fromTimestamp() functions in the right places, which are ready to be edited to insert/check+strip the brands as they cross the vat-timer boundary. I didn't think that far ahead on test-vat-timer.js but the changes are pretty mechanical.

The deeper task is all the existing code that deals with Timestamps. In general, all normal code would be getting Timestamps from the timer service in the first place, so they shouldn't be synthesizing Timestamps de novo (and would thus need to be changed to include a brand). But there are lots of unit tests which build them manually, and those would need to be changed.

Before starting this, I think I'd like to move all the timeMath.js code out of swingset and into its own package, tentatively named @agoric/timestamp.

cc @erights

Description of the Design

Change vat-timer.js toTimestamp to insert the brand, and fromTimestamp to assert the incoming brand matches before stripping it into a TimerValue. Change unit tests to match.

Security Considerations

none I can think of, this should be strictly more restrictive than before

Test Plan

[warner]

I'm trying to work out a plan for the ongoing transition to fully-branded time.

I see that Zoe and a bunch of contracts take deadline and poll_interval and other time-related things: some of those calls are defined to take a Timestamp, some are defined to take bigint. They do a lot of bigint comparison of time values (if (now > deadline) { doStuff }). And they don't know about TimerBrand.

Our desired end state is:

• the TimerService (e.g. vat-timer) requires branded Timestamp/RelativeTime values in all APIs
• zoe/etc take Timestamp for things like deadline, and RelativeTime for things like poll_interval, and these are all branded, so it's just an error to provide a time-related value that doesn't match the brand which Zoe is using for the TimerService
• @agoric/time defines Timestamp to be a branded object
• the TimestampRecord and RelativeTimeRecord identifiers do not appear anywhere

Of course, these types cross a lot of interface boundaries, so it's a big task to change everything in a single PR. I think the following incremental plan might work:

• 1: change TimerService API and implementation to require TimestampRecord (which is branded)
  • the Timestamp type continues to be the union of TimestampRecord (branded) and TimestampValue (unbranded, just a bigint)
  • the other TimeMath APIs still accept Timestamp, and the two-argument methods continue to do heroic things to tolerate the four input combinations
  • with this change, zoe/etc will fail typechecks, because they're passing mere TimestampValue to an API that requires TimestampRecord
  • they will also fail runtime checks, because vat-timer requires a brand
  • (don't stop here)
• 2: change the TimeMath APIs to require a Record
  • e.g. TimeMath.addAbsRel takes a TimestampRecord and a RelativeTimeRecord, and returns a TimestampRecord
  • removes a lot of tolerance heroics from TimeMath implementation
  • tolerance remains in Timestamp/RelativeTime types, and toAbs/toRel
  • toAbs() takes anything (plus a target brand) and returns a Record
  • (don't stop here)
• 3: change zoe/etc internals and TimerService-facing calls to use branded time
  • use toAbs/toRel to coerce/convert args from clients (e.g. deadline) to branded Records
    • (they will need a E(timerService).getTimerBrand() to get the brand object for their toAbs() calls)
  • change all time comparisons from > to e.g. TimeMath.compareAbs
  • zoe/etc internal storage will use TimestampRecord/RelativeTimeRecord
  • zoe/etc APIs continue to accept either bigint or Timestamp (union type)
    • QUESTION: where does zoe/etc return a time value? should it downcast to bigint, or return its branded TimestampRecords as a Timestamp?
    • where the zoe/etc API claims to return a Timestamp, let's return the TimestampRecord and see if anything breaks (i.e. was lying about accepting a Timestamp / unprepared for the TimestampRecord side of that union)
  • at this point, zoe typechecks should pass, and implementation should work
• 4: (maybe pause here)
• 5: define Timestamp to be only the Record type
  • make this one/two-line change temporarily, to get a list of failing typechecks that need updating
  • for zoe/etc APIs that accept Timestamp, clients who were relying on the bigint subset will now fail typechecks
    • this is the main signal that those clients need to be updated
  • zoe/etc APIs that explicitly take bigint will continue to work, but they ought to be changed too
    • the zoe/etc internal toAbs coercion could go away. This might simplify the internals (no need to fetch a brand). It might delay some errors (vat-timer would be the one noticing a mismatched brand, not zoe/etc's coercion point).
    • OTOH toAbs(input, brand) is a useful assertion
    • maybe just add TimeMath.assertBrand(input, brand), as a non-coercing runtime equality check
• 6: commit to Timestamp=Record
  • zoe/etc internals can be changed to use Timestamp
  • TimeMath APIs change to use Timestamp
  • TimerService APIs change to use Timestamp
  • delete TimestampValue
• 7: wait until all internals/APIs are using Timestamp instead of TimestampRecord
• 8: finally: collapse types
  • only changes @agoric/time/src/types.d.ts
  • delete Timestamp from that file, then rename TimestampRecord to Timestamp, same for relative times

I don't want to hang out in step 4 forever, but I also learned that there's a lot of code that thinks it knows what Time is, and I got stuck trying to push past that point in a single PR. So I'm going to make a PR that has a commit for 1, then 3, then 3, and stop there.

We'll have TimestampRecord scattered throughout the zoe/etc code, which is what I will regret. But they will correctly indicate that such code is doing the new thing, and the zoe/etc clients that are meeting a Timestamp interface will be correctly indicating that they might do the new thing and might do the old thing, and they're still allowed to do either until we try out step 5 and see which ones fail their typechecks because they're passing around a bigint.

(one note: code which thinks it can compare timestamps with > will fail in confusing ways, I don't know how objects compare but certainly not usefully. It would be nice if there were a type or lint rule that could detect these bare comparisons, so the step 5 fixes can include TimeMath.compareAbs() updates)

[warner]

I've got a branch (6003-branded-timestamps) that makes a start at the first step of a different path, where we change the return values of TimerService to provide branded Record types, but we leave the inputs tolerant of both branded TimestampRecord/RelativeTimeRecord and unbranded plain bigints. This still requires changes to a lot of unit tests (things like t.is(timestamp, 500n)), and zoe/contract code (things like if (timestamp < deadline) that needs to become if (TimeMath.compareAbs(timestamp, deadline) === -1)).

The branch does not yet pass all tests, but it's a start, and I think the remaining work should be a matter of adapting the changes so far to the remaining failing tests.

[warner]

PR #6904 just landed, which changes the TimerService (vat-timer) to exclusively emit/return branded TimestampRecords, and updates zoe/etc to handle these. This addresses the first half of the task, and is enough to allow contracts to be written against our preferred API (almost: their types will say TimestampRecord or RelativeTimeRecord, so they get the correct internal typechecking, but in the long run we want to get rid of the alternative TimestampValue, and rename TimestampRecord to just plain Timestamp, so there will be a second pass, after we get rid of the old API, where TimestampRecord is an alias for Timestamp, and Timestamp is branded, and then clients must replace their use of TimestampRecord with plain Timestamp, then we can remove the old precise name).

We aren't going to try to make the second change (vat-timer only accepts branded time) in the Vaults release: we'll gradually change zoe and contracts to send branded time first, then we'll change vat-timer.

So this ticket does not close yet, but can be moved out of the vaults release. We've done what we can for now.