fix(pass-style)! flip which symbols are passable#2777
Conversation
70adb87 to
69a390d
Compare
|
So far at least, it is very satisfying that I only needed to change two non-test files, only one non-trivially, and everything else other than passable symbol tests just worked. |
|
I don't understand how unregistered symbols with fresh identities can be used in anything meaningful. How do you look up that symbol on an object ? Do you have to iterate over all properties and check the description? That seems irrational. |
|
The ocapn Selector is a primitive forgeable type, like strings and numbers. Because it is primitive, it would be nice to map it onto JS primitives, i.e., such that
See #2779 Except for this case, which we will fix, there is no other possibility for passable objects to have symbol-named properties. |
|
IOW, neither @kriskowal nor I (and possibly also not @kumavis ) liked mapping an ocapn primitive value onto a JS object. OTOH, we have no choice but to map an ocapn ByteArray to a restricted Immutable ArrayBuffer, so this is a distinction that has to give anyway. I still think it is cleaner not to do it for ocapn Selectors. |
|
In OCapN as with status quo for So, in short, symbols are not terribly useful for Endo’s purposes, but using Symbols to denote the same passable values in OCapN, JavaScript, and Guile Scheme will be less confusing than alternatives. |
In our use of it, as in the OCapN semantics, it is forgeable in our distributed object semantics. At the JS level, they happen to be unforgeable, but we need to steer clear of that unforgeability. |
| // Cannot use `t.deepEqual` because it does not recognize that two | ||
| // unregistered symbols with the same `description` are the same in | ||
| // our distributed object semantics. Unfortunately, `compareRank` is | ||
| // too imprecise. We'd like to also test `keyEQ`, but that would violate |
There was a problem hiding this comment.
compareRankis too imprecise.
could you explain more here?
There was a problem hiding this comment.
Thanks. Comment on these usage sites deleted, and instead all these call the new makeFullOrderComparatorKit, which
- uses
fullComparerather thanrankCompareto get better precision. The difference is thatfullComparewill only judge two remotables as equal if they are the same remotable, whereasrankCompareconsiders all remotable to have the same rank. Really, that's the relevant additional precision we would have gotten fromkeyEQanyway.
The outliers are promises and errors. In our distributed object semantics, neither of these have a meaningful equality.
rankCompareandfullCompareboth treat all promises as having the same rank, and treat all errors as having the same rank.- Neither promises nor errors are Keys, so
keyEQrejects both.
Does this answer you question clearly?
| [Symbol.for('foo'), { '@qclass': 'symbol', name: 'foo' }], | ||
| // Registered symbol hilbert hotel | ||
| [Symbol.for('@@foo'), { '@qclass': 'symbol', name: '@@@@foo' }], | ||
| [Symbol.asyncIterator, { '@qclass': 'symbol', name: 'Symbol.asyncIterator' }], |
There was a problem hiding this comment.
Does this mean well known symbols are still passable?
There was a problem hiding this comment.
Good question!
-
To ease the transition, we are currently making a temporary special case for
Symbol.asyncIteratorboth for endo here, and for agoric-sdk at fix: prepare for symbol flip Agoric/agoric-sdk#11338 -
Aside from that temporary special case, we treat all unregistered symbols as passable, whether well-known or not. However, when we unserialize such symbols, we create fresh ones with the same description, as we do for all unregistered symbols. Thus they lose their JS equality to the well-known symbols.
kumavis
left a comment
There was a problem hiding this comment.
asked a few clarifying questions
|
Putting into Draft until we have time to discuss this approach. |
Now that this is in draft in order to postpone discussion, I'm going to do a bunch of squashing as well as merging #2779 PR into #2777 |
Staged on #2777 Closes: #XXXX Refs: #XXXX ## Description No more symbol named methods in remotable. Needed for #2777 to make sense. But a separate PR for now because it causes more errors to investigate. ### Security Considerations > Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls? ### Scaling Considerations > Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated? ### Documentation Considerations > Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users? ### Testing Considerations > Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet? ### Compatibility Considerations > Does this change break any prior usage patterns? Does this change allow usage patterns to evolve? ### Upgrade Considerations > What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed? > Include `*BREAKING*:` in the commit message with migration instructions for any breaking change. > Update `NEWS.md` for user-facing changes. > Delete guidance from pull request description before merge (including this!)
Staged on #2777 Closes: #XXXX Refs: #XXXX ## Description No more symbol named methods in remotable. Needed for #2777 to make sense. But a separate PR for now because it causes more errors to investigate. ### Security Considerations > Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls? ### Scaling Considerations > Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated? ### Documentation Considerations > Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users? ### Testing Considerations > Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet? ### Compatibility Considerations > Does this change break any prior usage patterns? Does this change allow usage patterns to evolve? ### Upgrade Considerations > What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed? > Include `*BREAKING*:` in the commit message with migration instructions for any breaking change. > Update `NEWS.md` for user-facing changes. > Delete guidance from pull request description before merge (including this!)
1b342d7 to
35d53eb
Compare
| [], | ||
| 'well known symbol', | ||
| ); | ||
| assertRoundTrip(Symbol('foo'), '#"%foo"', [], 'reg symbol'); |
There was a problem hiding this comment.
| assertRoundTrip(Symbol('foo'), '#"%foo"', [], 'reg symbol'); | |
| assertRoundTrip(Symbol('foo'), '#"%foo"', [], 'unreg symbol'); |
35d53eb to
84d4f5c
Compare
|
I believe we have tentative consensus that we want to move toward modeling CapTP symbols as JavaScript objects with passStyle So, the representation of CapTP symbols would bi-map:
For OCapN, we would bi-map method names to avoid collisions with loaded JS object property names:
I recommend that we close this PR and reconstruct the change for the new consensus. For expedience please reopen if you disagree. |
…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.
…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.
There was a problem hiding this comment.
Pull Request Overview
This PR fundamentally changes which symbols are considered passable in the distributed object system, flipping from treating registered and well-known symbols as passable to treating only unregistered symbols as passable. This change aims to solve garbage collection issues and improve distributed object semantics by treating unregistered symbols as equal based on their description string.
- Flip symbol passability: unregistered symbols are now passable, registered symbols are not
- Update symbol serialization to use description strings instead of registry keys
- Add legacy support for Symbol.asyncIterator via environment variable
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/pass-style/src/symbol.js | Core logic change to flip which symbols are passable |
| packages/pass-style/src/remotable.js | Add validation to reject remotables with symbol-named methods |
| packages/pass-style/src/passStyle-helpers.js | Add helper for method name validation |
| packages/marshal/tools/ava-full-order-eq.js | New testing utility for distributed object equality |
| packages/patterns/src/keys/checkKey.js | Remove Far wrapper from iterable to avoid symbol method issues |
| packages/marshal/src/marshal-justin.js | Update symbol serialization format in Justin encoding |
| Multiple test files | Update tests to use unregistered symbols and new testing utilities |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| getEnvironmentOption('PASS_STYLE_LEGACY_ASYNC_ITERATOR_SYMBOL', 'enabled', [ | ||
| 'disabled', |
There was a problem hiding this comment.
The environment variable name PASS_STYLE_LEGACY_ASYNC_ITERATOR_SYMBOL contains "LEGACY" but defaults to 'enabled' which contradicts the description stating it defaults to 'disabled'. This inconsistency could cause confusion about the actual default behavior.
| getEnvironmentOption('PASS_STYLE_LEGACY_ASYNC_ITERATOR_SYMBOL', 'enabled', [ | |
| 'disabled', | |
| getEnvironmentOption('PASS_STYLE_LEGACY_ASYNC_ITERATOR_SYMBOL', 'disabled', [ | |
| 'enabled', |
| export const passableSymbolForName = name => | ||
| specialCaseAsyncIteratorSymbol && | ||
| ['@@asyncIterator', symbolAsyncIteratorDescription].includes(name) | ||
| ? Symbol.asyncIterator | ||
| : Symbol(name); |
There was a problem hiding this comment.
The hardcoded array ['@@asyncIterator', symbolAsyncIteratorDescription] contains a magic string '@@asynciterator' that should be extracted to a named constant for better maintainability and to avoid typos.
| (canBeMethodName(key) || | ||
| (!!check && | ||
| CX(check)`A pass-by-remote cannot shadow ${q(PASS_STYLE)}`)))) | ||
| CX( | ||
| check, | ||
| )`Remotables can only have string-named methods: ${String(key)}`)))) |
There was a problem hiding this comment.
The error message uses String(key) which will produce unhelpful output like "Symbol(Symbol.iterator)" for symbols. Consider using a more descriptive representation or quoting the symbol appropriately for better error clarity.
Closes: #XXXX
Refs: ocapn/ocapn#165 #2772 #2774 #2778 #2779 https://github.com/tc39/proposal-symbol-predicates Agoric/agoric-sdk#11338
Description
@kriskowal and @kumavis just had a great idea! Instead of simply avoiding symbols altogether because of gc worries, switch from treating only registered and well known symbols as passable to treating only unregistered symbols as passable. Then, treat two unregistered symbols as equal within our distributed object semantics iff they have the same
descriptionstring.This does what I thought impossible: it recapitulates at the user level for unregistered symbols the implementation technique that I had recommended engines do internally for registered symbols --- forget any registry, and just treat two registered symbols are equal if they have the same contents. Just like JS already does for strings.
Most well known symbols are unregistered. I think they all must be unregistered, but at least on Node,
Symbol.disposeandSymbol.asyncDisposeare mistakenly registered symbols.Symbol.disposeandSymbol.asyncDisposeare mistakenly registered symbols.In any case, for all well-known symbols that are unregistered, these are passable with no special case. But remember that they unserialize to fresh symbol with distinct JS identities.
Security Considerations
By always unserializing to fresh symbols, we avoid coinciding with the identity of pre-existing JS symbols. This helps separate concerns in ways helpful to security.
Scaling Considerations
This fixes the gc problem we had with registered symbols without introducing a separate
Selectorobject type. This scales better.Documentation Considerations
This will make harmonization with ocapn smoother.
Testing Considerations
So far, the change has only required changes that were testing how symbols pass. We have not seen any other breakage yet. But the real test will be integration with agoric-sdk.
Compatibility Considerations
I marked this PR with
!because it is a compat break. To ease the transition I also added aPASS_STYLE_LEGACY_ASYNC_ITERATOR_SYMBOLenvironment variable which defaults to'disabled'. But if set to'enabled', it tries to be somewhat accommodating to legacy uses ofSymbol.asyncIteratoruntil we can retire those.passStyleOfto reject any such objects rather than classifying them as remotables.Upgrade Considerations
If we currently have any old-style passable symbols in durable storage, or if there are elsewhere persistent samples of serialized data containing old-style passable symbols, we may be in trouble. We need to check. If we are in trouble, we'll need an upgrade strategy to get across this transition.