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.

Sign up for GitHub

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

Jump to bottom

Update a3p-integration after upgrade 16 #9878

Merged
merged 6 commits into from
Aug 16, 2024
Merged

Update a3p-integration after upgrade 16 #9878

merged 6 commits into from
Aug 16, 2024

Conversation

Chris-Hibbert
Copy link
Contributor

closes: #9835
refs: Agoric/agoric-3-proposals#167

Description

Remove changes that were included in upgrade 16 from a3p-integration.

Security Considerations

N/A

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Set up the test environment for the next phase.

Upgrade Considerations

Clean up for the next set of upgrade tests.

@Chris-Hibbert Chris-Hibbert added test contract-upgrade next-release about next agoric-sdk or endo release labels Aug 12, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Aug 12, 2024
@Chris-Hibbert Chris-Hibbert requested review from dckc and mhofman August 12, 2024 18:13
@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link

cloudflare-workers-and-pages bot commented Aug 12, 2024 •
edited
Loading

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3ff2d5a
Status: ✅  Deploy successful!
Preview URL: https://87bf4c67.agoric-sdk.pages.dev
Branch Preview URL: https://9835-a3pi-post-u16.agoric-sdk.pages.dev

View logs

@turadg turadg added the force:integration Force integration tests to run on PR label Aug 13, 2024
turadg
turadg previously requested changes Aug 13, 2024
View reviewed changes
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.

b:enable-orchestration needs to stay. it wasn't part of u16. it's a separate proposal.

Regardless, c:stake-bld needn't be renamed.

a3p-integration/proposals/b:enable-orchestration/package.json Outdated Show resolved Hide resolved
@Chris-Hibbert
Copy link
Contributor Author

I reverted the removal of orchestration.

I will now wait for Agoric/agoric-3-proposals#166 before merging, but a final review would be appreciated.

@Chris-Hibbert Chris-Hibbert requested review from turadg and dckc August 13, 2024 23:50
turadg
turadg reviewed Aug 14, 2024
View reviewed changes
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 won't approve until this is based on latest that includes u16. Until then it's hard to say whether the changes are valid.

Weren't these in u16?

				// Enable low-level Orchestration.
				vm.CoreProposalStepForModules(
					"@agoric/builders/scripts/vats/init-network.js",
					"@agoric/builders/scripts/vats/init-localchain.js",
					"@agoric/builders/scripts/vats/init-transfer.js",
				),

@Chris-Hibbert
Copy link
Contributor Author

Weren't these in u16?

I removed those lines in upgrade.go.

I wasn't involved in orchestration. So some low level orchestration tooling was added in upgrade.go, but b:enable-orchestration was dropped. What did the latter contain? Were any tests for the orchestration tools run for U16?

@turadg
Copy link
Member

turadg commented Aug 14, 2024

b:enable-orchestration was dropped. What did the latter contain?

Starting vat-orchestration

Were any tests for the orchestration tools run for U16?

You mean the init-network, init-localchain, init-transfer? Yes, there are other tests in upgrade-next. E.g. https://github.com/Agoric/agoric-sdk/blob/master/a3p-integration/proposals/a:upgrade-next/localchain.test.js#L34

You don't need to analyze those. They'll all start failing when this PR is on top of an A3P image with u16. It's blocked on that.

@Chris-Hibbert Chris-Hibbert requested a review from turadg August 14, 2024 22:39
@Chris-Hibbert
Copy link
Contributor Author

Rerunning now with the latest latest.

turadg
turadg reviewed Aug 15, 2024
View reviewed changes
Copy link
Member

@turadg turadg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

failing yet on,

#27 46.52 Waiting for upgrade height for UNRELEASED_A3P_INTEGRATION to be reached (need 1119, have 1118)
#27 47.44 10:47PM ERR UPGRADE "UNRELEASED_A3P_INTEGRATION" NEEDED at height: 1119: {"coreProposals":[]}

a3p-integration/proposals/README.md Outdated

Once `latest` has changed, if it was a Software Upgrade Proposal then the
upgrade handler in master will fail. If that poses a problem, you can set
a3p-integration `agoricSyntheticChain.fromtTag` to a specific version instead
Copy link
Member

Choose a reason for hiding this comment

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

fromTag

a3p-integration/proposals/README.md
```
"releaseNotes": false,
"sdkImageTag": "unreleased",
"planName": "UNRELEASED_A3P_INTEGRATION",
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that some other docs are still pointing to UNRELEASED_UPGRADE, which wasn't updated in the docs when that string changed in #9575

@turadg turadg dismissed their stale review August 15, 2024 00:59

this is now on a3p:latest with u16

dckc
dckc reviewed Aug 15, 2024
View reviewed changes
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.

some suggestions to consider

I think approval should wait until it's passing ci.

a3p-integration/proposals/README.md
To build that artifact,
```
cd a3p-integration
scripts/build-all-submissions.sh
Copy link
Member

Choose a reason for hiding this comment

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

This results in files with absolute filenames on the developer's machine; for example /home/connolly/.agoric/cache/b1-.... Should those be changed? Some advice one way or the other would be nice here.

a3p-integration/proposals/a:upgrade-next/provisioning.test.js
t.is(incarnation, 1);
});

test.serial(`send invitation via namesByAddress`, async t => {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a feature that should work in every release going forward. Deleting the test seems unfortunate.

How about migrating it to the acceptance test?

Copy link
Member

Choose a reason for hiding this comment

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

Punted to #8961

@turadg turadg force-pushed the 9835-a3pi-post-u16 branch from 8126798 to 985c754 Compare August 16, 2024 12:22
turadg
turadg approved these changes Aug 16, 2024
View reviewed changes
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 implemented some innocuous changes and cleaned up the commit history to get this in to unbreak master.

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Aug 16, 2024
@turadg turadg mentioned this pull request Aug 16, 2024
Open
@turadg turadg force-pushed the 9835-a3pi-post-u16 branch from 985c754 to ea761fc Compare August 16, 2024 12:28
@turadg
Copy link
Member

turadg commented Aug 16, 2024

@mergify update

@turadg
Copy link
Member

turadg commented Aug 16, 2024

@mergify rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 16, 2024

update

✅ Branch has been successfully updated

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 16, 2024

rebase

✅ Branch has been successfully rebased

@turadg turadg force-pushed the 9835-a3pi-post-u16 branch from 8edbd18 to 3ff2d5a Compare August 16, 2024 16:01
@mergify mergify bot merged commit de95355 into master Aug 16, 2024
82 checks passed
@mergify mergify bot deleted the 9835-a3pi-post-u16 branch August 16, 2024 16:34
@turadg turadg mentioned this pull request Sep 9, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dckc dckc dckc left review comments

@turadg turadg turadg approved these changes

@mhofman mhofman Awaiting requested review from mhofman

Assignees

@Chris-Hibbert Chris-Hibbert

Labels
automerge:rebase Automatically rebase updates, then merge contract-upgrade force:integration Force integration tests to run on PR next-release about next agoric-sdk or endo release test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

update a3p-integration to post-upgrade16
3 participants
@Chris-Hibbert @turadg @dckc