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

Cannot JSON.stringify Remotables #2207

Closed
erights opened this issue Jan 16, 2021 · 6 comments
Closed

Cannot JSON.stringify Remotables #2207

erights opened this issue Jan 16, 2021 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@erights
Copy link
Member

erights commented Jan 16, 2021

bad-stringify

On the left we see a call to JSON.stringify in lib-wallet.js. On the right we see in the debugger an expansion of the values argument being stringified. We see at the lower right that, at [0].value[0] this includes handle, installation, and instance. Currently these are simply empty objects, and this stringifies them as empty objects. However, these are actually remotables. As explained at #804 and #2018 (expecially #2018 (comment) ) we must declare them as Remotable instead, as #2178 does.

This debugger screenshot is from debugging #2178 where these are properly declared Remotable. However, these are still stringified as empty objects. The breakage at https://github.com/Agoric/agoric-sdk/pull/2178/checks?check_run_id=1707115405 is due to this. What happens is when

const zoeInvitePurseState3 = JSON.parse(
pursesStateChangeLog[pursesStateChangeLog.length - 1],
)[0];
t.deepEqual(
zoeInvitePurseState3,
{

first does a JSON.parse to recover this structure, which turns these remotables back into plain empty objects, and then does an ava deepEqual comparison with what should be the equivalent structure but had not gone through this information loss.

To fix this correctly, we should use marshal's serialize and unserialize rather than JSON's stringify and parse. But I don't know what the purpose of the stringification in lib-wallet.js is, so I don't know what a correct fix is.

#2178 is blocked on this. Attn @michaelfig

There are a tremendous number of calls to JSON.stringify throughout our code. I have no idea if others have similar problems, but I worry.

@erights erights added the bug Something isn't working label Jan 16, 2021
@erights
Copy link
Member Author

erights commented Jan 16, 2021

This debugging display was only possible due to endojs/endo#559 . That PR was motivated by the experience of trying to figure out why #2178 was failing, which was finally diagnosed as this bug.

@michaelfig
Copy link
Member

I will try to comment on this more fully, but please don't assume that the flattening of presences is unintended. Several of the data structures are conveyed over json already. Different paths are used when speaking captp.

@erights
Copy link
Member Author

erights commented Jan 17, 2021

please don't assume that the flattening of presences is unintended.

Excellent news! Looking forward to understanding this. Thanks.

erights added a commit that referenced this issue Jan 17, 2021
@erights
Copy link
Member Author

erights commented Jan 17, 2021

If it is intentional, is 8ad59d5 a correct fix to unblock #2178 ?

erights added a commit that referenced this issue Jan 17, 2021
@michaelfig
Copy link
Member

If it is intentional, is 8ad59d5 a correct fix to unblock #2178 ?

IIUC, yes it is.

erights added a commit that referenced this issue Jan 19, 2021
@erights erights self-assigned this Feb 25, 2021
@erights
Copy link
Member Author

erights commented Feb 15, 2022

Fixed by fixing #2018

@erights erights closed this as completed Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants