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

export withOrchestration #9808

Closed
wants to merge 5 commits into from
Closed

export withOrchestration #9808

wants to merge 5 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Jul 30, 2024

refs: #9757
refs: #9765

Description

I noticed that withOrchestration isn't exported from the package. This exports it. It also exports the OrchestrationTools that the docs for that type require. I tried to not export provideOrchestration because it's more of an implementation detail, but JSDoc type expressions make it hard to export OrchestrationTools without also exporting provideOrchestration. I also considered getting rid of it but tests need to be able to set up OrchestrationTools without spinning up a contract. So I marked provideOrchestration as @internal so it doesn't show up in docs.

This also proposes a solution to,

Before

CosmosInterchainService: ReturnType<[MakeCosmosInterchainService

After

interface CosmosInterchainService {
    [PASS_STYLE]: "remotable";
    [toStringTag]: string;
    __getInterfaceGuard__?: (() => undefined | InterfaceGuard<{
        makeAccount: any;
        provideICQConnection: any;
    }>);
    makeAccount(chainId, hostConnectionId, controllerConnectionId): Vow<IcaAccount>;
    provideICQConnection(controllerConnectionId): Guarded<{
        getLocalAddress(): `/ibc-port/${string}`;
        getRemoteAddress(): `/${string}ibc-port/${string}/ordered/${string}` | `/${string}ibc-port/${string}/unordered/${string}`;
        query(msgs): Promise<JsonSafe<ResponseQuery>[]>;
    }> | Vow<Guarded<{
        getLocalAddress(): `/ibc-port/${string}`;
        getRemoteAddress(): `/${string}ibc-port/${string}/ordered/${string}` | `/${string}ibc-port/${string}/unordered/${string}`;
        query(msgs): Promise<JsonSafe<ResponseQuery>[]>;
    }>>;
}

If that is agreeable I can convert the others in this PR or follow up.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

  • provideOrchestration not in Markdown docs build for docs site
  • withOrchestration signature is clear and concise

Upgrade Considerations

@turadg turadg requested review from dckc and 0xpatrickdev July 30, 2024 20:47
@dckc
Copy link
Member

dckc commented Jul 30, 2024

I'd like to review this by looking at a cloudflare preview. Any idea why it's not here?

It was there on at least one recent PR: #9798 (comment)

@0xpatrickdev
Copy link
Member

I'd like to review this by looking at a cloudflare preview. Any idea why it's not here?

It was there on at least one recent PR: #9798 (comment)

Good idea. Clicking into Cloudflare Pages job I see:

Preview URL: https://d7d9f03e.agoric-sdk.pages.dev
Branch Preview URL: https://ta-export-withorchestration.agoric-sdk.pages.dev

Copy link
Member

@0xpatrickdev 0xpatrickdev left a comment

Choose a reason for hiding this comment

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

/** @interface */

Nice finding!

Tangential to these changes, but interfaces/_agoric_orchestration.Orchestrator is virtually unreadable with the KnownChains generic

@dckc
Copy link
Member

dckc commented Jul 31, 2024

... Orchestrator is virtually unreadable with the KnownChains generic

I noted that as a task in...

@dckc
Copy link
Member

dckc commented Jul 31, 2024

bleah. the preview suffers from...

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

this all looks like forward progress
in particular, https://ta-export-withorchestration.agoric-sdk.pages.dev/interfaces/_agoric_orchestration.CosmosInterchainService looks better.

some parts might not be working as well as expected, though

Comment on lines 50 to +52
const getPower = (powers, name) => {
powers.has(name) || Fail`need powers.${b(name)} for this method`;
return /** @type {OrchestrationPowers[K]} */ (powers.get(name));
return /** @type {CosmosInterchainPowers[K]} */ (powers.get(name));
Copy link
Member

Choose a reason for hiding this comment

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

this getPower thing still isn't gone? darn.

@@ -43,6 +43,7 @@ import { makeZoeTools } from './zoe-tools.js';
* @param {Baggage} baggage
* @param {OrchestrationPowers} remotePowers
* @param {Marshaller} marshaller
* @internal
Copy link
Member

Choose a reason for hiding this comment

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

doesn't seem to have the desired effect. I still see it:

image

https://ta-export-withorchestration.agoric-sdk.pages.dev/modules/_agoric_orchestration

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. It needs the excludeInternal option, but even with that it's appearing.
Screenshot 2024-07-31 at 8 33 18 AM

I'll wait for #9727 and try again. I don't want to create merge conflicts for that one and maybe it fixes the bug.

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 tried merging and that didn't fix it. Now I suspect a bug in typedoc

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I've tried without luck to get @internal to do anything

@turadg
Copy link
Member Author

turadg commented Sep 9, 2024

Delivered better in #10050

@turadg turadg closed this Sep 9, 2024
@turadg turadg deleted the ta/export-withOrchestration branch September 9, 2024 21:10
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.

3 participants