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(async-flow): fix endowment equate bug #9736

Merged
merged 4 commits into from
Oct 4, 2024

Conversation

erights
Copy link
Member

@erights erights commented Jul 18, 2024

closes: #9830
refs: #9722 #9719

Description

#9719 originally failed on upgrade replay for an endowment. It revealed a bug introduced to async-flow when adding support for endowments. Because of the so-called "unwrapping" of some guests, there can be two guests corresponding to one host, with the host of course only mapping back to one of them -- the outer one. This makes bijection.js more complicated and irregular than an actual bijection.

equate(g, h) had a test for early return, if the g and h were already "equated", i.e., were corresponding guest and host. But the equate test was written before the elaboration of bijection. In fact, it should only test whether this guest g maps to the host h, irrespective of whether h maps back to this g.

Additional testing revealed that the unwrapped function was also not passable, and would fail to be passed back as an argument.

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

This change potentially makes the diagnostic error when misusing async-flow slightly less precise.

Testing Considerations

Introduces equate checks in the endowments test exercising the bijection. For endowments with are further "unwrapped", we test both the original guest (which was previously failing) and the unwrapped one (which also was, but for a different reason).

Since #9719 landed with a failing test, this PR also sets that test as passing, effectively working as an integration test of functions as endowments.

Upgrade Considerations

Can be deployed as a new version of the async-flow NPM package.

@erights erights self-assigned this Jul 18, 2024
@erights erights requested a review from mhofman July 18, 2024 22:17
Copy link

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

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0b38035
Status: ✅  Deploy successful!
Preview URL: https://45b9a6fe.agoric-sdk.pages.dev
Branch Preview URL: https://markm-fix-async-flow-endowme-95hz.agoric-sdk.pages.dev

View logs

@erights erights force-pushed the markm-fix-async-flow-endowment-bug-2 branch from 6fb4df3 to 1b4f248 Compare July 22, 2024 01:05
@mhofman mhofman assigned mhofman and unassigned erights Jul 29, 2024
@mhofman mhofman added the asyncFlow related to membrane-based replay and upgrade of async functions label Jul 29, 2024
@mhofman mhofman linked an issue Aug 2, 2024 that may be closed by this pull request
mergify bot added a commit that referenced this pull request Sep 28, 2024
refs: #9303

## Description
This adds a test for upgrading send-anywhere. It doesn't yet pass without these in-flight PRs,
- #9736
- #9785

We've agreed to land this without those to reduce work-in-flight.

### Security Considerations
none

### Scaling Considerations
none

### Documentation Considerations
none

### Testing Considerations
The failing test has a link to the issue to make it pass.

### Upgrade Considerations
none
@mhofman mhofman force-pushed the markm-fix-async-flow-endowment-bug-2 branch from bd80b3e to 3e45a25 Compare October 1, 2024 19:08
@mhofman mhofman marked this pull request as ready for review October 2, 2024 05:32
@mhofman mhofman requested a review from a team as a code owner October 2, 2024 05:32
@mhofman mhofman requested a review from michaelfig October 2, 2024 05:33
@mhofman
Copy link
Member

mhofman commented Oct 2, 2024

@erights please take a look to the small change I added, and to the test, I cannot add you as an official reviewer since you opened the PR.

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM. It's not too complex, so I'm comfortable approving it, knowing that I might be called on to maintain it.

@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Oct 4, 2024
@erights erights force-pushed the markm-fix-async-flow-endowment-bug-2 branch from 7b06833 to 90febcc Compare October 4, 2024 00:42
@mhofman mhofman force-pushed the markm-fix-async-flow-endowment-bug-2 branch from 90febcc to 0b38035 Compare October 4, 2024 21:53
@mergify mergify bot merged commit 4bc297d into master Oct 4, 2024
80 checks passed
@mergify mergify bot deleted the markm-fix-async-flow-endowment-bug-2 branch October 4, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
asyncFlow related to membrane-based replay and upgrade of async functions automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

async-flow bijection is not always symmetric
3 participants