feat(async-flow): endowments #9566
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erights
added
the
asyncFlow
Deploying agoric-sdk with
|
| Latest commit: |
c4a252b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f7b41959.agoric-sdk.pages.dev |
| Branch Preview URL: | https://markm-endowment-asyncflow.agoric-sdk.pages.dev |
erights
force-pushed
the
markm-endowment-asyncFlow
branch
7 times, most recently
from
d19ac57
to
49046c8
Compare
erights
force-pushed
the
markm-endowment-asyncFlow
branch
from
49046c8
to
1947553
Compare
This was referenced Jun 23, 2024
Closed
erights
commented
Jun 24, 2024
dckc
reviewed
Jun 24, 2024
| const { denom } = await findBrandInVBank(amt.brand); | ||
| const chain = await orch.getChain(chainName); | ||
|
|
||
| // FIXME ok to use a heap var crossing the membrane scope this way? |
There was a problem hiding this comment.
Suggested change
| // FIXME ok to use a heap var crossing the membrane scope this way? |
There was a problem hiding this comment.
Done. Also cleaned up the two // XXX when() until membrane below
|
|
|
The existing failures are |
That problem fixed |
dtribble
approved these changes
Jun 25, 2024
mergify bot
pushed a commit
that referenced
this pull request
Jun 28, 2024
closes: #XXXX refs: https://github.com/Agoric/agoric-sdk/pull/9566/files#r1653602477 ## Description At https://github.com/Agoric/agoric-sdk/pull/9566/files#r1653602477 @dckc commented on a cleanup that I thought I did and marked as done, but was not done. Before #9566, the code that would be guest code had several workarounds marked `// XXX when() until membrane` that correctly indicated that they should be cleaned up as of #9566, which introduces exactly that membrane. In addition, guest code has no need for `heapVowE` since it doesn't see vows. It should just use the normal `E`. To keep this distinct, I stopped renaming `hostVowE` where host code needs it in the same file. ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations examples are now more like what we want to teach ### Testing Considerations I believe this PR is a pure refactor that should not have any observable effects. Thus, it should not affect any tests. We'll see soon. ### Upgrade Considerations none
This was referenced Sep 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes: #XXXX
refs: #9449 #9521 #9304 #9281
Description
Changed async-flow to support endowments. Changed
orchestrateto useasyncFlowwith endowments. ChangedsendAnywhereexample orchestration contract to be more compatible with this neworchestrate.The CI errors are all in the
orchestrationpackage. After some earlier iteration where orchestration failures indicated async-flow bugs, which I fixed, the remaining errors seem plausibly to be integration bugs on the orchestration side revealed by using this improvedorchestratefunction. If so, that satisfies the purpose of this PR -- to enable integration testing to reveal such errors. However, this leaves open the question of how to bring this PR to fruition despite these CI errors.In that iteration, the majority of errors were due to host-side promises, which we expected. To proceed with integration testing, I temporarily turned that case into a warning, by wrapping the host-side promise with a host-side vow. This stopgap measure is obviously fragile under upgrade. It would cause may upgrades to fail
However, I have not investigated these CI errors enough to be at all confident that none of them are due to bugs in async-flow. For any of those, they should be fixed in this PR.
Security Considerations
nothing new
Scaling Considerations
none, given that total endowments are low cardinality. All these endowments are prepare-time per-function. There should not be any cardinality limit on the activations making use of these endowments. But like all other async-flow scaling issues, that remains to be tested.
Documentation Considerations
The endowment rules and taxonomy is interesting, and should be documented.
Testing Considerations
We get CI errors only from the
orchestrationpackage. Some of these may be the integration bugs we wanted this exercise to reveal. However, others may be async-flow bugs, which should have been caught by async-flow unit tests.The warning stopgap I mentioned above appears in CI as, for example
The relevant lines are
where the first line indicates what method or method guard provided the inappropriate promise
and the second line indicates where the guest code called it
Upgrade Considerations
The orchestration code in question cannot be truly upgrade safe until we see no more of these "vow expected, not promise" warnings. Even then, we should expect that async-flow as of this PR is ready for lots of testing, but not yet ready to run on the main chain with durable state expected to survive real upgrades.