fix(pass-style): avoid symbol-named methods #2797

erights · 2025-05-07T21:52:30Z

Closes: #XXXX
Refs: #2793 #2792

Description

#2840 is the safely-pure-refactor portions that were extracted from this PR. This PR is then rebased on #2840, representing only the elements whose refactor-purity was questionable. So this PR should be reviewed move skeptically with regard to these questionable remaining almost-refactor elements.

An upcoming PR #2792 will impose restrictions on the values that can serve as method names of remotable. The precise restrictions TBD (at this time, #2792 is still Draft), but the biggest change will be to stop allowing symbols as method names, with possible special cases for select symbols like Symbol.asyncIterator. That PR has many compat hazards, and can probably only be rolled out as an endo release after the one releasing this PR.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

all tests that broke in CI, fixed. For this PR, that should be adequate.

Compatibility Considerations

Most of the remotable creation we needed to change were just local tests, so no hazard with them. A few look like they may be exposed to clients, which would be a compat hazard. We don't expect any, but we will test against agoric-sdk before merging this PR.

Upgrade Considerations

none

packages/daemon/src/reader-ref.js

erights · 2025-05-07T22:05:05Z

packages/pass-style/src/iter-helpers.js

+  /**
+   * An `Iterable` should not be a `Remotable` because it has a symbol-named
+   * method. That's why we use `harden` rather than, say, `Far`. However,
+   * an `Iterator` can be a `Remotable`.
+   *
+   * @type {Iterable<U>}
+   */
+  harden({


TODO test in agoric-sdk.

This is used by code in agoric-sdk, but it doesn't seem like anything relies on these Iterables being remotable, so it should be ok if they no longer are.

packages/pass-style/src/passStyle-helpers.js

packages/patterns/src/keys/checkKey.js

erights · 2025-05-08T03:30:17Z

packages/patterns/src/keys/checkKey.js

@@ -392,48 +391,14 @@ harden(getCopyMapValues);
 * @template {Key} K
 * @template {Passable} V
 * @param {CopyMap<K,V>} m
- * @returns {Array<[K,V]>}
- */
-export const getCopyMapEntryArray = m => {


TODO test in agoric-sdk

I renamed this method with its types to getCopyMapEntries which is more inline with similar names for other copy collections. I deleted the old getCopyMapEntries which returned an iterable, because nothing in endo or agoric-sdk seems sensitive to the difference between this being only an Iterable vs it being an array that also happens to be an Iterable. The name getCopyMapEntryArray was only used within this package and was never exported beyond this package, so no need to deprecate. We simply removed it.

erights · 2025-05-08T03:49:28Z

This is now Ready for Review. PTAL

kriskowal

As this is not a pure refactor, I would like to fully separate the changes that have zero risk from the changes that have non-zero risk.

The changes under daemon have zero risk.

Changing PropertyKey to RemotableMethodName that is effectively equivalent to PropertyKey has almost negligible risk. I don’t expect to find any number keys in Exo. But, I will still want to see an integration PR pass bound to this.

Changing getCopyMapEntries and removing getCopyMapEntryArray is risky. I might be inclined to just not do that in order to avoid that risk, but I would like to be able to bisect that risk with a separate commit, maybe even a separate PR, and I need a nod from @mhofman that it won’t affect the chain.

packages/pass-style/src/types.d.ts

erights · 2025-06-03T03:18:07Z

As this is not a pure refactor, I would like to fully separate the changes that have zero risk from the changes that have non-zero risk.

Done. The zero-risk changes moved to #2840. I then rebased this one on #2840 .

The changes under daemon have zero risk.

Moved to #2840

Changing PropertyKey to RemotableMethodName that is effectively equivalent to PropertyKey has almost negligible risk. I don’t expect to find any number keys in Exo. But, I will still want to see an integration PR pass bound to this.

The RemotableMethodName in #2840 is just an alias for PropertyKey, in keeping with the zero-risk refactor goal.

The more substantive change remains in this PR.

Changing getCopyMapEntries and removing getCopyMapEntryArray is risky. I might be inclined to just not do that in order to avoid that risk, but I would like to be able to bisect that risk with a separate commit, maybe even a separate PR, and I need a nod from @mhofman that it won’t affect the chain.

Fully omitted from #2840 . Remains here.

@kriskowal

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

erights self-assigned this May 7, 2025

erights commented May 7, 2025

View reviewed changes

packages/daemon/src/reader-ref.js Outdated Show resolved Hide resolved

erights force-pushed the markm-avoid-symbol-named-methods branch from b99a243 to e15427b Compare May 7, 2025 22:00

erights commented May 7, 2025

View reviewed changes

packages/pass-style/src/passStyle-helpers.js Outdated Show resolved Hide resolved

erights commented May 7, 2025

View reviewed changes

packages/patterns/src/keys/checkKey.js Outdated Show resolved Hide resolved

erights force-pushed the markm-avoid-symbol-named-methods branch from e15427b to b9349cf Compare May 7, 2025 22:14

erights mentioned this pull request May 7, 2025

fix!: only string named methods #2792

Draft

1 task

erights force-pushed the markm-abstract-symbol-construction branch from 3bc9bd6 to 0e9b250 Compare May 7, 2025 23:47

erights force-pushed the markm-avoid-symbol-named-methods branch 2 times, most recently from a2cb751 to 3c69fc7 Compare May 8, 2025 03:24

erights commented May 8, 2025

View reviewed changes

erights marked this pull request as ready for review May 8, 2025 03:48

erights requested review from kumavis, leotm and kriskowal May 8, 2025 03:48

erights force-pushed the markm-abstract-symbol-construction branch from 0e9b250 to a8e91d0 Compare May 12, 2025 19:29

erights force-pushed the markm-avoid-symbol-named-methods branch from 3c69fc7 to 9bf3a56 Compare May 12, 2025 19:30

erights force-pushed the markm-abstract-symbol-construction branch from a8e91d0 to 73effc9 Compare May 15, 2025 21:19

erights force-pushed the markm-avoid-symbol-named-methods branch from 9bf3a56 to 3aa39b2 Compare May 15, 2025 21:22

erights force-pushed the markm-abstract-symbol-construction branch from 4e96633 to ae9d7af Compare May 17, 2025 01:38

erights force-pushed the markm-avoid-symbol-named-methods branch from 3aa39b2 to 175969d Compare May 17, 2025 01:39

kriskowal requested changes May 23, 2025

View reviewed changes

packages/pass-style/src/types.d.ts Outdated Show resolved Hide resolved

Base automatically changed from markm-abstract-symbol-construction to master May 24, 2025 17:59

erights force-pushed the markm-avoid-symbol-named-methods branch 2 times, most recently from edbc30d to 7751603 Compare June 3, 2025 02:50

erights changed the base branch from master to markm-2797-refactor-only-part June 3, 2025 02:54

erights mentioned this pull request Jun 3, 2025

refactor(pass-style): refactor-only avoid symbol-named methods #2840

Merged

erights changed the title ~~refactor(pass-style): avoid symbol-named methods~~ fix(pass-style): avoid symbol-named methods Jun 3, 2025

erights requested a review from kriskowal June 3, 2025 03:18

erights force-pushed the markm-2797-refactor-only-part branch from 92dcebe to dda45be Compare June 4, 2025 23:41

erights force-pushed the markm-avoid-symbol-named-methods branch from 7751603 to 830380e Compare June 4, 2025 23:42

erights force-pushed the markm-2797-refactor-only-part branch from dda45be to 0250186 Compare June 25, 2025 04:44

erights added the ocapn label Jun 30, 2025

erights mentioned this pull request Jun 30, 2025

spike(pass-style)! remove passable symbols to see what breaks #2452

Closed

erights force-pushed the markm-2797-refactor-only-part branch from 0250186 to dd4055e Compare July 9, 2025 03:51

erights force-pushed the markm-avoid-symbol-named-methods branch from 830380e to 16d4a31 Compare July 9, 2025 05:42

erights force-pushed the markm-2797-refactor-only-part branch 2 times, most recently from fc85130 to c87dfb6 Compare July 11, 2025 18:34

erights mentioned this pull request Jul 11, 2025

Abstract out canBeMethodName so later PRs agree on method name restrictions #2884

Open

Base automatically changed from markm-2797-refactor-only-part to master July 11, 2025 21:34

erights force-pushed the markm-avoid-symbol-named-methods branch from 16d4a31 to a2fa3c1 Compare July 14, 2025 01:55

fix(pass-style): avoid symbol-named methods

6b58365

erights force-pushed the markm-avoid-symbol-named-methods branch from a2fa3c1 to 6b58365 Compare July 21, 2025 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(pass-style): avoid symbol-named methods #2797

fix(pass-style): avoid symbol-named methods #2797

erights commented May 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

erights May 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

erights May 8, 2025

Uh oh!

erights commented May 8, 2025

Uh oh!

kriskowal left a comment

Uh oh!

Uh oh!

erights commented Jun 3, 2025

Uh oh!

Uh oh!

fix(pass-style): avoid symbol-named methods #2797

Are you sure you want to change the base?

fix(pass-style): avoid symbol-named methods #2797

Conversation

erights commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

Uh oh!

Uh oh!

erights May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erights May 8, 2025

Choose a reason for hiding this comment

Uh oh!

erights commented May 8, 2025

Uh oh!

kriskowal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erights commented Jun 3, 2025

Uh oh!

Uh oh!

erights commented May 7, 2025 •

edited

Loading

erights May 7, 2025 •

edited

Loading