-
Notifications
You must be signed in to change notification settings - Fork 212
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
Upgrade wallet-factory and zoe in handler #8872
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any problems.
I'm not certain I know enough to approve this, but given the test coverage, I'll go ahead.
// First, upgrade wallet factory | ||
vm.CoreProposalStepForModules("@agoric/builders/scripts/smart-wallet/build-wallet-factory2-upgrade.js"), | ||
// Then, upgrade Zoe and ZCF | ||
vm.CoreProposalStepForModules("@agoric/builders/scripts/vats/replace-zoe.js"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm only vaguely familiar with this CoreProposalSteps
go stuff. I presume that the mechanism itself is sufficiently tested and the tests inpost.test.js
suffice to show that this usage is wired up right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the mechanism now used in the release branch, so this is just replicating that here on the master branch
+export { | ||
+ getAwaitArgGuardPayload, | ||
+ getMethodGuardPayload, | ||
+ getInterfaceGuardPayload, | ||
+ getInterfaceMethodKeys, | ||
+} from './src/patterns/getGuardPayloads.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not confident about how to review a patch in detail, but this part of it seems to correspond to
https://github.com/endojs/endo/pull/2038/files#diff-785fa700ea5ccbedf6df4f2d5f2abc2b68e4e7d87dd2c5638b264c49f1f90509
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we usually track the work to get rid of such a patch when it's no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called out in the maintainers steps.
I also plan on staging a commit removing the patch in the integration-endo-master
branch, used for nightly integration with endo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removal staged here: 0104d4d
closes: #8826
refs: #8773
refs: #8802 #8793 and #7946
Description
The release branch cherry-picks (with adaptations) the wallet factory and zoe+zcf changes from master. However it also contains logic to perform the core proposal upgrade of these vats, which master lacks. This was previously blocked on #8826 which prevented upgrading vats when a new liveslots supervisor with a newer endo version with incompatible patterns packages is in use. This PR includes the fix provided by endojs/endo#2038 as a patch to put master back in sync with the release branch.
Security Considerations
None
Scaling Considerations
None
Documentation Considerations
None
Testing Considerations
Integration ran manually and will need to keep passing for merging
Upgrade Considerations
This replicates what the upgrade release branch does. Once the a3p base image is updated to capture the upcoming upgrade-14, the core proposals will need to be removed here (or at least the tests updated).