feat(zoe): Make zcf singleton durable #9531
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
Deploying agoric-sdk with
|
| Latest commit: |
ecd1c75
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://041634c6.agoric-sdk.pages.dev |
| Branch Preview URL: | https://markm-exofy-zcf.agoric-sdk.pages.dev |
turadg
approved these changes
Jun 19, 2024
There was a problem hiding this comment.
LGTM.
For testing, I think our existing ZCF upgrade tests cover it
agoric-sdk/golang/cosmos/app/app.go
Line 938 in e4a4693
However, that's part of current upgrade handler, so once this is in master @iomekam will need to be sure not to include in the u16 release branch. (I expect u16 is close enough to release that we shouldn't delay it with this; and it's easy to include in the next core-eval)
Chris-Hibbert
approved these changes
Jun 19, 2024
erights
force-pushed
the
markm-setTestJig-param-optional
branch
from
5c8f836
to
426a3be
Compare
turadg
added
automerge:rebase
mergify bot
added a commit
that referenced
this pull request
Jun 19, 2024
closes: #XXXX refs: #9531 (comment) ## Description The implementation of `setTestJig` at https://github.com/Agoric/agoric-sdk/blob/37ec151b08de3d8e432a2599ccc532d4e72caedb/packages/zoe/src/contractFacet/zcfZygote.js#L358 treats its param as optional. The call to `setTestJig` at https://github.com/Agoric/documentation/blob/89a7dd53cd59b4008a36d23abdfa5665d1852336/snippets/tools/zcfTesterContract.js#L12 omits its parameter. The doc-comment on the type at https://github.com/Agoric/agoric-sdk/blob/37ec151b08de3d8e432a2599ccc532d4e72caedb/packages/zoe/src/contractFacet/types-ambient.d.ts#L139 explains the parameter as optional. But the type itself declares the parameter as mandatory. This initially led me at #9531 to declare the parameter as mandatory in the new `ZcfI` interface guard, but that caused the failure in the documentation repo discussed at https://github.com/Agoric/agoric-sdk/blob/37ec151b08de3d8e432a2599ccc532d4e72caedb/packages/zoe/src/contractFacet/types-ambient.d.ts#L139 . My comment in the code there and the subsequent discussion assumes that the usage at the documentation repo is what needs to be fixed. But given this other evidence, I think the static type needs to be fixed to type that parameter as optional. #9531 would then be ok as is, only needing removal of the comment indicating something is amiss. ### Security Considerations There is an existing security concern with the existence of `setTestJig` at all. But this PR does not affect that security concern at all. ### Scaling Considerations none ### Documentation Considerations This PR would make the `setTestJig` call currently in the documentation repo correct. ### Testing Considerations This problem was initially detected when testing #9531 when the guard declared the parameter as mandatory. This does reenforce the lesson that TS types are unsound by enforced guards are sound. ### Upgrade Considerations This PR is only a static change consistent with all current usage and implementation, and so should have no upgrade considerations. However, just to minimize risk, it still makes sense to hold this back till after master is snapshot for u16.
|
@Mergifyio requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
mhofman
pushed a commit
that referenced
this pull request
Jun 20, 2024
closes: #XXXX refs: #9531 (comment) ## Description The implementation of `setTestJig` at https://github.com/Agoric/agoric-sdk/blob/37ec151b08de3d8e432a2599ccc532d4e72caedb/packages/zoe/src/contractFacet/zcfZygote.js#L358 treats its param as optional. The call to `setTestJig` at https://github.com/Agoric/documentation/blob/89a7dd53cd59b4008a36d23abdfa5665d1852336/snippets/tools/zcfTesterContract.js#L12 omits its parameter. The doc-comment on the type at https://github.com/Agoric/agoric-sdk/blob/37ec151b08de3d8e432a2599ccc532d4e72caedb/packages/zoe/src/contractFacet/types-ambient.d.ts#L139 explains the parameter as optional. But the type itself declares the parameter as mandatory. This initially led me at #9531 to declare the parameter as mandatory in the new `ZcfI` interface guard, but that caused the failure in the documentation repo discussed at https://github.com/Agoric/agoric-sdk/blob/37ec151b08de3d8e432a2599ccc532d4e72caedb/packages/zoe/src/contractFacet/types-ambient.d.ts#L139 . My comment in the code there and the subsequent discussion assumes that the usage at the documentation repo is what needs to be fixed. But given this other evidence, I think the static type needs to be fixed to type that parameter as optional. #9531 would then be ok as is, only needing removal of the comment indicating something is amiss. ### Security Considerations There is an existing security concern with the existence of `setTestJig` at all. But this PR does not affect that security concern at all. ### Scaling Considerations none ### Documentation Considerations This PR would make the `setTestJig` call currently in the documentation repo correct. ### Testing Considerations This problem was initially detected when testing #9531 when the guard declared the parameter as mandatory. This does reenforce the lesson that TS types are unsound by enforced guards are sound. ### Upgrade Considerations This PR is only a static change consistent with all current usage and implementation, and so should have no upgrade considerations. However, just to minimize risk, it still makes sense to hold this back till after master is snapshot for u16.
mhofman
pushed a commit
that referenced
this pull request
Jun 22, 2024
closes: #XXXX refs: #9531 (comment) ## Description The implementation of `setTestJig` at https://github.com/Agoric/agoric-sdk/blob/37ec151b08de3d8e432a2599ccc532d4e72caedb/packages/zoe/src/contractFacet/zcfZygote.js#L358 treats its param as optional. The call to `setTestJig` at https://github.com/Agoric/documentation/blob/89a7dd53cd59b4008a36d23abdfa5665d1852336/snippets/tools/zcfTesterContract.js#L12 omits its parameter. The doc-comment on the type at https://github.com/Agoric/agoric-sdk/blob/37ec151b08de3d8e432a2599ccc532d4e72caedb/packages/zoe/src/contractFacet/types-ambient.d.ts#L139 explains the parameter as optional. But the type itself declares the parameter as mandatory. This initially led me at #9531 to declare the parameter as mandatory in the new `ZcfI` interface guard, but that caused the failure in the documentation repo discussed at https://github.com/Agoric/agoric-sdk/blob/37ec151b08de3d8e432a2599ccc532d4e72caedb/packages/zoe/src/contractFacet/types-ambient.d.ts#L139 . My comment in the code there and the subsequent discussion assumes that the usage at the documentation repo is what needs to be fixed. But given this other evidence, I think the static type needs to be fixed to type that parameter as optional. #9531 would then be ok as is, only needing removal of the comment indicating something is amiss. ### Security Considerations There is an existing security concern with the existence of `setTestJig` at all. But this PR does not affect that security concern at all. ### Scaling Considerations none ### Documentation Considerations This PR would make the `setTestJig` call currently in the documentation repo correct. ### Testing Considerations This problem was initially detected when testing #9531 when the guard declared the parameter as mandatory. This does reenforce the lesson that TS types are unsound by enforced guards are sound. ### Upgrade Considerations This PR is only a static change consistent with all current usage and implementation, and so should have no upgrade considerations. However, just to minimize risk, it still makes sense to hold this back till after master is snapshot for u16.
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.
Staged on #9533
refs: #9281
Description
The
zcfobject will effectively need to be passed throughorchestrateas an endowment. Because zcf is not durable, or even an exo, we were originally planning to do it with a mechanism involving a standing durable object, and then wrap and unwrap it on either side of the membrane. But ifzcfwere durable, we wouldn't need all this complexity. It turns out, if this PR is correct, that makingzcfdurable is trivial.Security Considerations
Making
zcfinto a durable exo also involves giving it an interface guard. The interface guard in the first commit of this PR makes a needed exception formakeInvitationandsetTestJigbecause both of them accept non-passable parameters. ThedefaultGuards: 'passable'option means that all other methods default to a guard that merely enforces that all arguments and return results are passable. This does makezcfsomewhat more defensive, but not much.Given this starting point, we can grow that
ZcfIinterface guard to do more explicit input validation of the other methods, which will help security, and make us less vulnerable to insufficient input validation in the zcf methods themselves. As we move more of the input validation to the method guards, we should be able to remove ad hoc input validation code in the method which has become redundant. Replacement of ad hoc input validation with declarative guard-based input validation should help security.I don't yet know whether I'll grow the
ZcfIinterface guard to have these explicit method guards in further commits to this PR or in later PR.Scaling Considerations
The extra guard checks are potentially an issue, but we won't know until we profile.
Documentation Considerations
none
Testing Considerations
I need to understand
setTestJigbetter.Upgrade Considerations
Making
zcfdurable means that it has a durable identity that survives upgrade. As a durable exo singleton, it is stateless, meaning that it gets back all the state it needs duringprepareExoas state that its methods capture (close over) rather than as exo instance state. This reflects naturally the initial intuition that thezcfendowment, being stateless, could just be represented toasyncFlowas a singleton standin, re-endowed during the prepare phase.