Conversation
b99a243 to
e15427b
Compare
| /** | ||
| * 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({ |
There was a problem hiding this comment.
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.
e15427b to
b9349cf
Compare
3bc9bd6 to
0e9b250
Compare
a2cb751 to
3c69fc7
Compare
| * @param {CopyMap<K,V>} m | ||
| * @returns {Array<[K,V]>} | ||
| */ | ||
| export const getCopyMapEntryArray = m => { |
There was a problem hiding this comment.
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.
|
This is now Ready for Review. PTAL |
0e9b250 to
a8e91d0
Compare
3c69fc7 to
9bf3a56
Compare
a8e91d0 to
73effc9
Compare
9bf3a56 to
3aa39b2
Compare
4e96633 to
ae9d7af
Compare
3aa39b2 to
175969d
Compare
kriskowal
left a comment
There was a problem hiding this comment.
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.
edbc30d to
7751603
Compare
Done. The zero-risk changes moved to #2840. I then rebased this one on #2840 .
Moved to #2840
The The more substantive change remains in this PR.
Fully omitted from #2840 . Remains here. |
92dcebe to
dda45be
Compare
7751603 to
830380e
Compare
dda45be to
0250186
Compare
0250186 to
dd4055e
Compare
830380e to
16d4a31
Compare
fc85130 to
c87dfb6
Compare
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
16d4a31 to
a2fa3c1
Compare
a2fa3c1 to
6b58365
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors code to avoid using symbol-named methods in remotables as part of a preparation for upcoming restrictions. The changes simplify getCopyMapEntries to return arrays directly instead of iterable objects, and update iterables to use harden instead of Far when they contain symbol-named methods.
- Simplified
getCopyMapEntriesto return arrays directly instead of remotable iterables - Removed the separate
getCopyMapEntryArrayfunction and consolidated functionality - Updated iterable helpers to use
hardeninstead ofFarfor objects with symbol-named methods
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/patterns/test/copyMap.test.js | Updated tests to work with array return instead of iterable from getCopyMapEntries |
| packages/patterns/src/patterns/patternMatchers.js | Updated import and function call to use getCopyMapEntries instead of removed function |
| packages/patterns/src/keys/compareKeys.js | Updated import and function reference to use getCopyMapEntries |
| packages/patterns/src/keys/checkKey.js | Simplified getCopyMapEntries to return arrays directly and removed getCopyMapEntryArray |
| packages/pass-style/src/remotable.js | Reverted method name validation to exclude numbers (commented code removal) |
| packages/pass-style/src/iter-helpers.js | Changed iterables from Far to harden to avoid remotables with symbol-named methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
a25532e to
0ae2b3c
Compare
0ae2b3c to
244b278
Compare
|
244b278 to
0064388
Compare
Closes: #XXXX
Refs: #2793 #2792 #2840
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