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(zoe): Fix guards to accurately guard args #8642

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

erights
Copy link
Member

@erights erights commented Dec 11, 2023

closes: #XXXX
refs: #8641 #8514 endojs/endo#1765 endojs/endo#1890

Description

#8641 throws errors when a call has more arguments than are declared in a method guard. These throws have diagnosed several errors that were otherwise undiagnosed, even after endojs/endo#1765 . After endojs/endo#1765 these mismatches would have been genuine observable runtime bugs that remained undiagnosed by CI, as seen by the clean CI run at #8514 .

This PR reproduces the bug fixes from #8641 by themselves, without the endo-branch directive or the rest of #8514 , testing that these fixes are also compat with current agoric-sdk and should be merged as is.

Security Considerations

More accurate arg guarding supports better input validation.

Scaling Considerations

None

Documentation Considerations

For the methods whose guards are fixed here, we should also check whether the documentation of those methods include those args.

Testing Considerations

The fact that the bugs fixed in this PR were not detected by #8514 shows that endojs/endo#1765 was too hazardous: It would introduce changes in behavior that could be unlikely to be detected in tests, but be genuine dynamic bugs.

Upgrade Considerations

See the Upgrade Considerations of endojs/endo#1890

@erights erights self-assigned this Dec 11, 2023
@erights erights marked this pull request as ready for review December 11, 2023 04:32
packages/zoe/src/typeGuards.js Outdated Show resolved Hide resolved
@erights erights added the automerge:squash Automatically squash merge label Dec 11, 2023
@erights erights removed the automerge:squash Automatically squash merge label Dec 11, 2023
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

A test that would have previously failed would hedge against regression.

@erights
Copy link
Member Author

erights commented Dec 11, 2023

A test that would have previously failed would hedge against regression.

Done. Since this PR is currently tested against an endo prior to endojs/endo#1765 , the test's difference in behavior is in what error message is generated. This same test, prior to the rest of this PR while still prior to endojs/endo#1765 emits an error message from the operation itself. Whereas with the rest of this PR, we're getting the error from the guard mismatch.

After agoric-sdk is updated to current endo, i.e., one post endojs/endo#1890 , then this test without the rest of this PR would still fail from a guard mismatch, but still with a different error: that there were too many arguments.

@erights erights mentioned this pull request Dec 12, 2023
Copy link

@erights - This PR appears to be stuck. It's had a merge label for > 24 hours

1 similar comment
Copy link

@erights - This PR appears to be stuck. It's had a merge label for > 24 hours

@turadg turadg force-pushed the markm-fix-missing-arg-guards branch from 4c935b5 to 3016a9a Compare December 15, 2023 15:38
@mergify mergify bot merged commit b235bd8 into master Dec 15, 2023
67 checks passed
@mergify mergify bot deleted the markm-fix-missing-arg-guards branch December 15, 2023 16:08
anilhelvaci pushed a commit to Jorge-Lopes/agoric-sdk that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants