Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove crankNumber from transcript entries #2429

Merged
merged 3 commits into from
Feb 15, 2021
Merged

Conversation

warner
Copy link
Member

@warner warner commented Feb 15, 2021

The VatManager is responsible for maintaining a transcript of all deliveries
to their vat worker, so the vat can be reloaded later. These transcript
entries have been including the crankNumber: a global counter indicating
how many cranks have been delivered (to all vats, not just the one for which
the transcript entry is being created).

This removes that crankNumber from the transcript:

  • each vat should be independent, this global crankNumber is revealing
    information about what happens in other vats
  • Add Counter abstraction to Swingset #2400 is changing the type of crankNumber to a BigInt, which cannot be
    serialized by the simple JSON.stringify used in vatKeeper.js

It also removes the now-unnecessary kernelKeeper argument from
makeTranscriptManager, and changes one test that happened to depend upon
the presence of the crankNumber field.

closes #2428

cc @katelynsills

The VatManager is responsible for maintaining a transcript of all deliveries
to their vat worker, so the vat can be reloaded later. These transcript
entries have been including the `crankNumber`: a global counter indicating
how many cranks have been delivered (to all vats, not just the one for which
the transcript entry is being created).

This removes that crankNumber from the transcript:

* each vat should be independent, this global crankNumber is revealing
information about what happens in other vats
* #2400 is changing the type of `crankNumber` to a BigInt, which cannot be
serialized by the simple `JSON.stringify` used in vatKeeper.js

It also removes the now-unnecessary `kernelKeeper` argument from
`makeTranscriptManager`, and changes one test that happened to depend upon
the presence of the crankNumber field.

closes #2428
@warner warner added the SwingSet package: SwingSet label Feb 15, 2021
@warner warner requested a review from FUDCo February 15, 2021 20:20
@erights erights self-requested a review February 15, 2021 20:21
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this LGTM. But you should probably still wait for @FUDCo 's review.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that slog more properly subsumes the role that motivated putting crank number in the transcript in the first place, I'm fine with this.

@warner warner enabled auto-merge (squash) February 15, 2021 21:07
@warner warner merged commit d7886c0 into master Feb 15, 2021
@warner warner deleted the 2428-remove-cranknumber branch February 15, 2021 21:35
warner added a commit that referenced this pull request Feb 16, 2021
because #2429 changed transcripts stop including `crankNumber`, which is a
Nat (and therefore now a BigInt).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove crankNumber from transcript
3 participants