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

feat(asyncFlow): Stopgap E support #9519

Merged
merged 13 commits into from
Jun 28, 2024
Merged

feat(asyncFlow): Stopgap E support #9519

merged 13 commits into from
Jun 28, 2024

Conversation

erights
Copy link
Member

@erights erights commented Jun 17, 2024

closes: #XXXX
refs: #9322, #9299 #9443

Description

PR #9322 is supposed to provide production quality support for asyncFlow guest functions to use E. It is being reviewed for that goal, and will not be merged until we think it meets that bar. However, we need to start integration testing of asyncFlow with orchestration, to spot mismatched assumptions we may have missed. For this purpose, we do not immediately need production quality E support. That is the purpose of this PR. It starts as a copy of the code from #9322 but need only be evaluated as adequate for these stopgap purposes before being merged. This PR does NOT claim to f-i-x #9299 , leaving that job to remain with #9322

Even though the requirements on this PR are so much lighter, reviewers should still look at the unresolved conversations on #9322 and determine if any of those need to first be solved even in this PR.

Security Considerations

When merging stopgap code to master, there is always the danger that it might be used as if it production code. We need to remember not to do so, instead waiting for #9322 to do the job for real.

Scaling Considerations

none

Documentation Considerations

just as this stopgap unblocks integration testing, it also likely unblocks documenting how to use asyncFlow, both in general and for orchestration.

Testing Considerations

As a stopgap, this PR does not need the rigorous testing that #9322 should have.

Upgrade Considerations

We need to not use this stopgap for production purposes.

@erights erights self-assigned this Jun 17, 2024
Copy link

cloudflare-workers-and-pages bot commented Jun 17, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: c811934
Status: ✅  Deploy successful!
Preview URL: https://f928622f.agoric-sdk.pages.dev
Branch Preview URL: https://markm-stopgap-asyncflow-e.agoric-sdk.pages.dev

View logs

@erights erights changed the title feat(asyncFlow): E support feat(asyncFlow): Stopgap E support Jun 17, 2024
@erights erights mentioned this pull request Jun 17, 2024
3 tasks
@erights erights marked this pull request as ready for review June 17, 2024 22:27
@erights erights marked this pull request as draft June 18, 2024 23:32
@erights
Copy link
Member Author

erights commented Jun 18, 2024

After talking to @dtribble , we are no longer confident we need this stopgap at all for integration testing, so I'm putting it into Draft. If we find we do need it before #9322 is ready, we can pick this one up again and proceed, so I'm keeping this PR open until then.

@erights erights added the asyncFlow related to membrane-based replay and upgrade of async functions label Jun 20, 2024
0xpatrickdev added a commit that referenced this pull request Jun 21, 2024
 * Perform remote calls to agoricNames in membrane-friendly way. This is an
 * interim approach until #9541,
 * #9322, or
 * #9519
0xpatrickdev added a commit that referenced this pull request Jun 21, 2024
 * Perform remote calls to agoricNames in membrane-friendly way. This is an
 * interim approach until #9541,
 * #9322, or
 * #9519
0xpatrickdev added a commit that referenced this pull request Jun 25, 2024
 * Perform remote calls to agoricNames in membrane-friendly way. This is an
 * interim approach until #9541,
 * #9322, or
 * #9519
0xpatrickdev added a commit that referenced this pull request Jun 25, 2024
 * Perform remote calls to agoricNames in membrane-friendly way. This is an
 * interim approach until #9541,
 * #9322, or
 * #9519
mergify bot added a commit that referenced this pull request Jun 25, 2024
refs: #9449

## Description

 Perform remote calls to agoricNames in membrane-friendly way. This is an interim approach until #9541, #9322, or #9519.
@erights
Copy link
Member Author

erights commented Jun 26, 2024

Note to self:
Why is it sending onOpen to an EchoConnectionKit?

@erights erights marked this pull request as ready for review June 26, 2024 19:20
@erights
Copy link
Member Author

erights commented Jun 26, 2024

Ready for review again! We've decided that integration testing will proceed including this PR. Though possibly with the addition of a switch for turning it off (restoring the previous error behavior) if we decide such a switch is warranted. That can easily happen after this PR.

Copy link
Member

@dtribble dtribble left a comment

Choose a reason for hiding this comment

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

Mostly a few questions to make sure I'm oriented.

packages/async-flow/src/replay-membrane.js Show resolved Hide resolved
packages/async-flow/src/replay-membrane.js Outdated Show resolved Hide resolved
packages/async-flow/src/replay-membrane.js Outdated Show resolved Hide resolved
packages/async-flow/src/replay-membrane.js Outdated Show resolved Hide resolved
@erights erights requested a review from dtribble June 26, 2024 20:45
@erights erights requested a review from dtribble June 27, 2024 19:05
@erights erights added the automerge:squash Automatically squash merge label Jun 27, 2024
mergify bot pushed a commit that referenced this pull request Jun 28, 2024
closes: #XXXX
refs: #9519

## Description

While trying to debug #9519 , I'm seeing errors like
```
Error#20: {"type":1,"data":"CmgKIy9jb3Ntb3Muc3Rha2luZy52MWJldGExLk1zZ0RlbGVnYXRlEkEKGFVOUEFSU0FCTEVfQ0hBSU5fQUREUkVTUxISYWdvcmljMXZhbG9wZXJmdWZ1GhEKBXVmbGl4EggxMDAwMDAwMA==","memo":""}
  at parseTxPacket (file:///Users/markmiller/src/ongithub/agoric/agoric-sdk/packages/orchestration/src/utils/packet.js:87:14)
```
which only has the data without any message text saying what's wrong with the data. While fixing, also switched to a `Fail` to give us control over redaction. I made the conservative assumption that to not unredact the actual packet data, because I don't know why it should be public on the error even in non-debugging scenarios. If it should be unredacted, I can easily change to `${q(response)}`. Reviewers, please advise.

Note that either way, the unredacted info will still show up in error logs, as above, and will still even show up on the error object in standard debugging configurations.

### Security Considerations
Possibly repaired an unintended disclosure of info that should have been redacted, though the main motivation was adding an informative error message.

### Scaling Considerations
none
### Documentation Considerations
none
### Testing Considerations
none
### Upgrade Considerations
none
@mergify mergify bot merged commit 4adf64f into master Jun 28, 2024
77 checks passed
@mergify mergify bot deleted the markm-stopgap-asyncFlow-E branch June 28, 2024 06:37
mergify bot added a commit that referenced this pull request Aug 2, 2024
refs: #9541, #9322, #9519

## Description

add `getVBankAssetInfo()` method on agoric chain object.

DRAFT until

 - [x] how to add chain-specific methods to return type of `getChain()`?
 - [x] refactor uses of `makeResumableAgoricNamesHack` to use this

### Documentation Considerations

change is visible via `orchestration-api.d.ts`
cc @dtribble 

### Security Considerations

none?

### Scaling Considerations

returns O(n) data from O(1) args

### Testing Considerations

perhaps not independently tested

### Upgrade Considerations

not yet deployed
michaelfig added a commit that referenced this pull request Sep 25, 2024
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:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants