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.

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 type improvements to contracts #6287

Merged
merged 11 commits into from
Sep 21, 2022
Merged

static type improvements to contracts #6287

merged 11 commits into from
Sep 21, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Sep 20, 2022

Pulled out of #6256 , to punt on ERTP package exports.

Description

Various commits to increase type coverage.

One new types feature is that the Installation type carries through to offer args validation and offer results, so we no longer need to cast (or guess) those.

Security Considerations

n/a, static types

Documentation Considerations

--

Testing Considerations

Types check

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// @ts-expect-error TS/jsdoc things 'nat' can't be assigned to K subclassing AssetKind
// @ts-expect-error TS/jsdoc thinks 'nat' can't be assigned to K subclassing AssetKind

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 thought I fixed this but I see it's not pushed. When you approve I'll do a rebase to fixup the commit this was in and force push. (Holding off on that so I don't make it harder to review)

Comment on lines +4 to +7
export type Brand = unknown;
export type Amount = { brand: Brand; value: bigint };
export type Instance = unknown;
export type Proposal = any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these being left untyped?

Copy link
Member Author

Choose a reason for hiding this comment

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

Explained in a comment

Copy link
Contributor

Choose a reason for hiding this comment

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

you mean the line above that says

types that are only provided ambiently have to be defined.

Why is it better to declare them as unknown or any than to copy their actual types? Does the CLI not rely on their values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does the CLI not rely on their values?

Well the runtime doesn't rely on these typedefs at all. This package doesn't export anything to consumes so the types are there for dx productivity. The trade-off here is that there's more productivity to be gained by leaving them as is (vs spending more time on this).

Why is it better to declare them as unknown or any than to copy their actual types?

The runtime types also don't match because the CLI gets its values from RPC and doesn't have CapTP to translate them into regular objects. That's what the xxx RpcRemote comments are about.

Comment on lines 14 to 15
// FIXME import InterfaceGuard from @agoric/store
/** @typedef {*} InterfaceGuard */
Copy link
Contributor

Choose a reason for hiding this comment

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

why not do the import now?

Copy link
Member Author

Choose a reason for hiding this comment

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

The type isn't available. Documented in 17aea73

@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Sep 21, 2022
@mergify mergify bot merged commit 59139b4 into master Sep 21, 2022
@mergify mergify bot deleted the ta/contract-types branch September 21, 2022 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants