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

chore: Integrate on top of upgrade-15 #9436

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

gibson042
Copy link
Member

Fixes #9260

Description

As of Agoric/agoric-3-proposals#157 , a3p:latest now includes results of upgrade-15. This removes the duplication from the UNRELEASED upgrade handler and a3p-integration.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

n/a

Testing Considerations

How much of the upgrade-15 a3p-integration should remain in a:upgrade-next? I'm thinking specifically about exit-reclaim.test.js and associated preparation.

Upgrade Considerations

@Chris-Hibbert Does initial.test.js needs some extension for the vaultFactory and/or scaledPriceAuthority upgrades?

@gibson042 gibson042 added the force:integration Force integration tests to run on PR label May 31, 2024
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.

How much of the upgrade-15 a3p-integration should remain in a:upgrade-next?

Everything that's not already tested in the A3P repo.

I'm thinking specifically about exit-reclaim.test.js and associated preparation.

That is covered in https://github.com/Agoric/agoric-3-proposals/tree/main/proposals/74%3Aupgrade-15

So please remove all tests in upgrade-next that aren't testing upgrade-next changes.

Comment on lines 899 to 900
// Upgrade ZCF only
vm.CoreProposalStepForModules("@agoric/builders/scripts/vats/upgrade-zcf.js"),
Copy link
Contributor

Choose a reason for hiding this comment

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

for Upgrade 16, we want to replace upgrade ZCF only with upgrade Zoe. I presume that's separate from this.

Copy link
Member

Choose a reason for hiding this comment

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

Since the failing test indicates that a ZCF upgrade is needed, this PR should at least maintain the ZCF upgrade core proposal. Since the plan for the next upgrade seem to upgrade both Zoe and ZCF, we could also do that instead in this PR.

@Chris-Hibbert
Copy link
Contributor

Does initial.test.js need some extension for the vaultFactory and/or scaledPriceAuthority upgrades?

I don't think it'll be necessary to add anything.

@gibson042
Copy link
Member Author

I'm seeing failures in c:stake-bld/stakeBld.test.js broadcasting executeOffer with offer id 'request-stake': https://github.com/Agoric/agoric-sdk/actions/runs/9321875452/job/25661786100?pr=9436#step:9:2071

SwingSet: ls: v77: TypeError: atomicTransfer: no function
at atomicTransfer (.../zoe/src/contractSupport/atomicTransfer.js:97)
at withdrawFromSeat (.../zoe/src/contractSupport/zoeHelpers.js:228)
at withdrawFromSeat (.../zoe/src/contractSupport/zoeHelpers.js:225)
at (.../orchestration/src/examples/stakeBld.contract.js:88)
at ()
at ()

Is that meaningful to anyone here?

@Chris-Hibbert
Copy link
Contributor

#8520 points to this problem.
#7900 changed the helper to rely on the new ZCF. #7900 shouldn't be used to build bundles before upgrade 16 is on chain.

I guess bundles in A3P are built from HEAD, which can be incompatible

@Chris-Hibbert
Copy link
Contributor

Chris-Hibbert commented May 31, 2024

What are the chances that the proposal for orchestration ran before the proposal that set a new ZCF version? If that happened, stakeBid could have been built against a support library that presumed that ZCF was updated, but then deployed against the ZCF that hadn't yet changed.

Copy link

cloudflare-workers-and-pages bot commented Jun 3, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 147ee96
Status: ✅  Deploy successful!
Preview URL: https://7350fbc8.agoric-sdk.pages.dev
Branch Preview URL: https://gibson-9260-a3p-integration.agoric-sdk.pages.dev

View logs

@turadg turadg force-pushed the gibson-9260-a3p-integration-upgrade-15 branch from 634c565 to 147ee96 Compare June 3, 2024 16:36
@gibson042 gibson042 added the automerge:rebase Automatically rebase updates, then merge label Jun 3, 2024
@gibson042
Copy link
Member Author

For posterity, the ZCF changes in upgrade-15 were intentionally minimal (and in particular excluded the addition of atomicRearrange), so upgrade-16 needs its own ZCF upgrade. Adding that to this PR fixed the above issue.

@mergify mergify bot merged commit 440d7a8 into master Jun 3, 2024
71 of 74 checks passed
@mergify mergify bot deleted the gibson-9260-a3p-integration-upgrade-15 branch June 3, 2024 17:27
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.

u15: base a3p-integration on upgrade-15
5 participants