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.

Sign up for GitHub

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

Jump to bottom

static types improvements #6256

Closed
wants to merge 13 commits into from
Closed

static types improvements #6256

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4169857
chore: ts-check types.js
turadg Sep 16, 2022
52aa261
chore: fix types
turadg Sep 16, 2022
cf27bc4
chore: parameterize offer()
turadg Sep 17, 2022
c46e789
chore: fix offer/invitation types
turadg Sep 17, 2022
ab55fb8
chore: Invitation type result first
turadg Sep 17, 2022
89ee478
chore(types): never allow offer args to arg-less invitations
turadg Sep 17, 2022
cafb4e9
chore: ts-check most files that have typedefs
turadg Sep 17, 2022
5349bf5
refactor(types): improvements
turadg Sep 17, 2022
34c702b
refactor(solo): runtime validation
turadg Sep 20, 2022
428b274
feat(ertp): types module
turadg Sep 20, 2022
7e677cd
chore(types): maxNodeModuleJsDepth=0 and disable ambient
turadg Sep 20, 2022
503219b
chore(types): maxNodeModuleJsDepth=0 and disable ambient
turadg Sep 20, 2022
2057268
chore(agoric-cli): maxNodeModuleJsDepth=0 and disable ambient types
turadg Sep 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/ERTP/src/amountMath.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,12 +176,13 @@ const checkLRAndGetHelpers = (leftAmount, rightAmount, brand = undefined) => {

/**
* @template {AssetKind} K
* @param {MathHelpers<AssetKind>} h
* @param {MathHelpers<AssetValueForKind<K>>} h
* @param {Amount<K>} leftAmount
* @param {Amount<K>} rightAmount
* @returns {[K, K]}
*/
const coerceLR = (h, leftAmount, rightAmount) => {
// @ts-expect-error could be arbitrary subtype
return [h.doCoerce(leftAmount.value), h.doCoerce(rightAmount.value)];
};

Expand Down Expand Up @@ -243,9 +244,9 @@ const AmountMath = {
* Return the amount representing an empty amount. This is the
* identity element for MathHelpers.add and MatHelpers.subtract.
*
* @template {AssetKind} K
* @template {AssetKind} [K='nat']
* @param {Brand<K>} brand
* @param {K} [assetKind]
* @param {K} [assetKind='nat']
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense for both line 247 and 249 to specify defaults?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so. One is JSDoc telling the consumer what the default is and the other the runtime making it so.

Copy link
Contributor

Choose a reason for hiding this comment

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

The runtime making it happen is on line 254. As I read it (I'm admittedly not the expert here) line 247 says that the AssetKind type doesn't require that its parameter be specified, and if it's omitted from the types, it'll assume 'nat'. Line 249 says that the call to makeEmpty doesn't require a second arg, with the same default. Don't those have the same impact?

If a call appears without the second arg, both typescript and javascript should use 'nat'. If the arg is provided, both should draw the same conclusion. If there's an explicit declaration that conflicts with the code, TS should complain. Won't all the same things happen if line 247 has the default and 249 doesn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I misread the discussion as being about line 254 (the runtime default).

You're right, 249 shouldn't have a default. I'll fix in this in #6287

* @returns {Amount<K>}
*/
// @ts-expect-error TS/jsdoc things 'nat' can't be assigned to K subclassing AssetKind
Expand All @@ -254,6 +255,7 @@ const AmountMath = {
assertRemotable(brand, 'brand');
assertAssetKind(assetKind);
const value = helpers[assetKind].doMakeEmpty();
// @ts-expect-error TS/jsdoc things 'nat' can't be assigned to K subclassing AssetKind
return harden({ brand, value });
},
/**
Expand Down
6 changes: 3 additions & 3 deletions packages/ERTP/src/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

/**
* @typedef {import('@endo/marshal').InterfaceSpec} InterfaceSpec
* @typedef {import('@endo/marshal').GetInterfaceOf} GetInterfaceOf
* @typedef {import('@endo/marshal').MarshalGetInterfaceOf} GetInterfaceOf
*/

/**
Expand Down Expand Up @@ -410,8 +410,8 @@
*/

/**
* @template {AssetKind} K
* @typedef {object} MathHelpers<K>
* @template {AmountValue} V
* @typedef {object} MathHelpers
* All of the difference in how digital asset amount are manipulated can be
* reduced to the behavior of the math on values. We extract this
* custom logic into mathHelpers. MathHelpers are about value
Expand Down
2 changes: 2 additions & 0 deletions packages/inter-protocol/src/psm/psm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,3 +328,5 @@ export const start = async (zcf, privateArgs, baggage) => {
publicFacet,
});
};

/** @typedef {Awaited<ReturnType<typeof start>>['publicFacet']} PsmPublicFacet */
1 change: 0 additions & 1 deletion packages/inter-protocol/src/psm/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
// @ts-check
Copy link
Member

Choose a reason for hiding this comment

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

backtrack previous commit?

/** @typedef {import('./psm').PSM} PSM */
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/vaultFactory/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@

/**
* @typedef {object} Liquidator
* @property {() => Promise<Invitation<{ debt: Amount<'nat'> }, void>>} makeLiquidateInvitation
* @property {() => Promise<Invitation<{ debt: Amount<'nat'>; penaltyRate: Ratio; }, void>>} makeLiquidateInvitation
*/

/**
Expand Down
1 change: 1 addition & 0 deletions packages/inter-protocol/src/vpool-xyk-amm/addPool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ export const makeAddPoolInvitation = (
);
const minLiqAmount = AmountMath.make(
liquidityBrand,
// @ts-expect-error known nat
minPoolLiquidity.value,
);

Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/vpool-xyk-amm/multipoolMarketMaker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const trace = makeTracer('XykAmm', false);
* @typedef {Readonly<{
* zcf: ZCF<AmmTerms>,
* secondaryBrandToPool: WeakMapStore<Brand,PoolFacets>,
* secondaryBrandToLiquidityMint: WeakMapStore<Brand,ZCFMint>,
* secondaryBrandToLiquidityMint: WeakMapStore<Brand,ZCFMint<'nat'>>,
* centralBrand: Brand,
* timer: TimerService,
* quoteIssuerKit: IssuerKit,
Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/vpool-xyk-amm/params.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const DEFAULT_PROTOCOL_FEE_BP = 6n;
* @param {Invitation} electorateInvitation - invitation for the question poser
* @param {bigint} protocolFeeBP
* @param {bigint} poolFeeBP
* @param {Amount} minInitialLiquidity
* @param {Amount<'nat'>} minInitialLiquidity
*/
const makeAmmParams = (
electorateInvitation,
Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/vpool-xyk-amm/pool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export const definePoolKind = (baggage, ammPowers, storageNode, marshaller) => {
);

/**
* @param {ZCFMint} liquidityZcfMint
* @param {ZCFMint<'nat'>} liquidityZcfMint
* @param {ZCFSeat} poolSeat
* @param {Brand} secondaryBrand
* @returns {ImmutableState & MutableState}
Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/test/psm/setupPsm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ export const setupPsm = async (
};
const governedInstance = E(governorPublicFacet).getGovernedContract();

/** @type { GovernedPublicFacet<PSM> } */
/** @type { GovernedPublicFacet<import('../../src/psm/psm.js').PsmPublicFacet> } */
const psmPublicFacet = await E(governorCreatorFacet).getPublicFacet();
const psm = {
psmCreatorFacet: psmFacets.psmCreatorFacet,
Expand Down
19 changes: 11 additions & 8 deletions packages/zoe/src/contractFacet/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
* @property {(issuer: Issuer) => Brand} getBrandForIssuer
* @property {(brand: Brand) => Issuer} getIssuerForBrand
* @property {GetAssetKindByBrand} getAssetKind
* @property {<K extends AssetKind>(keyword: Keyword, assetKind?: K, displayInfo?: AdditionalDisplayInfo) => Promise<ZCFMint<K>>} makeZCFMint
* @property {<K extends AssetKind = 'nat'>(keyword: Keyword, assetKind?: K, displayInfo?: AdditionalDisplayInfo) => Promise<ZCFMint<K>>} makeZCFMint
* @property {ZCFRegisterFeeMint} registerFeeMint
* @property {ZCFMakeEmptySeatKit} makeEmptySeatKit
* @property {SetTestJig} setTestJig
Expand Down Expand Up @@ -103,15 +103,15 @@
* @param {string} description
* @param {object=} customProperties
* @param {Pattern} [proposalShape]
* @returns {Promise<Invitation<OR>>}
* @returns {Promise<Invitation>}
*/

/**
* @callback ZCFRegisterFeeMint
* @param {Keyword} keyword
* @param {FeeMintAccess} allegedFeeMintAccess - an object that
* purports to be the object that grants access to the fee mint
* @returns {Promise<ZCFMint>
* @returns {Promise<ZCFMint<'nat'>>}
*/

/**
Expand Down Expand Up @@ -219,12 +219,15 @@

/**
* API for a contract start function.
* FIXME before merge: assumes synchronous
*
* @template {object} [PF] Public facet
* @template {object} [CF] Creator facet
* @template {object} [CT] Custom terms
* @template {object} [PA] Private args
* CAVEAT: assumes synchronous
*
* @deprecated define function signature directly
*
* @template {object} [PF=any] Public facet
* @template {object} [CF=any] Creator facet
* @template {object} [CT=any] Custom terms
* @template {object} [PA=any] Private args
* @callback ContractStartFn
* @param {ZCF<CT>} zcf
* @param {PA} privateArgs
Expand Down
2 changes: 2 additions & 0 deletions packages/zoe/src/contractFacet/zcfZygote.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ export const makeZCFZygote = async (
assetKind,
displayInfo,
);
// @ts-expect-error could be instantiated with a different subtype
return zcfMintFactory.makeZCFMintInternal(keyword, zoeMint);
};

Expand All @@ -161,6 +162,7 @@ export const makeZCFZygote = async (
keyword,
feeMintAccess,
);
// @ts-expect-error could be instantiated with a different subtype
return zcfMintFactory.makeZCFMintInternal(keyword, zoeMint);
};

Expand Down
3 changes: 2 additions & 1 deletion packages/zoe/src/contracts/otcDesk.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const start = zcf => {
* @returns {Promise<Payment>}
*/
const makeQuote = async (price, assets, timeAuthority, deadline) => {
/** @type {{ creatorInvitation: Invitation<unknown, Payment>} } */
/** @type {{ creatorInvitation: Invitation<{}, Payment>} } */
const { creatorInvitation } = await E(zoe).startInstance(
coveredCallInstallation,
zcf.getTerms().issuers,
Expand All @@ -84,6 +84,7 @@ const start = zcf => {
},
});

/** @type {{ userSeatPromise: Promise<UserSeat<Payment>>}} */
const { userSeatPromise: coveredCallUserSeat } = await offerTo(
zcf,
creatorInvitation,
Expand Down
10 changes: 5 additions & 5 deletions packages/zoe/src/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@
* @property {Installation} installation
* @property {Instance} instance
* @property {AnyTerms} terms - contract parameters

*
*/

/**
* @template {AssetKind} [K=AssetKind]
* @typedef {object} IssuerRecord
* @property {Brand<K>} brand
Expand All @@ -40,11 +41,10 @@
* @property {any} [displayInfo]
*
* @typedef {AmountKeywordRecord} Allocation
* @typedef {Record<Keyword,AmountMath>} AmountMathKeywordRecord
*/

/**
* @template [OfferArgs]
* @template [OfferResult]
* @template {object} [A=any] Offer args
Chris-Hibbert marked this conversation as resolved.
Show resolved Hide resolved
* @template {object} [R=unknown] Offer result
* @typedef {Payment<'set'>} Invitation
*/
9 changes: 5 additions & 4 deletions packages/zoe/src/zoeService/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,12 +152,13 @@
* Verify that an alleged Invitation is real, and return the Bundle ID it
* will use for contract code.
*
* @param {ERef<Installation>}
* @param {ERef<Installation>} allegedInstallation
* @returns {Promise<BundleID>}
*/

/**
* @template {object} [OR=any]
* @template {object} [OA=any] offer args
* @template {object} [OR=any] offer result
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be Args and Result rather than OA and OR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes of course. Do you think we should adopt that style? So far we've been using all caps to denote a generic type. Usually one letter. I'm more inclined to A for args and R for results.

If we want to change to words, I'd prefer that be done separately in a repo-wide change.

Copy link
Contributor

Choose a reason for hiding this comment

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

OA and OR seem strictly worse than either alternative. When a single letter is used, I much prefer to also have some part of the commentary indicate what it's mnemonic for, as is the case here, but I think I saw cases where that was dropped. Do some of the formats for specifying typescript not allow room for comments on individual parameters or template values? If that's the case, I'd prefer clearer names.

It's often the case that the role of the templated parameters is opaque from just the type declarations. If I have to specify them for offers, invitations, or start functions, it's really helpful to get an indication of what the missing declaration is for.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the reason generics are usually initialisms is to distinguish from types, which are StudlyCaps.

I take the point that it's helpful to have the parameter described, and when using the TS syntax there's no documentation string. So I agree when there are multiple type slots and there isn't a documentation string that we should use the more descriptive name, even it it might be confused with a typedef.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #6287

* @callback Offer
*
* To redeem an invitation, the user normally provides a proposal (their
Expand All @@ -174,10 +175,10 @@
* values are the actual payments to be escrowed. A payment is
* expected for every rule under `give`.
*
* @param {ERef<Invitation<OR>>} invitation
* @param {ERef<Invitation<OA, OR>>} invitation
* @param {Proposal=} proposal
* @param {PaymentPKeywordRecord=} paymentKeywordRecord
* @param {object=} offerArgs
* @param {OA=} offerArgs
* @returns {Promise<UserSeat<OR>>} seat
*/

Expand Down
2 changes: 1 addition & 1 deletion packages/zoe/test/unitTests/zcf/test-zcf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ test(`zcfSeat.isOfferSafe from zcf.makeEmptySeatKit`, async t => {
* @param {Keyword} zcfMintKeyword
* @param {ZCFSeat} zcfSeat
* @param {Keyword} gainsKeyword
* @param {AmountValue} gainsValue
* @param {bigint} gainsValue
Chris-Hibbert marked this conversation as resolved.
Show resolved Hide resolved
* @returns {Promise<IssuerRecord>}
*/
const allocateEasy = async (
Expand Down