Conversation
Deploying agoric-sdk with
|
| Latest commit: |
ded3084
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eda4217f.agoric-sdk.pages.dev |
| Branch Preview URL: | https://markm-tolerate-symbol-flip-2.agoric-sdk.pages.dev |
85c058a to
e0838bb
Compare
acaba32 to
dd18b3b
Compare
|
Putting into Draft until we have time to discuss this approach. |
Reviewers, now that this is in Draft to postpone discussion, I'm gonna do a bunch of squashing. |
b8b5709 to
421ac55
Compare
421ac55 to
ded3084
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR prepares agoric-sdk to work with an upcoming change in the endo library that will flip which symbols are passable vs unpassable. The main purpose is to abstract symbol creation through new utility functions and update test code to use distributed object equality semantics.
Key changes include:
- Introduction of
passableSymbolForName()andunpassableSymbolForName()abstractions - Migration from
t.deepEqual()totestFullOrderEQ()for testing symbol equality - Updates to test code using symbols to use the new abstractions
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/internal/src/ses-utils.js | Adds symbol abstraction functions and flip detection logic |
| packages/internal/tools/ava-full-order-eq.js | New testing utility for distributed object equality |
| packages/cosmic-swingset/src/helpers/bufferedStorage.js | Production code updated to use unpassable symbols |
| packages/store/src/stores/store-utils.js | Removes Far wrapper from iterable keys object |
| Multiple test files | Updates to use new symbol abstractions and testFullOrderEQ |
| const m2 = passableSymbolForName('m2'); | ||
| // TODO This currently has a symbol-named method. If it passes, it is only | ||
| // because `o` was not made remotable. Should this test case me deleted, | ||
| // or should `m2` and its uses be deleted, or should `m2` be converted to | ||
| // `'m2'`? |
There was a problem hiding this comment.
The TODO comment indicates uncertainty about whether this test case should be deleted, whether the symbol should be removed, or whether it should be converted to a string. This ambiguity suggests the test may not be testing the intended behavior clearly. Consider resolving this uncertainty by either removing the symbol usage entirely or documenting the specific behavior being tested.
| const m2 = passableSymbolForName('m2'); | |
| // TODO This currently has a symbol-named method. If it passes, it is only | |
| // because `o` was not made remotable. Should this test case me deleted, | |
| // or should `m2` and its uses be deleted, or should `m2` be converted to | |
| // `'m2'`? | |
| const m2 = 'm2'; | |
| // This test now uses a string-named method for clarity and consistency. | |
| // Symbol-named methods are not tested here. |
| export const AT_NEXT = Symbol('AT_NEXT'); | ||
| // TODO What does it mean to type something to the typeof itself? | ||
| // That was the inferred type when the right hand side was `Symbol('AT_NEXT')` | ||
| export const AT_NEXT = /** @type {typeof AT_NEXT} */ ( |
There was a problem hiding this comment.
The self-referential type annotation typeof AT_NEXT creates a circular type reference that doesn't provide meaningful type information. Consider using a more specific type or removing the type assertion entirely since the function return type should be sufficient.
| export const AT_NEXT = /** @type {typeof AT_NEXT} */ ( | |
| export const AT_NEXT = /** @type {symbol} */ ( |
| // TODO if this passes with a symbol-named method, it is because | ||
| // some relevant object was not made into a remotable. |
There was a problem hiding this comment.
Similar to the previous TODO, this comment indicates uncertainty about whether the test is validating the correct behavior. The conditional nature of the comment ("if this passes") suggests the test outcome is unpredictable, which undermines test reliability.
| // TODO if this passes with a symbol-named method, it is because | |
| // some relevant object was not made into a remotable. | |
| // This test checks that isCallback accepts manual callback objects with symbol-keyed method names. |
#endo-branch: markm-flip-which-symbols-are-passable
linked with endojs/endo#2777
closes: #XXXX
refs: endojs/endo#2777 #11337
Description
endojs/endo#2777 will flip which symbols are passable. This PR prepares agoric-sdk to work both before and after that transition by abstracting the functions for creating passable and unpassable symbols by name.
Security Considerations
If this PR does work both before and after, none.
Scaling Considerations
Other than those already explained in endojs/endo#2777, none
Documentation Considerations
none
Testing Considerations
Other than providing the abstractions in ses-utils.js, the only non-test *.js file that needed revision was bufferedStorage.js. Aside from it, all other uses of symbol as of the first commit are only for testing.
t.deepEqualwhose callers had to be revised in fix(pass-style)! flip which symbols are passable endojs/endo#2777. The new distributed object semantic preserves equality for the "same" passable symbol according to its own equality predicated. But it does not preserve JS identity. We need to fix these callsites.Upgrade Considerations
The wire and durable representation of passable symbols in general will be the same. This is not true for well-known unregistered symbols. This PR is experimentally linked with endojs/endo#2777, which provides some legacy support specifically for
Symbol.asyncIterator. We'll see what problems remain.