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

Clarify a3p-integration #8886

Merged
merged 3 commits into from
Feb 15, 2024
Merged

Clarify a3p-integration #8886

merged 3 commits into from
Feb 15, 2024

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Feb 10, 2024

Description

Clarifies the structure of a3p-integration and its role in agoric-sdk.

Testing & Upgrade Considerations

Maintainer documentation only

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

This is wonderful. Great job!

I request some changes in the explanation, but won't place a block on someone else approving. It's just docs so nearly as easy to revise after merge.

a3p-integration/README.md Outdated Show resolved Hide resolved
a3p-integration/README.md Outdated Show resolved Hide resolved
a3p-integration/README.md Outdated Show resolved Hide resolved
a3p-integration/README.md Outdated Show resolved Hide resolved
a3p-integration/README.md Outdated Show resolved Hide resolved

The only proposal in this end-to-end `a3p-integration` test is named `a:upgrade-next` and performs a chain software upgrade proposal.

The details of a proposal are configured through the `agoricProposal` field of its `package.json`. For a chain software upgrade, the `type` is `"Software Upgrade Proposal"`, and the relevant fields are `sdkImageTag`, `planName` and `upgradeInfo`. The first, `sdkImageTag`, is the docker image tag to use that contains the upgraded chain software, and has a value of `unreleased`, which is expected to have been built from the enclosing `agoric-sdk` repository. The other 2 are details of the proposed chain software upgrade. The `planName` is `UNRELEASED_UPGRADE` in the `master` branch, or `agoric-upgrade-*` in release branches. The `upgradeInfo` can be configured to include a `coreProposal` field to run any core proposals not otherwise configured in the chain software's upgrade handler (see `CoreProposalSteps` in `golang/cosmos/app/app.go`).
Copy link
Member

Choose a reason for hiding this comment

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

This is getting into the basic mechanics of @agoric/synthetic-chain. I think it would be better to direct the reader to that documentation and expect they understand it.

https://github.com/Agoric/agoric-3-proposals/
https://github.com/Agoric/agoric-3-proposals/tree/main/packages/synthetic-chain

Insofar as that documentation is lacking, let's improve it there. Having it duplicated here isn't awful but it's bound to get stale.

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 wanted this to document the integration with the tool, but did extend on how the tool works since the doc is lacking there, and couldn't make references.

Copy link
Member

Choose a reason for hiding this comment

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

To not slow this down, I'm fine with accepting refactoring to there as future work

a3p-integration/README.md Outdated Show resolved Hide resolved
a3p-integration/README.md Outdated Show resolved Hide resolved

For each proposal, their package.json is also separate and can't access the SDK code. There is an issue to automatically build proposals from scripts declared in the proposal package.json: https://github.com/Agoric/agoric-3-proposals/issues/87 . Until that is resolved, use `agoric run` on the proposal and copy the outputs to a `submission` directory within the proposals package, to be checked in.
## Hooks
Copy link
Member

Choose a reason for hiding this comment

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

imo this section belongs in @agoric/synthetic-chain docs instead

a3p-integration/README.md Show resolved Hide resolved
@mhofman
Copy link
Member Author

mhofman commented Feb 15, 2024

Updated to merge latest master changes, and address remaining feedback. PTAL

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

I think we should be more precise about the build artifacts (e.g. "layer") but it's not a blocker. I expect this doc to continue to evolve.

a3p-integration/README.md Outdated Show resolved Hide resolved
a3p-integration/README.md Outdated Show resolved Hide resolved
a3p-integration/README.md Outdated Show resolved Hide resolved
a3p-integration/README.md Outdated Show resolved Hide resolved
@mhofman mhofman requested a review from turadg February 15, 2024 16:50
@mhofman
Copy link
Member Author

mhofman commented Feb 15, 2024

Reworked some of the text, and more importantly, made the sdk image rebuilding automatic as it's a foot gun currently

@mhofman mhofman added the force:integration Force integration tests to run on PR label Feb 15, 2024
.dockerignore Outdated Show resolved Hide resolved
a3p-integration/package.json Show resolved Hide resolved
a3p-integration/README.md Outdated Show resolved Hide resolved
@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Feb 15, 2024
@mergify mergify bot merged commit c7efeba into master Feb 15, 2024
67 checks passed
@mergify mergify bot deleted the mhofman/clarify-a3p branch February 15, 2024 22:26
mhofman pushed a commit that referenced this pull request Feb 18, 2024
mhofman pushed a commit that referenced this pull request Feb 18, 2024
mhofman pushed a commit that referenced this pull request Feb 18, 2024
mhofman pushed a commit that referenced this pull request Feb 19, 2024
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.

3 participants