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

handle replacing installations in writeCoreEval #10172

Closed
dckc opened this issue Sep 30, 2024 · 4 comments · Fixed by #10163
Closed

handle replacing installations in writeCoreEval #10172

dckc opened this issue Sep 30, 2024 · 4 comments · Fixed by #10163
Labels
cosmic-swingset package: cosmic-swingset deployment Chain deployment mechanism (e.g. testnet) devex developer experience enhancement New feature or request

Comments

@dckc
Copy link
Member

dckc commented Sep 30, 2024

What is the Problem Being Solved?

The myContractRef: publishRef(install(...)) idiom works fine for initial deployment of installations. You might think it would work to replace installations. But in recent experience, it does not, and lack of .reset() in the code below strongly suggests why the limitation.

await Promise.all(
installationEntries.map(([key, value]) => {
produceInstallations[key].resolve(value);

Description of the Design

Add .reset().

Security Considerations

Should improve security by matching behavior better with developer expectations.

Could be a risk if any code is currently relying on the .resolve() being a no-op.

Scaling Considerations

N/A

Test Plan

A bootstrap test should suffice:

  1. boot as usual
  2. save the installation
  3. run a core proposal to replace it
  4. test that the new one is in place

cc @Chris-Hibbert

Upgrade Considerations

The coreProposalBehavior.js code is copied into each deployment, so there's no need to upgrade previous deployments.

Could be a risk if any code is currently relying on the .resolve() being a no-op.

@dckc dckc added enhancement New feature or request cosmic-swingset package: cosmic-swingset deployment Chain deployment mechanism (e.g. testnet) devex developer experience labels Sep 30, 2024
@Chris-Hibbert
Copy link
Contributor

Putting myContractRef: publishRef(install(...)) in the proposalBuilder script causes the proposal to be invoked with myContractRef bound to the installation in its options. A side-effect is that the installation is registered in the promise space in consume.installation.myContract. The bug above means that on upgrade, the installation will still be passed through in myContractRef, but the installation won't be updated in the promise space. That means that null-upgrades of these contracts would get an earlier installation.

The bug has been mitigated in several proposals by manually updating produce.installation.myContract. I see that code for auctions, vaults, and priceAggregator.

I don't see it for scaledPriceAuthority, zoe or walletFactory.

@Chris-Hibbert
Copy link
Contributor

Provisioning was upgraded in U16, and did not include this fix.

@dckc
Copy link
Member Author

dckc commented Sep 30, 2024

provisioning is a plain vat, not a contract. There's no relevant installation.

And note that code has options: { provisioningRef } -- no use of the publishRef(install(...)) idiom.

walletFactory is a contract, but upgrade-wallet-factory2-proposal.js doesn't publish any new installation (handle).

@Chris-Hibbert
Copy link
Contributor

For that matter, Zoe is also not a contract.

@mergify mergify bot closed this as completed in #10163 Oct 9, 2024
@mergify mergify bot closed this as completed in da89a20 Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cosmic-swingset package: cosmic-swingset deployment Chain deployment mechanism (e.g. testnet) devex developer experience enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants