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(vow): fix vow bug instead #9702

Closed
wants to merge 1 commit into from
Closed

Conversation

erights
Copy link
Member

@erights erights commented Jul 13, 2024

Staged on #9696

closes: #XXXX
refs: #XXXX

Description

@mhofman 's comment at #9696 (comment) indicated a puzzling rejection in a place that should not happen. If the code were correct, this extra .catch(reject) should not have been necessary, and should never do anything. The flaw in when.js is caused by the await at the top of

        result = await basicE(vowV0)
          .shorten()
          .then(
            res => {...},
            e => {
              ...
              throw e;
            },
          );

The problem is that if that onRejected clause happens, it throws e. That should be fine, because it is in a then which will turn that throw into a rejected promise. However the await at the top of the above snippet turns it back into a thrown error.

This PR does not remove that await. That turned out to be trickier than I expected. Rather, it surrounds most of the when code with a try/catch, turning any error thrown in the body into a call to the onRejected callback if present.

Reviewers, please review with whitespace differences turned off. Or, @turadg , feel free to simply merge this PR into your #9696.

Security Considerations

none beyond the usual: Correct code is better for security than incorrect code.

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

Would be good for this to be caught by a vow test. Attn @michaelfig

Upgrade Considerations

none

@erights erights self-assigned this Jul 13, 2024
Copy link

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 52e75a0
Status: ✅  Deploy successful!
Preview URL: https://187d79e0.agoric-sdk.pages.dev
Branch Preview URL: https://markm-fix-vow-bug-instead.agoric-sdk.pages.dev

View logs

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

Way too complicated. Alternative PR incoming

@erights
Copy link
Member Author

erights commented Jul 13, 2024

Closing in favor of #9704

@erights erights closed this Jul 13, 2024
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.

2 participants