refactor(pass-style,...): abstract symbol making to prepare for flip#2793
Merged
refactor(pass-style,...): abstract symbol making to prepare for flip#2793
Conversation
erights
commented
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)}`; |
Contributor
Author
There was a problem hiding this comment.
This part is not a pure refactor. passableSymbolForName can no longer return undefined. It throws instead
erights
commented
May 5, 2025
| | 'promise'; | ||
|
|
||
| export type TaggedOrRemotable = 'tagged' | 'remotable'; | ||
| export type PassStyleMarker = 'tagged' | 'remotable'; |
Contributor
Author
There was a problem hiding this comment.
This rename is a drive-by, anticipating the members of the enum to expand.
3bbd381 to
9ee0a81
Compare
379f83a to
bb72411
Compare
bb72411 to
3bc9bd6
Compare
1 task
3bc9bd6 to
0e9b250
Compare
2 tasks
0e9b250 to
a8e91d0
Compare
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.
a8e91d0 to
73effc9
Compare
4e96633 to
ae9d7af
Compare
kriskowal
approved these changes
May 23, 2025
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Symbol.for('foo')for making a passable symbol, topassableSymbolForName('foo')Symbol('foo')for making a non-passable symbol for testing purposes, tounpassableSymbolForName('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.