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

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 all commits
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
Loading