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

governance types inferred by start functions #7469

Merged
merged 8 commits into from
Apr 20, 2023
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Apr 20, 2023

refs: #7178

Description

Working on PSM upgrade I got tripped up on some governance facet naming and went down a rabbit hole.

This largely addresses #7178 but I don't want to close that until I'm more confident and have types regression tests.

It also removes getContractGovernor from the governed creatorFacet, per an earlier discussion

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

n/a

Testing Considerations

CI and some manual spot checking that errors aren't gone due to any

@turadg turadg requested review from dckc and Chris-Hibbert April 20, 2023 21:34
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.

next level stuff!

Comment on lines -574 to -592
* @property {(name: string) => Amount} getAmount
* @property {(name: string) => Brand} getBrand
* @property {(name: string) => Instance} getInstance
* @property {(name: string) => Installation} getInstallation
* @property {(name: string) => Amount} getInvitationAmount
* @property {(name: string) => bigint} getNat
* @property {(name: string) => Ratio} getRatio
* @property {(name: string) => string} getString
* @property {(name: string) => any} getUnknown
Copy link
Member

Choose a reason for hiding this comment

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

These were a fiction? Or they come back some other way? still reading...

@@ -304,6 +309,8 @@ const start = async (zcf, privateArgs) => {
});
};

/** @type {GovernorCreatorFacet<SF>} */
// @ts-expect-error cast
Copy link
Member

Choose a reason for hiding this comment

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

this error is a little surprising; aren't we supplying exactly what the type requires?

Copy link
Member Author

Choose a reason for hiding this comment

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

It surprised me too. Apparently within the function TS will only trust the minimum of the parameter (what SF is said to extend) which is {}. I tried doing this without an error but timed out

* @property {() => CF} getLimitedCreatorFacet - the creator
* @property {() => ERef<CF>} getLimitedCreatorFacet - the creator
Copy link
Member

Choose a reason for hiding this comment

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

this was a bug that was hidden behind an any somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's just not exercised. the code calling it never assumed it to be a presence but this is more accurate

Comment on lines 716 to 723
* Akin to StartedInstanceKit but designed for the results of starting governed contracts
* Akin to StartedInstanceKit but designed for the results of starting governed contracts. Used in bootstrap space.
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated line?

* @typedef {object} AuctioneerKit
* @property {Awaited<ReturnType<import('../auction/auctioneer.js').start>>['creatorFacet']} creatorFacet
* @property {Awaited<ReturnType<import('../auction/auctioneer.js').start>>['publicFacet']} publicFacet
* @property {GovernedContractFacetAccess<{},{}>} governorCreatorFacet
* @property {AdminFacet} adminFacet
* @typedef {GovernanceFacetKit<import('../auction/auctioneer.js').start>} AuctioneerKit
Copy link
Member

Choose a reason for hiding this comment

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

👏

* governorCreatorFacet: GovernedContractFacetAccess<VaultFactoryPublicFacet, VaultFactoryCreatorFacet>,
* adminFacet: AdminFacet,
* },
* reserveKit: GovernanceFacetKit<import('../reserve/assetReserve.js')['start']>,
Copy link
Member

Choose a reason for hiding this comment

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

ooh... ahhh...

reserveKit: { reserveCreatorFacet },
reserveKit: { creatorFacet: reserveCreatorFacet },
Copy link
Member

Choose a reason for hiding this comment

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

was this a latent bug?

Copy link
Member Author

@turadg turadg Apr 20, 2023

Choose a reason for hiding this comment

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

this was to change less in the test file when renaming reserveCreatorFacet in reserveKit

EDIT: oh but that was erroneous https://github.com/Agoric/agoric-sdk/actions/runs/4759091407/jobs/8458009082?pr=7469

*/

/**
* @see {StartedInstanceKit}
* @template {GovernableStartFn} SF
* @typedef GovernanceFacetKit
* Akin to StartedInstanceKit but designed for the results of starting governed contracts
Copy link
Member

Choose a reason for hiding this comment

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

ah. you found it.

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.

I guess we better resolve this before I approve:

message: 'Cannot deliver "addIssuer" to target; typeof target is "undefined"'

@turadg turadg force-pushed the 7178-governance-sf branch from 0c1bdb1 to 9ffde73 Compare April 20, 2023 22:36
@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Apr 20, 2023
@mergify mergify bot merged commit 7ccf3ab into master Apr 20, 2023
@mergify mergify bot deleted the 7178-governance-sf branch April 20, 2023 23:40
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