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 failure handling #9696

Merged
merged 2 commits into from
Jul 14, 2024
Merged

fix vow failure handling #9696

merged 2 commits into from
Jul 14, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Jul 12, 2024

follow up on #9449

Description

#9685 unwrapped an asVow() to get the promise failure to propagate. This reverts that now that #9704 removed the need for it.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

CI

Upgrade Considerations

not yet deployed

Copy link

cloudflare-workers-and-pages bot commented Jul 12, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: c9969a5
Status: ✅  Deploy successful!
Preview URL: https://c5baeeac.agoric-sdk.pages.dev
Branch Preview URL: https://ta-vow-fail.agoric-sdk.pages.dev

View logs

@turadg turadg mentioned this pull request Jul 12, 2024
@turadg turadg marked this pull request as ready for review July 12, 2024 19:17
@@ -571,7 +571,7 @@ export const makeReplayMembrane = ({
}
}
},
);
).catch(reject);
Copy link
Member

Choose a reason for hiding this comment

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

This really shouldn't be necessary, we should never have a rejection at this point. We're likely papering over a much larger bug.

Copy link
Member

Choose a reason for hiding this comment

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

@erights could you help me figure this one out?

Copy link
Member

Choose a reason for hiding this comment

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

What I found out so far:

An error is being throw from then body of the async when function defined at the bottom of when.js

The error is

Error: chain info lacks staking denom
    at makeError (file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/node_modules/ses/src/error/assert.js:347:61)
    at fail (file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/node_modules/ses/src/error/assert.js:479:20)
    at Fail (file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/node_modules/ses/src/error/assert.js:489:39)
    at file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/packages/orchestration/src/exos/remote-chain-facade.js:98:25
    at asVow (file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/packages/vow/src/vow-utils.js:90:16)
    at Object.makeAccount (file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/packages/orchestration/src/exos/remote-chain-facade.js:94:18)
    at In "makeAccount" method of (RemoteChainFacade public) [as makeAccount] (file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/node_modules/@endo/exo/src/exo-tools.js:171:24)
    at performCall (file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/packages/async-flow/src/replay-membrane.js:136:30)
    at guestCallsHost (file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/packages/async-flow/src/replay-membrane.js:197:19)
    at In "makeAccount" method of (RemoteChainFacade public) [as makeAccount] (file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/packages/async-flow/src/replay-membrane.js:466:16)
    at <anonymous> (/Users/markmiller/src/ongithub/agoric/agoric-sdk/packages/orchestration/test/facade.test.ts:121:33) {stack: 'Error: chain info lacks staking denom
    at …ges/orchestration/test/facade.test.ts:121:33)', message: 'chain info lacks staking denom'}

The hVow argument is a vow for a promise rejected with this error.

The when function is not calling the onRejected handler with this error.

What I don't yet understand is why the when body throws this error instead of passing it to the onRejected callback.

attn @michaelfig ?

Copy link
Member

@mhofman mhofman Jul 13, 2024

Choose a reason for hiding this comment

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

Exactly what I found. This is a bug in vow's when which completely ignores the handlers if there is a rejection at any point. I'm fixing now.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, I think a bug in the when implementation. PR coming shortly if I'm right.

Copy link
Member

@mhofman mhofman Jul 13, 2024

Choose a reason for hiding this comment

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

I have the PR and test incoming

Edit: #9704

@turadg turadg changed the title ta/vow fail fix vow failure handling Jul 12, 2024
@turadg turadg marked this pull request as draft July 12, 2024 20:50
@mhofman
Copy link
Member

mhofman commented Jul 13, 2024

I have confirmed that with the fix in #9704 and the .catch(reject) removed, the new test here passes. Once that PR merges, I'll update this PR, but I'm wondering if this would be better stacked in top of #9701 ?

@turadg
Copy link
Member Author

turadg commented Jul 13, 2024

better stacked in top of #9701 ?

This only needs 4e09ff4 from 9107 so I've trimmed this PR to merge indepenently.

@turadg turadg marked this pull request as ready for review July 13, 2024 17:20
Copy link
Member

@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.

LGTM, thanks!

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Jul 14, 2024
@mergify mergify bot merged commit 66bf702 into master Jul 14, 2024
94 checks passed
@mergify mergify bot deleted the ta/vow-fail branch July 14, 2024 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants