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

convert fast-usdc contract to .ts #10480

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

convert fast-usdc contract to .ts #10480

wants to merge 6 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Nov 14, 2024

refs: #5760

Description

Convert some of the fast-usdc package to .ts. It also adds generics to mapStore() and makeDurablePublishKit().

I'd like to migrate to .ts incrementally because this package is in active development and if I were to migrate the whole thing now there would be lots of merge conflicts.

Security Considerations

As usual, readers must keep in mind that the types are advisory and be sure that adequate runtime validation is also in place.

Scaling Considerations

none

Documentation Considerations

Once this is working, we should convert the demo dapps to it to make them more accessible.

Testing Considerations

CI should cover it.

Upgrade Considerations

Should have no runtime impacts.

/** @type {PublishKit<CctpTxEvidence>} */
const { publisher, subscriber } = makeDurablePublishKit();
const { publisher, subscriber } =
makeDurablePublishKit() as PublishKit<CctpTxEvidence>;

Copy link
Member

Choose a reason for hiding this comment

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

Is this a substantive change in static type checking?

Does casting with as require assignment compatibility? I think @type does.

Copy link
Member

@dckc dckc Nov 14, 2024

Choose a reason for hiding this comment

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

Should it use makeDurablePublishKit<CctpTxEvidence>() instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Once that type is generic, yes

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 now

@@ -8,7 +8,7 @@ import type { Zone } from '@agoric/zone';
import type { VowTools } from '@agoric/vow';
import { prepareAdvancer } from '../../src/exos/advancer.js';
import { prepareStatusManager } from '../../src/exos/status-manager.js';
import { prepareTransactionFeedKit } from '../../src/exos/transaction-feed.js';
import { prepareTransactionFeedKit } from '../../src/exos/transaction-feed.ts';
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the build step does just a bit more than erase types.

I guess bundleSource groks, though

@@ -102,10 +101,9 @@ export const contract = async (zcf, privateArgs, zone, tools) => {
*
* Provide an API call in the form of an invitation maker, so that the
* capability is available in the smart-wallet bridge.
*
* @param {CctpTxEvidence} evidence
* @param evidence
Copy link
Member

Choose a reason for hiding this comment

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

Is this @param still necessary?
Perhaps only until we tell eslint otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. Our eslint config is that if there's a jsdoc comment on a function it must have params. I'd like to leave changing out that as out of scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

It’s not awful to have a place to document the meaning of the param for tsdoc. We just don’t have to note the type.

Copy link

cloudflare-workers-and-pages bot commented Nov 24, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2899c22
Status: ✅  Deploy successful!
Preview URL: https://d09c15c3.agoric-sdk.pages.dev
Branch Preview URL: https://ta-fu-ts.agoric-sdk.pages.dev

View logs

@turadg turadg changed the base branch from master to typescript-5.7 November 24, 2024 01:51
@turadg turadg changed the base branch from typescript-5.7 to master November 24, 2024 04:43
Copy link

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

@turadg turadg changed the title convert fast-usdc package to .ts convert fast-usdc contract to .ts Nov 24, 2024
@turadg turadg marked this pull request as ready for review November 25, 2024 01:22
@turadg turadg requested a review from a team as a code owner November 25, 2024 01:22
@turadg turadg requested a review from AgoricTriage November 25, 2024 01:22
}

export const prepareTransactionFeedKit = (zone: Zone, zcf: ZCF) => {
const kinds = zone.mapStore<string, unknown>('Kinds');
Copy link
Member

Choose a reason for hiding this comment

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

Why is this unknown?

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 just didn't know what to type it and this worked

makeOperatorInvitation(operatorId) {
makeOperatorInvitation(
operatorId: string,
): Promise<Invitation<OperatorKit>> {
Copy link
Member

Choose a reason for hiding this comment

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

Why type the return? It seems the same value is inferred

Copy link
Member Author

Choose a reason for hiding this comment

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

Just moved from what was there. I'll leave since it doesn't hurt to be explicit

@turadg
Copy link
Member Author

turadg commented Nov 25, 2024

I'll hold off until in-flight FU PRs are landed so this doesn't create merge conflicts for them

import { depositToSeat } from '@agoric/zoe/src/contractSupport/zoeHelpers.js';
import { E } from '@endo/far';
import { M, objectMap } from '@endo/patterns';
import type { Zone } from '@agoric/zone';
Copy link
Member

Choose a reason for hiding this comment

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

I was concerned that conversion to .ts would mean that type imports have to be changed from devDependencies to dependencies, but I don't see a change to package.json for @agoric/zone, so evidently not. Interesting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants