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(marshal): Avoid bigint literals for Google Apps Script compatibility #1589

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gibson042
Copy link
Contributor

Ref #1582

This doesn't fix the redefined issue, but hopefully does remove the incompatibility that originally prompted it.

@gibson042 gibson042 requested review from dckc and erights May 20, 2023 20:20
Copy link
Contributor

@dckc dckc left a comment

Choose a reason for hiding this comment

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

Would this change be visible in XS heap snapshots? I hesitate to approve until we're quite clear on how we're dealing with endo/agoric-sdk compatibility going forward.

@mhofman
Copy link
Contributor

mhofman commented May 23, 2023

Would this change be visible in XS heap snapshots? I hesitate to approve until we're quite clear on how we're dealing with endo/agoric-sdk compatibility going forward.

Changes in Endo will only be reflected once the vat is upgraded, either the lockdown / supervisor bundle and / or the contract itself. I think it's safe to merge changes in Endo, unless they are dealing with some kind of protocol that isn't backwards compatible (e.g. a new marshal encoding/serialization)

@dckc
Copy link
Contributor

dckc commented May 23, 2023

Changes in Endo will only be reflected once the vat is upgraded, either the lockdown / supervisor bundle and / or the contract itself.

Right... but suppose we decide later that we want to make a change in endo... call it B... one that does not impact the supervisor bundle. But we had already merged this change--call it A--which does impact the supervisor bundle. And we want to start new contracts/vats that take advantage of B. Are we forced to start them with a supervisor bundle that includes A? How do we even do that? Suppose it's not automatic, but we forget/neglect to do it... does the vat get launched with the pre-A supervisor bundle? Is it compatible?

I'm not going to stand in the way / request changes. But Google Apps Script compatibility is not a clear product requirement. Until we have changes motivated by a clear product requirement and we have a clear story on how they're going to be deployed, I'm not inclined to approve.

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

FWIW the change looks good to me. But I'm not approving to leave @dckc 's concern in place.

Separately, do we have any idea what other impediments there are to using marshal in Google Sheets?

It seems weird to me that there is a version of JS that does have bigints and bigint arithmetic, but does not parse bigint literals. How confident are we that this parsing issue is a limitation of the JS engine and not some earlier tool in their internal toolchain?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants