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

use readPublished for types in agoric-cli #10450

Merged
merged 3 commits into from
Nov 12, 2024
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Nov 11, 2024

follow-up

Description

Pulled out of #10422 while debugging an intermittent Docker integration failure.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

CI

Upgrade Considerations

none

Copy link

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

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: c890f53
Status: ✅  Deploy successful!
Preview URL: https://fab14af4.agoric-sdk.pages.dev
Branch Preview URL: https://ta-cli-readpublished.agoric-sdk.pages.dev

View logs

@turadg turadg added the force:integration Force integration tests to run on PR label Nov 11, 2024
@turadg turadg marked this pull request as ready for review November 11, 2024 23:03
@turadg turadg requested a review from a team as a code owner November 11, 2024 23:03
@@ -98,13 +98,13 @@ export const makeGovCommand = (_logger, io = {}) => {
optUtils,
) {
const utils = await (optUtils || makeVstorageKit({ fetch }, networkConfig));
const { agoricNames, readLatestHead } = utils;
const { agoricNames, readPublished } = utils;
Copy link
Member Author

Choose a reason for hiding this comment

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

should we just pass the utils object instead of de- and re-structuring?

Copy link
Member

Choose a reason for hiding this comment

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

No strong feelings, as long as the callees are clear about the least authority they need (using Pick<...> perhaps).

My vague memory is that I tried out several different ways to organize this stuff and either didn't find anything that I particularly liked or at least didn't go back to make things uniform.

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 way is done so I'll leave any other iterations to a future refactoring.

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Nov 12, 2024
@mergify mergify bot merged commit 3b799b8 into master Nov 12, 2024
112 checks passed
@mergify mergify bot deleted the ta/cli-readPublished branch November 12, 2024 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants