Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cosmic-swingset): apply Far/Data everywhere I could think of #2567

Merged
merged 3 commits into from
Mar 11, 2021

Conversation

warner
Copy link
Member

@warner warner commented Mar 3, 2021

@michaelfig I added Far/Data everywhere I could think of, but the cosmic-swingset tests are still failing (with marshal.js's throw-on-unmarked-empty-objects asserts uncommented). I think you can probably spot the problem faster than me, also I think there's a lot of code that isn't covered by the tests that I'd probably miss. Can you move this forward?

Switch to this branch, then apply the following patch to marshal.js:

diff --git a/packages/marshal/src/marshal.js b/packages/marshal/src/marshal.js
index 1cb78971..6b58d3f8 100644
--- a/packages/marshal/src/marshal.js
+++ b/packages/marshal/src/marshal.js
@@ -293,8 +293,8 @@ function isPassByCopyRecord(val) {
   // error, then (TODO) it will go back to pass-by-copy.
   // See https://github.com/Agoric/agoric-sdk/issues/2018
   if (descKeys.length === 0 && proto === objectPrototype) {
-    // console.log(`--- @@marshal: empty object without Data/Far/Remotable`);
-    // assert.fail(X`empty object without Data/Far/Remotable`);
+    console.log(`--- @@marshal: empty object without Data/Far/Remotable`);
+    assert.fail(X`empty object without Data/Far/Remotable`);
     return false;
   }
   for (const descKey of descKeys) {
@@ -512,8 +512,9 @@ export function passStyleOf(val) {
         return 'copyRecord';
       }
       assertRemotable(val);
-      // console.log(`--- @@marshal: pass-by-ref object without Far/Remotable`);
-      // assert.fail(X`pass-by-ref object without Far/Remotable`);
+      const desc = Object.entries(val).map(([k,v]) => `[${k}]=${v}`).concat(' , ');
+      console.log(`--- @@marshal: pass-by-ref [${desc}] object without Far/Remotable`, Error());
+      assert.fail(X`pass-by-ref object without Far/Remotable`);
       return REMOTE_STYLE;
     }
     case 'function': {

I've experimented with commenting out the asserts but leaving the logs in place, and then grepping the output. Let me know if I can help out, happy to pair on this.

@warner warner added the cosmic-swingset package: cosmic-swingset label Mar 3, 2021
@michaelfig
Copy link
Member

I'll gladly peer with you tomorrow. The only error I saw on trivial inspection of the test output was handled by you in a different PR (the SwingSet/src/vats/network/ changes).

@@ -113,7 +114,7 @@ export function buildRootObject(vatPowers) {
...decentralObjects, // TODO: Remove; replaced by .agoric
...privateObjects, // TODO: Remove; replaced by .local
...handyObjects,
agoric: { ...decentralObjects },
agoric: { ...decentralObjects }, // TODO: maybe needs Data()???
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, needs Data().

@warner warner force-pushed the 2018-annotate-cosmic-swingset branch from 6e9488a to 8429da5 Compare March 10, 2021 21:56
@warner warner requested a review from michaelfig March 10, 2021 21:57
@warner
Copy link
Member Author

warner commented Mar 10, 2021

@michaelfig this is ready for proper review now. The safety check to store/weakstore will land in #2585 after this lands.

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM111

@warner warner enabled auto-merge (squash) March 10, 2021 23:06
@warner warner merged commit 92b63b6 into master Mar 11, 2021
@warner warner deleted the 2018-annotate-cosmic-swingset branch March 11, 2021 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cosmic-swingset package: cosmic-swingset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants