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

import ERTP types #104

Merged
merged 9 commits into from
Apr 17, 2024
Merged

import ERTP types #104

merged 9 commits into from
Apr 17, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Apr 16, 2024

closes: #103

Description

Converts packages/web-component to .ts so it can import that actual ERTP types

Security Considerations

n/a, types

Scaling Considerations

n/a, types

Documentation Considerations

none

Testing Considerations

CI suffices

Copy link
Contributor

@samsiegart samsiegart left a comment

Choose a reason for hiding this comment

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

Looks good, just some non-blocking comments/questions. I'm going to try generating the typedocs and testing e2e with dapp-offer-up later to make sure everything still works.

@@ -260,8 +243,10 @@ export const watchWallet = (

for await (const update of iterateEach(follower)) {
console.debug('wallet update', update);
// @ts-expect-error xxx
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, what was the error here? Maybe we can provide a type for the follower, or iterateEach<>? Or casting doesn't support it well enough..

@@ -100,6 +82,7 @@ export const watchWallet = (
smartWalletStatusNotifierKit.updater.updateState(
harden({ provisioned: true }),
);
// @ts-expect-error xxx
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be possible to provide a type to chainStorageWatcher.watchLatest to resolve this

/** @type { import('@agoric/smart-wallet/src/smartWallet.js').CurrentWalletRecord['offerToPublicSubscriberPaths'] | null } */ (
null
),
/** @type { import('@agoric/smart-wallet/src/smartWallet.js').CurrentWalletRecord['offerToPublicSubscriberPaths'] | null } */ null,
);

const walletUpdatesNotifierKit = makeNotifierKit(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we provide a type arg to makeNotifierKit instead of casting here?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was just a prettier change. I forgot to convert from jsdoc so TS isn't even seeing it. I'll fix

* @param {string} networkConfigHref
* @param {string=} caption
* @param networkConfigHref
* @param [caption]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these doc comments necessary now that we have type params?

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 jsdoc lint plugin is enforcing it when there's any jsdoc comment. I'd be fine disabling that rule. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

second thought, I think that would risk defining some of the params by jsdoc and not others.

@@ -45,11 +30,14 @@ const zeroFee = () => {

/**
* Use a signing client to
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this comment was previously messed up. Should be something like "Use a signing client to create a signer for Agoric-specific transactions"

@samsiegart
Copy link
Contributor

I fixed some build errors and typedocs here: #106

Tested with dapp-offer-up and seems to work.

@turadg
Copy link
Member Author

turadg commented Apr 17, 2024

I fixed some build errors and typedocs here: #106

thanks, I cherry-picked that into this. (I didn't merge because my local branch had already moved from the remote)

@turadg turadg requested a review from samsiegart April 17, 2024 16:36
Copy link
Contributor

@samsiegart samsiegart left a comment

Choose a reason for hiding this comment

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

LGTM assuming you can fix lint/tests

@turadg turadg merged commit 6ccbd0f into main Apr 17, 2024
1 check passed
@turadg turadg deleted the 103-import-ERTP branch April 17, 2024 19:59
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.

JSDoc redefines Amount, Brand, ...
2 participants