Skip to content

refactor: improved debugging messages for durability#5401

Merged
mergify[bot] merged 5 commits intomasterfrom
decodeToJustin
May 21, 2022
Merged

refactor: improved debugging messages for durability#5401
mergify[bot] merged 5 commits intomasterfrom
decodeToJustin

Conversation

@Chris-Hibbert
Copy link
Copy Markdown
Contributor

@Chris-Hibbert Chris-Hibbert commented May 19, 2022

Description

While working on durability (#5283), we (@erights and I) improved some error message so they explicitly told us where our problem was.

Our first version threw undefined must be a string from inside decodeToJustin, even when there was not problem in the serialization. We eventually traced that to decodeToJustin not understanding the current representation of ibids. This version sidesteps that issue when there's no durability problem.

      if (durable) {
        serializedValue.slots.forEach((vref, slotIndex) =>
          assert(
            vrm.isDurable(vref),
            X`value is not durable: ${value} at slot ${slotIndex} of ${decodeToJustin(
              {
                body: JSON.parse(serializedValue.body),
                slots: serializedValue.slots,
              },
              true,
            )}`,
          ),
        );
      }

Security Considerations

Reveals some internal information, but should only appear when debugging.

Documentation Considerations

No user-serviceable parts inside.

Testing Considerations

This is useful during tests.

@Chris-Hibbert Chris-Hibbert added marshal package: marshal cosmic-swingset package: cosmic-swingset devex developer experience labels May 19, 2022
@Chris-Hibbert Chris-Hibbert requested review from FUDCo and erights May 19, 2022 23:09
@Chris-Hibbert Chris-Hibbert self-assigned this May 19, 2022
Copy link
Copy Markdown
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.

The code LGTM, thanks!

The PR comment reads like the embedded snippet may be the replacement code rather than the code being replaced.

Copy link
Copy Markdown
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.

Doing a request changes to block submission until I make a suggestion. Coming soon...

Copy link
Copy Markdown
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.

LGTM

Copy link
Copy Markdown
Collaborator

@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.

Seems legit.

@erights erights added the automerge:squash Automatically squash merge label May 21, 2022
@mergify mergify bot merged commit 2145163 into master May 21, 2022
@mergify mergify bot deleted the decodeToJustin branch May 21, 2022 03:58
erights added a commit to endojs/endo that referenced this pull request May 15, 2025
Closes: #XXXX
Refs: Agoric/agoric-sdk#5401

## Description

We often need to see the deep contents of a Passable. Indeed, this was
kicked off by a slack thread about wanting to quote
```js
M.splitRecord(
    harden({
      assetKind: M.or('nat', 'set', 'copySet', 'copyBag'),
      decimalPlaces: M.and(M.gte(-100), M.lte(100)),
    }),
  )
```

This PR provides a `qp` function meaning "quote passable". You can use
it where you'd use `q`. It produces a quasi-quoted Justin expression of
the form in which the passable would be passed. For example, the `qp` of
the above pattern is

```js
`makeTagged("match:splitRecord",  [
  {
    assetKind: makeTagged("match:or", [
      "nat",
      "set",
      "copySet",
      "copyBag",
    ]),
    decimalPlaces: makeTagged("match:and", [
      makeTagged("match:gte", -100),
      makeTagged("match:lte", 100),
    ]),
  },
])`
```

### Security Considerations

Unlike `q`, `qp` does not lazily postpone rendering the expression into
Justin. `q` can do this easily because it is inside the `'ses'`
assert.js module and can use the rights amplification available only
there. `qp` depends on `'@endo/marshal'` which is several levels up from
`'ses'`. Rather than expose these rights amplification powers (or
several other kludges that failed), I just made `qp` non-lazy. Thus
there are no security implications.

### Scaling Considerations

Because `qp` is not lazy, simply constructing an error with `Fail` or
`makeError(X...)` will pay all the cost of rendering to Justin, whether
or not that error is ever used.

### Documentation Considerations

Because Justin is a subset of HardenedJS, the Justin expressions
produced by `qp` can be used in documentation if needed. Though see
Compatibility Considerations below.

### Testing Considerations

- [ ] Only minimal tests written for `qp`. We should also write a test
that loops through all the `jsonJustinPairs`, just as other Justin tests
do.

### Compatibility Considerations

Using Justin with symbols exposes in the rendered string how Justin
currently represents passable symbols. That changes in #2793. Whichever
of these gets merged first, the other will have to reconcile merge
conflicts. And any saved Justin strings containing that old Justin
representation will need to be updated or marked stale.

### Upgrade Considerations

Other than the Compatibility Considerations above, none.

- [x] Update `NEWS.md` for user-facing changes.
boneskull pushed a commit to endojs/endo that referenced this pull request Jun 4, 2025
Closes: #XXXX
Refs: Agoric/agoric-sdk#5401

## Description

We often need to see the deep contents of a Passable. Indeed, this was
kicked off by a slack thread about wanting to quote
```js
M.splitRecord(
    harden({
      assetKind: M.or('nat', 'set', 'copySet', 'copyBag'),
      decimalPlaces: M.and(M.gte(-100), M.lte(100)),
    }),
  )
```

This PR provides a `qp` function meaning "quote passable". You can use
it where you'd use `q`. It produces a quasi-quoted Justin expression of
the form in which the passable would be passed. For example, the `qp` of
the above pattern is

```js
`makeTagged("match:splitRecord",  [
  {
    assetKind: makeTagged("match:or", [
      "nat",
      "set",
      "copySet",
      "copyBag",
    ]),
    decimalPlaces: makeTagged("match:and", [
      makeTagged("match:gte", -100),
      makeTagged("match:lte", 100),
    ]),
  },
])`
```

### Security Considerations

Unlike `q`, `qp` does not lazily postpone rendering the expression into
Justin. `q` can do this easily because it is inside the `'ses'`
assert.js module and can use the rights amplification available only
there. `qp` depends on `'@endo/marshal'` which is several levels up from
`'ses'`. Rather than expose these rights amplification powers (or
several other kludges that failed), I just made `qp` non-lazy. Thus
there are no security implications.

### Scaling Considerations

Because `qp` is not lazy, simply constructing an error with `Fail` or
`makeError(X...)` will pay all the cost of rendering to Justin, whether
or not that error is ever used.

### Documentation Considerations

Because Justin is a subset of HardenedJS, the Justin expressions
produced by `qp` can be used in documentation if needed. Though see
Compatibility Considerations below.

### Testing Considerations

- [ ] Only minimal tests written for `qp`. We should also write a test
that loops through all the `jsonJustinPairs`, just as other Justin tests
do.

### Compatibility Considerations

Using Justin with symbols exposes in the rendered string how Justin
currently represents passable symbols. That changes in #2793. Whichever
of these gets merged first, the other will have to reconcile merge
conflicts. And any saved Justin strings containing that old Justin
representation will need to be updated or marked stale.

### Upgrade Considerations

Other than the Compatibility Considerations above, none.

- [x] Update `NEWS.md` for user-facing changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge:squash Automatically squash merge cosmic-swingset package: cosmic-swingset devex developer experience marshal package: marshal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants