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.

Sign up for GitHub

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

Jump to bottom

Json safe #620

Merged
merged 5 commits into from
May 25, 2024
Merged

Json safe #620

merged 5 commits into from
May 25, 2024

Conversation

pyramation
Copy link
Collaborator

@pyramation pyramation commented May 20, 2024 •
edited
Loading

  • added JsonSafe helper type
  • added the prototypes.typingsFormat.jsonSafe option
  • for v3/v4 fixtures output, set this option to true

should close #605

cc @turadg

@pyramation pyramation mentioned this pull request May 20, 2024
Closed
@turadg
Copy link
Contributor

turadg commented May 20, 2024

Looks like it does the job, but the option could be more clear. JsonSafe is a utility type, but doesn't really express the intent for the person setting the option.

Consider, prototypes.typingsFormat.returnKnown. That contracts with the unknown` default.

Though if it were me I'd always return the typed value and not have an option. I can't think of why anyone would prefer to get unknown to the true value. It also shouldn't be considered a breaking change since the change improves accuracy. (i.e. any type check that breaks with this is revealing a bug)

@pyramation
Copy link
Collaborator Author

great feedback @turadg

The reason we use options (for any feature) is so developers that upgrade can choose to have few code changes as possible since a large amount of code is generated.

I would suggest we do two phases. One smaller release (patch) with the option defaulting to unknown, and then a minor release to make it the default to have types

@pyramation pyramation merged commit fba9188 into main May 25, 2024
2 checks passed
@turadg turadg mentioned this pull request May 28, 2024
Merged
mergify bot added a commit to Agoric/agoric-sdk that referenced this pull request May 29, 2024
@mergify
605 telescope json (#9415)
e7745e6 
refs: #8917 

## Description

Uses cosmology-tech/telescope#620 to get proper
return types from `toJSON`. Cleans up our accommodations.

Review suggested by commit.

### Security Considerations

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations

<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

### Upgrade Considerations

<!-- What aspects of this PR are relevant to upgrading live production
systems, and how should they be addressed? -->
@turadg turadg mentioned this pull request Jun 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

return usable types from toJSON
2 participants
@pyramation @turadg