Skip to content

refactor(pass-style,...): abstract symbol making to prepare for flip#2793

Merged
erights merged 3 commits intomasterfrom
markm-abstract-symbol-construction
May 24, 2025
Merged

refactor(pass-style,...): abstract symbol making to prepare for flip#2793
erights merged 3 commits intomasterfrom
markm-abstract-symbol-construction

Conversation

@erights
Copy link
Copy Markdown
Contributor

@erights erights commented May 5, 2025

Closes: #XXXX
Refs: #2792 #2777

Description

This PR is essentially a pure refactor to set things up for upcoming substantive PRs like #2792 . The main one will change the passable symbol representation, along the lines of #2777 but likely different. This is for symbols used for purposes passing, or test counterexamples to being passable. All other symbol uses should not change.

The change from this PR is

  • change calls to Symbol.for('foo') for making a passable symbol, to passableSymbolForName('foo')
  • change calls to Symbol('foo') for making a non-passable symbol for testing purposes, to unpassableSymbolForName('foo')

Security Considerations

Assuming we changed each symbol-making callsite from the old to the correct new one, and assuming that we did not alter the code that was making symbols for other reasons, none.

Scaling Considerations

Using registered symbols like Symbol.for('foo') is a semantically perfect binding to the pass-style and ocapn category 'symbol'. However its scaling properties are disastrous. In some js systems like v8, unused registered symbols are not garbage collected. This is why we're going to switch passable symbol representation. However this PR does none of that yet. It merely sets the stage. This PR should not cause any change in scaling.

Documentation Considerations

The need to pass symbols is rare and obscure, and most users will not need to be aware of this. However, any reference documentation should clearly explain when to use the new abstract symbol-creation functions, and when not to.

Testing Considerations

As a pure refactor, the main concern is that all the old tests still pass. Any substantive new tests or test changes will be downstream, in the PRs that this PR enables.

Compatibility Considerations

Assuming we changed each symbol-making callsite from the old to the correct new one, and assuming that we did not alter the code that was making symbols for other reasons, none.

Upgrade Considerations

As a pure refactor, none. Beyond that, when we change the JS representation of a passable symbol, we will not change the wire representation in any of our encodings. Thus, durable state saved using the representation will, at that time, revive into the new representation, so still none. Likewise with messages sent from code on one side of the upgrade and received by code on the other side of the upgrade.

@erights erights self-assigned this May 5, 2025
Comment on lines +96 to +100
* @returns {symbol}
*/
export const passableSymbolForName = name => {
if (typeof name !== 'string') {
return undefined;
}
typeof name === 'string' ||
Fail`${q(name)} must be a string, not ${q(typeof name)}`;
Copy link
Copy Markdown
Contributor Author

@erights erights May 5, 2025

Choose a reason for hiding this comment

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

This part is not a pure refactor. passableSymbolForName can no longer return undefined. It throws instead

| 'promise';

export type TaggedOrRemotable = 'tagged' | 'remotable';
export type PassStyleMarker = 'tagged' | 'remotable';
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This rename is a drive-by, anticipating the members of the enum to expand.

@erights erights changed the title refactor(pass-style,...): abstract symbol construction to prepare for… refactor(pass-style,...): abstract symbol making to prepare for flip May 5, 2025
@erights erights force-pushed the markm-abstract-symbol-construction branch from 3bbd381 to 9ee0a81 Compare May 5, 2025 22:41
@erights erights mentioned this pull request May 5, 2025
1 task
@erights erights force-pushed the markm-abstract-symbol-construction branch 2 times, most recently from 379f83a to bb72411 Compare May 5, 2025 23:59
@erights erights marked this pull request as ready for review May 5, 2025 23:59
@erights erights force-pushed the markm-abstract-symbol-construction branch from bb72411 to 3bc9bd6 Compare May 7, 2025 20:34
@erights erights force-pushed the markm-abstract-symbol-construction branch from 3bc9bd6 to 0e9b250 Compare May 7, 2025 23:47
@erights erights force-pushed the markm-abstract-symbol-construction branch from 0e9b250 to a8e91d0 Compare May 12, 2025 19:29
erights added a commit 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.
@erights erights force-pushed the markm-abstract-symbol-construction branch from a8e91d0 to 73effc9 Compare May 15, 2025 21:19
@erights erights force-pushed the markm-abstract-symbol-construction branch from 4e96633 to ae9d7af Compare May 17, 2025 01:38
@erights erights enabled auto-merge (squash) May 24, 2025 16:59
@erights erights merged commit d8ec3a0 into master May 24, 2025
16 checks passed
@erights erights deleted the markm-abstract-symbol-construction branch May 24, 2025 17:59
@Ebisaki

This comment was marked as spam.

Ebisaki

This comment was marked as spam.

@Ebisaki

This comment was marked as spam.

@Ebisaki

This comment was marked as spam.

@Ebisaki

This comment was marked as spam.

Ebisaki

This comment was marked as spam.

@Ebisaki

This comment was marked as spam.

@Ebisaki

This comment was marked as spam.

boneskull pushed a commit 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.
boneskull pushed a commit that referenced this pull request Jun 4, 2025
…2793)

Closes: #XXXX
Refs: #2792 #2777

## Description

This PR is essentially a pure refactor to set things up for upcoming
substantive PRs like #2792 . The main one will change the passable
symbol representation, along the lines of #2777 but likely different.
This is for symbols used for purposes passing, or test counterexamples
to being passable. All other symbol uses should not change.

The change from this PR is
- change calls to `Symbol.for('foo')` for making a passable symbol, to
`passableSymbolForName('foo')`
- change calls to `Symbol('foo')` for making a non-passable symbol for
testing purposes, to `unpassableSymbolForName('foo')`

### Security Considerations

Assuming we changed each symbol-making callsite from the old to the
correct new one, and assuming that we did not alter the code that was
making symbols for other reasons, none.

### Scaling Considerations

Using registered symbols like `Symbol.for('foo')` is a semantically
perfect binding to the pass-style and ocapn category `'symbol'`. However
its scaling properties are disastrous. In some js systems like v8,
unused registered symbols are not garbage collected. This is why we're
going to switch passable symbol representation. However this PR does
none of that yet. It merely sets the stage. This PR should not cause any
change in scaling.

### Documentation Considerations

The need to pass symbols is rare and obscure, and most users will not
need to be aware of this. However, any reference documentation should
clearly explain when to use the new abstract symbol-creation functions,
and when not to.

### Testing Considerations

As a pure refactor, the main concern is that all the old tests still
pass. Any substantive new tests or test changes will be downstream, in
the PRs that this PR enables.

### Compatibility Considerations

Assuming we changed each symbol-making callsite from the old to the
correct new one, and assuming that we did not alter the code that was
making symbols for other reasons, none.

### Upgrade Considerations

As a pure refactor, none. Beyond that, when we change the JS
representation of a passable symbol, we will not change the wire
representation in any of our encodings. Thus, durable state saved using
the representation will, at that time, revive into the new
representation, so still none. Likewise with messages sent from code on
one side of the upgrade and received by code on the other side of the
upgrade.
erights added a commit that referenced this pull request Jul 11, 2025
Extracted from #2797 

Closes: #XXXX
Refs: #2793 #2792

## Description

This PR is the extraction from #2797 of only the safe pure refactor
elements of #2797 as suggested by @kriskowal at
#2797 (review) .
#2797 is then rebased on this one, representing only the elements whose
refactor-purity was questionable.

This PR should be a pure refactor, to avoid creating remotables with
symbol-named methods when there is clearly no possible compat hazard.
Non-remotable objects with symbol-named methods are fine. But see
Compatibility Considerations below.

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

none
### Testing Considerations

Most of the remotable objects with symbol-named methods were in tests.
These can clearly be fixed without any compat hazard, so I count these
as pure refactors. However, in removing the symbol-ness of some of these
tests, I may have accidentally removed the purposes of the tests. They
might now be fully redundant with other tests and should be removed. If
anyone does investigate this, that cleanup could be in a later PR.

### Compatibility Considerations

none. But see Compatibility Considerations of #2797 

### Upgrade Considerations

none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants