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

feat: repair KREAd contract on zoe upgrade #8853

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

Chris-Hibbert
Copy link
Contributor

closes: #8837

Description

Invoke KREAd's revive

Security Considerations

negligible

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Needs to be tested. I think doing any KREAd transaction after the upgrade demonstrates success.

Upgrade Considerations

#8714 showed that a Zoe upgrade would break promises that the KREAd contract depends on. We expect creatorFacet.reviveMarketExitSubscribers(); to reconnect to the relevant promises.

@Chris-Hibbert Chris-Hibbert added the Zoe Contract Contracts within Zoe label Feb 1, 2024
@Chris-Hibbert Chris-Hibbert requested a review from mhofman February 1, 2024 00:59
@Chris-Hibbert Chris-Hibbert self-assigned this Feb 1, 2024
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

You need to include the revive kread script in the list of core proposal steps to execute during the upgrade, after Zoe upgrade here:

// Then, upgrade Zoe and ZCF
vm.CoreProposalStepForModules("@agoric/vats/scripts/replace-zoe.js"),

const creatorFacet = kreadKit.creatorFacet;
console.log(`KREAd creatorFacet`, creatorFacet);
await E(creatorFacet).reviveMarketExitSubscribers();
console.log('success!');
Copy link
Member

Choose a reason for hiding this comment

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

Please use a more descriptive message. This will be mixed with other bootstrap vat logging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@Chris-Hibbert Chris-Hibbert force-pushed the 8837-kread-postZoeUpgrade branch from adc4f87 to 1f982b6 Compare February 5, 2024 20:13
@Chris-Hibbert
Copy link
Contributor Author

I've now added a test showing a KREAd character purchase.

@Chris-Hibbert Chris-Hibbert requested a review from mhofman February 5, 2024 20:32
@ivanlei ivanlei linked an issue Feb 6, 2024 that may be closed by this pull request
@mhofman mhofman force-pushed the mhofman/upgrade-zoe-zcf branch from e052586 to 40407e9 Compare February 8, 2024 21:38
Base automatically changed from mhofman/upgrade-zoe-zcf to dev-upgrade-14 February 8, 2024 22:44
export const defaultProposalBuilder = async () =>
harden({
sourceSpec: '@agoric/vats/src/proposals/kread-proposal.js',
getManifestCall: ['getManifestForKread', harden({})],
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary nested harden.

Suggested change
getManifestCall: ['getManifestForKread', harden({})],
getManifestCall: ['getManifestForKread', {}],

But for that matter, this argument is ignored by packages/vats/src/proposals/kread-proposal.js getManifestForKread anyway and should itself be unnecessary.

Suggested change
getManifestCall: ['getManifestForKread', harden({})],
getManifestCall: ['getManifestForKread'],

Comment on lines 88 to 89
// eslint-disable-next-line no-use-before-define
watchForAdminNodeDone(adminNode, instanceAdmin);
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer watchForAdminNodeDone to be part of the watcher for less circularity, but this does work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a separate function because it's called from other places outside the watcher.

Copy link
Member

Choose a reason for hiding this comment

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

Out of scope of for this PR

@mhofman mhofman added the force:integration Force integration tests to run on PR label Feb 19, 2024
@mhofman mhofman force-pushed the 8837-kread-postZoeUpgrade branch from e01012d to cb20dc9 Compare February 20, 2024 00:20
@mhofman mhofman merged commit 8047ca6 into dev-upgrade-14 Feb 20, 2024
61 checks passed
@mhofman mhofman deleted the 8837-kread-postZoeUpgrade branch February 20, 2024 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR Zoe Contract Contracts within Zoe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When upgrading Zoe on chain, revive KREAd subscribers
3 participants