-
Notifications
You must be signed in to change notification settings - Fork 212
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
Conversation
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 |
@@ -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()??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, needs Data().
4335679
to
6e9488a
Compare
6e9488a
to
8429da5
Compare
@michaelfig this is ready for proper review now. The safety check to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM111
@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:
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.