Skip to content

Commit

Permalink
types(zoe): setTestJig param type optional (#9533)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mergify[bot] authored Jun 19, 2024
2 parents 9274bc7 + 426a3be commit bf9f03b
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions packages/zoe/src/contractFacet/types-ambient.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,12 @@ type ZCFRegisterFeeMint = (
* never in production; i.e., it is only called if `testJigSetter`
* was supplied.
*
* If no, `testFn` is supplied, then an empty jig will be used.
* If no `testFn` is supplied, then an empty jig will be used.
* An additional `zcf` property set to the current ContractFacet
* will be appended to the returned jig object (overriding any
* provided by the `testFn`).
*/
type SetTestJig = (testFn: () => Record<string, unknown>) => void;
type SetTestJig = (testFn?: () => Record<string, unknown>) => void;
type ZCFMint<K extends AssetKind = AssetKind> = {
getIssuerRecord: () => IssuerRecord<K>;
/**
Expand Down

0 comments on commit bf9f03b

Please sign in to comment.