TimerService emits branded TimeStampRecord/RelativeTimeRecord #6904

warner · 2023-02-01T11:06:40Z

Change SwingSet vat-timer (aka TimerService) to only emit branded TimestampRecord / RelativeTimeRecord. The service continues to accept non-branded values (BigInts), but the payloads of handler callbacks and getCurrentTimestamp() will be a record with a .timerBrand property.

Change zoe/inter-protocol/etc to accomodate this. Most contracts still use plain BigInts internally.

We will finish the transition later, by changing the TimerService to only accept branded records, and reject plain BigInts.

refs #6003

packages/zoe/src/cleanProposal.js

Chris-Hibbert · 2023-02-01T17:46:09Z

packages/zoe/src/contractSupport/priceAuthority.js

@@ -301,7 +301,7 @@ export function makeOnewayPriceAuthorityKit(opts) {
      return quote;
    },
    async quoteAtTime(deadline, amountIn, brandOut) {
-      assert.typeof(deadline, 'bigint');
+      assert.typeof(deadline, 'object'); // Timestamp


fit(deadline, TIMESTAMP_SHAPE);

@Chris-Hibbert I've got a patch for this:

async quoteAtTime(deadline, amountIn, brandOut) { + mustMatch(deadline, TimestampShape); AmountMath.coerce(actualBrandIn, amountIn); assertBrands(amountIn.brand, brandOut);

but I'm worried that it will impose a change on the clients of Zoe that they may not be prepared to handle yet. We decided to reduce the goals of this PR: have the timerService return branded timestamps, but accept either. So a lot of the changes are to ensure that when Zoe receives a timestamp, it can deal with it, but we want to keep everything tolerant of code that' still sending plain BigInts into time stuff, including zoe. If we can get that into the Vaults release, then contracts can begin to write code that matches our long-term goals (always use brands), because both "send bigints, accept brands" and "send brands, accept brands" will work.

Do you think I should apply this mustMatch change?

For this one, we decided to use deadline = coerceToTimestampRecord(deadline, timerBrand), which will accept any of number | bigint | TimestampRecord (with correct brand), but reject TimestampRecords with the wrong brand.

This will succeed or fail in exactly the same situations as the current PR's lack of any assertions, but the failure will happen as the first line of quoteAtTime, instead of happening inside the timer service a moment later.

Actually, this is turning out to be awkward to implement. The enclosing function (makeOnewayPriceAuthorityKit) is not async, which means I can't do timerBrand = await E(timerService).getTimerBrand() outside of each call to quoteAtTime call (which would add some overhead that I'd like to avoid). The next earliest place to fetch the brand is in the four callers to makeOnewayPriceAuthorityKit():

zoe/src/contracts/priceAggregator.js

inter-protocol/src/price/fluxAggregatorKit.js

zoe/tools/manualPriceAuthority.js

zoe/tools/scriptedPriceAuthority.js

The first two are async, and not a problem. The zoe/tools/ ones are sync, but it probably also use a local (manual)TimerService, so in theory I can just do timerBrand = timerService.getTimerBrand(), but the typechecker complains because it thinks that timerService is an ERef.

I think I'm not going to fix this one, and rely upon TimerService complaining about a mismatched brand, instead of trying to catch it slightly earlier.

Since the two sync calls are from test tools, it would be fine to // @ts-expect-error them.

packages/zoe/src/contracts/auction/index.js

packages/zoe/src/contracts/callSpread/fundedCallSpread.js

packages/zoe/src/contracts/priceAggregator.js

gibson042 · 2023-04-27T21:10:39Z

@warner I've rebased and refreshed this PR, and think I've fixed the zoe tests but not yet the inter-protocol ones. Can you check that it's on the right track?

packages/zoe/src/contracts/priceAggregator.js

packages/zoe/test/unitTests/contracts/test-priceAggregator.js

warner · 2023-04-28T09:29:17Z

packages/zoe/test/unitTests/contracts/test-priceAggregator.js

+      amountIn: { brand: remotable('$ATOM brand'), value: 1n },
+      amountOut: { brand: remotable('$USD brand'), value: 1020n },
+      timer: remotable('ManualTimer'),
+      timestamp: { timerBrand: remotable('timerBrand'), absValue: 1n },


this should probably use the coerce or toAbs -type helper, so we can keep the property names (timerBrand and absValue) semi-private to the implementation

This makes sense, but not in a time crunch IMO (especially since there are other similar flavors). How would you feel about deferral?

gibson042 · 2023-04-28T21:41:20Z

@warner @Chris-Hibbert I just pushed changes that should resolve everything outside of inter-protocol, but I'm going to be away all weekend and don't know if I'll have a chance to address that package before Monday. I'll try to do more tonight, but please feel free to take the baton.

packages/SwingSet/docs/timer.md

packages/SwingSet/src/vats/timer/vat-timer.js

warner · 2023-04-30T23:28:18Z

I pushed a couple of changes to address those issues, and to fix the inter-protocol tests. We'll see how CI feels about them.

gibson042

CI is green!

packages/inter-protocol/test/price/snapshots/test-fluxAggregatorKit.js.md

warner · 2023-05-02T19:34:51Z

Ok @erights said to go ahead without his review. I'll rebase and squash now.

The service still accepts both branded and unbranded (i.e. bigint) TimestampValue and RelativeTimeValue, but all emitted values (`getCurrentTimestamp`, `handler.wake()` callbacks, Promise resolution values, Notifier updates) will be branded records. Client code must be prepared to accept these, in particular it must use TimeMath.compareAbs() to compare two timestamps, since they are records (objects), and using `>` will give erroneous results. Client authors should begin the transition process of submitting branded TimestampRecord and RelativeTimeRecord to the TimerService. A future upgrade will stop accepting non-branded values. Also the `TimeMath.toAbs`/`toRel` functions were replaced with `coerceTimestampRecord`/`coerceRelativeTimeRecord`, and accept any of Number, Bigint, or a correctly-branded record. They throw if given a branded record that does not match the target brand in the second argument. This should be used when converting a bare number (supplied by some external code which thinks it understands time) to a record suitable for submitting to the future (strict) TimerService. refs #6003 Co-authored-by: Richard Gibson <[email protected]>

The service still accepts both branded and unbranded (i.e. bigint) TimestampValue and RelativeTimeValue, but all emitted values (`getCurrentTimestamp`, `handler.wake()` callbacks, Promise resolution values, Notifier updates) will be branded records. The manual-timer.js mock accepts bigints on the control surface, for convenience, but returns branded TimestampRecord to API callers, just like the real vat-timer. Some additional conveniences were added. refs #6003 Co-authored-by: Richard Gibson <[email protected]>

Also update some testing tools like manualTimer.js and the fake price authority Co-authored-by: Richard Gibson <[email protected]>

Co-authored-by: Richard Gibson <[email protected]>

Chris-Hibbert reviewed Feb 1, 2023

View reviewed changes

warner force-pushed the 6003-branded-timestamps branch 2 times, most recently from b7c6492 to f69d791 Compare February 7, 2023 06:08

warner force-pushed the 6003-branded-timestamps branch from f69d791 to 0ba5adc Compare February 15, 2023 22:06

gibson042 force-pushed the 6003-branded-timestamps branch from 0ba5adc to a4274dd Compare April 27, 2023 21:08

Chris-Hibbert reviewed Apr 27, 2023

View reviewed changes

packages/zoe/src/contracts/priceAggregator.js Outdated Show resolved Hide resolved

warner commented Apr 28, 2023

View reviewed changes

packages/zoe/test/unitTests/contracts/test-priceAggregator.js Show resolved Hide resolved

warner commented Apr 28, 2023

View reviewed changes

warner commented Apr 29, 2023

View reviewed changes

packages/SwingSet/docs/timer.md Outdated Show resolved Hide resolved

warner commented Apr 29, 2023

View reviewed changes

packages/SwingSet/src/vats/timer/vat-timer.js Show resolved Hide resolved

warner force-pushed the 6003-branded-timestamps branch 2 times, most recently from 1f94a77 to a586df9 Compare May 1, 2023 01:18

warner marked this pull request as ready for review May 1, 2023 01:18

warner requested review from Chris-Hibbert and gibson042 May 1, 2023 01:21

warner changed the title ~~6003 branded timestamps~~ TimerService emits branded TimeStampRecord/RelativeTimeRecord May 1, 2023

gibson042 approved these changes May 1, 2023

View reviewed changes

warner commented May 1, 2023

View reviewed changes

packages/inter-protocol/test/price/snapshots/test-fluxAggregatorKit.js.md Show resolved Hide resolved

warner requested a review from erights May 1, 2023 19:00

Chris-Hibbert approved these changes May 1, 2023

View reviewed changes

warner force-pushed the 6003-branded-timestamps branch from e992f54 to d6989a4 Compare May 2, 2023 19:39

fix: TimerBrand has isMyTimerService(), not isMyTimer()

9f4e867

warner force-pushed the 6003-branded-timestamps branch from fcc8e42 to ead3251 Compare May 2, 2023 19:57

warner and others added 3 commits May 2, 2023 13:26

fix(zoe): handle branded TimestampRecord

2cebcf0

Also update some testing tools like manualTimer.js and the fake price authority Co-authored-by: Richard Gibson <[email protected]>

warner and others added 2 commits May 2, 2023 13:27

fix(inter-protocol): handle branded TimestampRecord

a350418

Co-authored-by: Richard Gibson <[email protected]>

fix: handle branded TimestampRecord in solo/store/agoric-cli/governance

8369dd6

Co-authored-by: Richard Gibson <[email protected]>

warner force-pushed the 6003-branded-timestamps branch from ead3251 to 8369dd6 Compare May 2, 2023 20:28

warner self-assigned this May 2, 2023

warner added the automerge:rebase Automatically rebase updates, then merge label May 2, 2023

mergify bot merged commit 2354e8d into master May 2, 2023

mergify bot deleted the 6003-branded-timestamps branch May 2, 2023 21:23

warner mentioned this pull request May 2, 2023

change TimerService to use branded Timestamps #6003

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TimerService emits branded TimeStampRecord/RelativeTimeRecord #6904

TimerService emits branded TimeStampRecord/RelativeTimeRecord #6904

warner commented Feb 1, 2023 •

edited

Loading

Chris-Hibbert Feb 1, 2023

warner May 1, 2023

warner May 1, 2023

warner May 1, 2023

Chris-Hibbert May 1, 2023

gibson042 commented Apr 27, 2023

warner Apr 28, 2023

gibson042 Apr 28, 2023

gibson042 commented Apr 28, 2023

warner commented Apr 30, 2023

gibson042 left a comment

warner commented May 2, 2023

TimerService emits branded TimeStampRecord/RelativeTimeRecord #6904

TimerService emits branded TimeStampRecord/RelativeTimeRecord #6904

Conversation

warner commented Feb 1, 2023 • edited Loading

Chris-Hibbert Feb 1, 2023

Choose a reason for hiding this comment

warner May 1, 2023

Choose a reason for hiding this comment

warner May 1, 2023

Choose a reason for hiding this comment

warner May 1, 2023

Choose a reason for hiding this comment

Chris-Hibbert May 1, 2023

Choose a reason for hiding this comment

gibson042 commented Apr 27, 2023

warner Apr 28, 2023

Choose a reason for hiding this comment

gibson042 Apr 28, 2023

Choose a reason for hiding this comment

gibson042 commented Apr 28, 2023

warner commented Apr 30, 2023

gibson042 left a comment

Choose a reason for hiding this comment

warner commented May 2, 2023

warner commented Feb 1, 2023 •

edited

Loading