-
Notifications
You must be signed in to change notification settings - Fork 208
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
types(zoe): setTestJig param type optional #9533
Conversation
Deploying agoric-sdk with Cloudflare Pages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed the type and docs are correct by reviewing
agoric-sdk/packages/zoe/src/contractFacet/zcfZygote.js
Lines 358 to 363 in 37ec151
setTestJig: (testFn = () => ({})) => { | |
if (testJigSetter) { | |
testJigSetter({ ...testFn(), zcf }); | |
} | |
}, | |
getInstance: () => getInstanceRecHolder().getInstanceRecord().instance, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
5c8f836
to
426a3be
Compare
Staged on #9533 refs: #9281 ## Description The `zcf` object will effectively need to be passed through `orchestrate` as 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 if `zcf` were durable, we wouldn't need all this complexity. It turns out, if this PR is correct, that making `zcf` durable is trivial. ### Security Considerations Making `zcf` into a durable exo also involves giving it an interface guard. The interface guard in the first commit of this PR makes a needed exception for `makeInvitation` and `setTestJig` because both of them accept non-passable parameters. The `defaultGuards: 'passable'` option means that all other methods default to a guard that merely enforces that all arguments and return results are passable. This does make `zcf` somewhat more defensive, but not much. Given this starting point, we can grow that `ZcfI` interface 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 `ZcfI` interface 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 `setTestJig` better. ### Upgrade Considerations Making `zcf` durable 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 during `prepareExo` as state that its methods capture (close over) rather than as exo instance state. This reflects naturally the initial intuition that the `zcf` endowment, being stateless, could just be represented to `asyncFlow` as a singleton standin, re-endowed during the prepare phase.
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.
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.
Rebase todo: ``` # Branch fix-vow-make-watch-when-more-robust-against-loops-and-hangs-9487- label base-fix-vow-make-watch-when-more-robust-against-loops-and-hangs-9487- pick bcecf52 test(vow): add test of more vow upgrade scenarios pick d7135b2 test: switch vow test to run under xs for metering pick 99fccca test(vow): add test for resolving vow to external promise pick 6d3f88c test(vow): add test for vow based infinite vat ping pong pick c78ff0e test(vow): check vow consumers for busy loops or hangs pick 3c63cba fix(vow): prevent loops and hangs from watching promises pick 188c810 chore(vat-data): remove the deprecated `@agoric/vat-data/vow.js` pick 44a6d16 fix(vow): allow resolving vow to external promise label fix-vow-make-watch-when-more-robust-against-loops-and-hangs-9487- reset base-fix-vow-make-watch-when-more-robust-against-loops-and-hangs-9487- merge -C 4fca040 fix-vow-make-watch-when-more-robust-against-loops-and-hangs-9487- # fix(vow): make watch/when more robust against loops and hangs (#9487) # Branch ci-mergify-strip-merge-commit-HTML-comments-9499- label base-ci-mergify-strip-merge-commit-HTML-comments-9499- pick 63e21ab ci(mergify): strip merge commit HTML comments label ci-mergify-strip-merge-commit-HTML-comments-9499- reset base-ci-mergify-strip-merge-commit-HTML-comments-9499- merge -C 7b93671 ci-mergify-strip-merge-commit-HTML-comments-9499- # ci(mergify): strip merge commit HTML comments (#9499) # Pull request #9506 pick 249a5d4 fix(SwingSet): Undo deviceTools behavioral change from #9153 (#9506) # Pull request #9507 pick a19a964 fix(liveslots): promise watcher to cause unhandled rejection if no handler (#9507) # Branch feat-make-vat-localchain-resumable-9488- label base-feat-make-vat-localchain-resumable-9488- pick 76c17c6 feat: make vat-localchain resumable pick 40ccba1 fix(vow): correct the typing of `unwrap` pick 90e062c fix(localchain): work around TypeScript mapped tuple bug pick 3027adf fix(network): use new `ERef` and `FarRef` label feat-make-vat-localchain-resumable-9488- reset base-feat-make-vat-localchain-resumable-9488- merge -C 5856dc0 feat-make-vat-localchain-resumable-9488- # feat: make vat-localchain resumable (#9488) # Branch ci-mergify-clarify-queue-conditions-and-merge-conditions-9510- label base-ci-mergify-clarify-queue-conditions-and-merge-conditions-9510- pick 7247bd9 ci(mergify): clarify `queue_conditions` and `merge_conditions` label ci-mergify-clarify-queue-conditions-and-merge-conditions-9510- reset base-ci-mergify-clarify-queue-conditions-and-merge-conditions-9510- merge -C 30e56ae ci-mergify-clarify-queue-conditions-and-merge-conditions-9510- # ci(mergify): clarify `queue_conditions` and `merge_conditions` (#9510) # Branch fix-liveslots-cache-delete-does-not-return-a-useful-value-9509- label base-fix-liveslots-cache-delete-does-not-return-a-useful-value-9509- pick 42ea8a3 fix(liveslots): cache.delete() does not return a useful value label fix-liveslots-cache-delete-does-not-return-a-useful-value-9509- reset base-fix-liveslots-cache-delete-does-not-return-a-useful-value-9509- merge -C a2e54e1 fix-liveslots-cache-delete-does-not-return-a-useful-value-9509- # fix(liveslots): cache.delete() does not return a useful value (#9509) # Branch chore-swingset-re-enable-test-of-unrecognizable-orphan-cleanup-8694- label base-chore-swingset-re-enable-test-of-unrecognizable-orphan-cleanup-8694- pick 9930bd3 chore(swingset): re-enable test of unrecognizable orphan cleanup label chore-swingset-re-enable-test-of-unrecognizable-orphan-cleanup-8694- reset base-chore-swingset-re-enable-test-of-unrecognizable-orphan-cleanup-8694- merge -C bc53ef7 chore-swingset-re-enable-test-of-unrecognizable-orphan-cleanup-8694- # chore(swingset): re-enable test of unrecognizable orphan cleanup (#8694) # Pull request #9508 pick 513adc9 refactor(internal): move async helpers using AggregateError to node (#9508) # Branch report-bundle-sizing-in-agoric-run-9503- label base-report-bundle-sizing-in-agoric-run-9503- pick 68af59c refactor: inline addRunOptions pick a0115ed feat: writeCoreEval returns plan pick bd0edcb feat: stat-bundle and stat-plan scripts pick 0405202 feat: agoric run --verbose pick 22b43da feat(stat-bundle): show CLI to explode the bundle label report-bundle-sizing-in-agoric-run-9503- reset base-report-bundle-sizing-in-agoric-run-9503- merge -C 7b30169 report-bundle-sizing-in-agoric-run-9503- # report bundle sizing in agoric run (#9503) # Branch ci-test-boot-split-up-test-jobs-via-AVA-recipe-9511- label base-ci-test-boot-split-up-test-jobs-via-AVA-recipe-9511- pick 5f3c1d1 test(boot): use a single bundle directory for all tests pick 50229bd ci(all-packages): split tests according to AVA recipe label ci-test-boot-split-up-test-jobs-via-AVA-recipe-9511- reset base-ci-test-boot-split-up-test-jobs-via-AVA-recipe-9511- merge -C 5375019 ci-test-boot-split-up-test-jobs-via-AVA-recipe-9511- # ci(test-boot): split up test jobs via AVA recipe (#9511) # Pull request #9514 pick f908f89 fix: endow with original unstructured `assert` (#9514) # Pull request #9535 pick 989aa19 fix(swingset): log vat termination and upgrade better (#9535) # Branch types-zoe-setTestJig-param-type-optional-9533- label base-types-zoe-setTestJig-param-type-optional-9533- pick 426a3be types(zoe): setTestJig param type optional label types-zoe-setTestJig-param-type-optional-9533- reset base-types-zoe-setTestJig-param-type-optional-9533- merge -C bf9f03b types-zoe-setTestJig-param-type-optional-9533- # types(zoe): setTestJig param type optional (#9533) # Branch adopt-TypeScript-5-5-9476- label base-adopt-TypeScript-5-5-9476- pick 381b6a8 chore(deps): bump Typescript to 5.5 release label adopt-TypeScript-5-5-9476- reset base-adopt-TypeScript-5-5-9476- merge -C 0cfea88 adopt-TypeScript-5-5-9476- # adopt TypeScript 5.5 (#9476) # Branch retry-flaky-agoric-cli-integration-test-9550- label base-retry-flaky-agoric-cli-integration-test-9550- pick 2a68510 ci: retry agoric-cli integration test label retry-flaky-agoric-cli-integration-test-9550- reset base-retry-flaky-agoric-cli-integration-test-9550- merge -C c5c52ec retry-flaky-agoric-cli-integration-test-9550- # retry flaky agoric-cli integration test (#9550) # Pull Request #9556 pick 0af876f fix(vow): watcher args instead of context (#9556) # Pull Request #9561 pick a4f86eb fix(vow): handle resolution loops in vows (#9561) # Branch Restore-a3p-tests-9557- label base-Restore-a3p-tests-9557- pick d36382d chore(a3p): restore localchain test pick 5ff628e Revert "test: drop or clean-up old Tests" pick b5cf8bd fix(localchain): `callWhen`s return `PromiseVow` label Restore-a3p-tests-9557- reset base-Restore-a3p-tests-9557- merge -C c65915e Restore-a3p-tests-9557- # Restore a3p tests (#9557) # Pull Request #9559 pick 6073b2b fix(agoric): convey tx opts to `agoric wallet` and subcommands (#9559) # Branch explicit-heapVowTools-9548- label base-explicit-heapVowTools-9548- pick 100de68 feat!: export heapVowTools pick 8cb1ee7 refactor: use heapVowTools import pick 0ac6774 docs: vow vat utils pick 9128f27 feat: export heapVowE pick 3b0c8c1 refactor: use heapVowE pick 9b84bfa BREAKING CHANGE: drop V export pick 6623af5 chore(types): concessions to prepack label explicit-heapVowTools-9548- reset base-explicit-heapVowTools-9548- merge -C 4440ce1 explicit-heapVowTools-9548- # explicit heapVowTools (#9548) # Pull Request #9564 pick 44926a7 fix(bn-patch): fix bad html evasion (#9564) # Branch Fix-upgrade-behaviors-9526- label base-Fix-upgrade-behaviors-9526- pick ef1e0a2 feat: Upgrade Zoe pick e4cc97c Revert "fix(a3p-integration): workaround zoe issues" pick 84dd229 feat: repair KREAd contract on zoe upgrade pick cb77160 test: validate KREAd character purchase pick e1d961e test: move vault upgrade from test to use phase (#9536) label Fix-upgrade-behaviors-9526- reset base-Fix-upgrade-behaviors-9526- merge -C 8d05faf Fix-upgrade-behaviors-9526- # Fix upgrade behaviors (#9526) # Branch Support-for-snapshots-export-command-9563- label base-Support-for-snapshots-export-command-9563- pick 2a3976e refactor(cosmos): use DefaultBaseappOptions for newApp pick 84208e9 fix(cosmos): use dedicated dedicate app creator for non start commands pick 8c1a62d chore(cosmos): refactor cosmos command extension pick 4386f8e feat(cosmos): support snapshot export pick 2dabb52 test(a3p): add test for snapshots export and restore label Support-for-snapshots-export-command-9563- reset base-Support-for-snapshots-export-command-9563- merge -C 309c7e1 Support-for-snapshots-export-command-9563- # Support for `snapshots export` command (#9563) # Branch Swing-store-export-data-outside-of-genesis-file-9549- label base-Swing-store-export-data-outside-of-genesis-file-9549- pick f1eacbe fix(x/swingset): handle defer errors on export write pick 496a430 feat(cosmos): add hooking kv reader pick f476bd5 feat(cosmos): separate swing-store export data from genesis file pick 17a5374 test(a3p): add genesis fork acceptance test label Swing-store-export-data-outside-of-genesis-file-9549- reset base-Swing-store-export-data-outside-of-genesis-file-9549- merge -C 3aa5d66 Swing-store-export-data-outside-of-genesis-file-9549- # Swing-store export data outside of genesis file (#9549) # Branch Remove-scaled-price-authority-upgrade-9585- label base-Remove-scaled-price-authority-upgrade-9585- pick bce49e3 test: add test during upgradeVaults; vaults detect new prices pick 88f6500 test: repair test by dropping upgrade of scaledPriceAuthorities label Remove-scaled-price-authority-upgrade-9585- reset base-Remove-scaled-price-authority-upgrade-9585- merge -C 8376991 Remove-scaled-price-authority-upgrade-9585- # Remove scaled price authority upgrade (#9585) # Branch feat-make-software-upgrade-coreProposals-conditional-on-upgrade-plan-name-9575- label base-feat-make-software-upgrade-coreProposals-conditional-on-upgrade-plan-name-9575- pick 95174a2 feat(builders): non-ambient `strictPriceFeedProposalBuilder` in `priceFeedSupport.js` pick 5cc190d feat(app): establish mechanism for adding core proposals by `upgradePlan.name` pick 52f02b7 fix(builders): use proper `oracleBrand` subkey case pick ddc072d chore(cosmos): extract `app/upgrade.go` pick b3182a4 chore: fix error handling of upgrade vaults proposal pick ea568a2 fix: retry upgrade vaults price quote label feat-make-software-upgrade-coreProposals-conditional-on-upgrade-plan-name-9575- reset base-feat-make-software-upgrade-coreProposals-conditional-on-upgrade-plan-name-9575- merge -C cbe061c feat-make-software-upgrade-coreProposals-conditional-on-upgrade-plan-name-9575- # feat: make software upgrade `coreProposals` conditional on upgrade plan name (#9575) ``` This is followed by a commit to remove the `orchestration` and `async-flow` packages from the release, as these are not baked in yet and are not deployed anyway.
closes: #XXXX
refs: #9531 (comment)
Description
The implementation of
setTestJig
atagoric-sdk/packages/zoe/src/contractFacet/zcfZygote.js
Line 358 in 37ec151
setTestJig
at https://github.com/Agoric/documentation/blob/89a7dd53cd59b4008a36d23abdfa5665d1852336/snippets/tools/zcfTesterContract.js#L12 omits its parameter. The doc-comment on the type atagoric-sdk/packages/zoe/src/contractFacet/types-ambient.d.ts
Line 139 in 37ec151
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 atagoric-sdk/packages/zoe/src/contractFacet/types-ambient.d.ts
Line 139 in 37ec151
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.