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

vat-priceAuthority upgrade is not tested #10727

Closed
anilhelvaci opened this issue Dec 18, 2024 · 3 comments · Fixed by #10731
Closed

vat-priceAuthority upgrade is not tested #10727

anilhelvaci opened this issue Dec 18, 2024 · 3 comments · Fixed by #10731
Assignees

Comments

@anilhelvaci
Copy link
Collaborator

anilhelvaci commented Dec 18, 2024

Problem Definition

Related #10401

Currently the registry.test.js is not included in test.sh.

Also looking at the contents of registry.test.sh:

// @ts-check
import test from 'ava';
import '@endo/init/debug.js';
import {
getDetailsMatchingVats,
getIncarnation,
} from '@agoric/synthetic-chain';
/**
* @file
* A test of upgrading vat-priceAuthority, which is planned to ship in Upgrade 9
*/
test('priceAuthorityRegistry upgrade', async t => {
t.is(await getIncarnation('priceAuthority'), 1);
const priceAuthorityVats = await getDetailsMatchingVats('priceAuthority');
t.is(priceAuthorityVats.length, 1);
});

We don't see that the vat-priceAuthority upgrade isn't carried out (looking for something like evalBundles or another way of running a core-eval) in registry.test.js.

As to the nature of how we planned Upgrade 19, vat upgrade core-evals are commented out in upgrade.go including upgrade-paRegistry.js.

In conclusion;

  • There's no code running that covers upgrade-paRegistry.js
  • Code that covers priceAuthority upgrade is not executed

Solution

  • Add another test to registry.test.js like:

    test.serial('upgrade vat-priceAuthority', async t => {
      await evalBundles('upgradePaRegistry');
    });
  • Add below line to test.sh

    yarn ava registery.test.js

Potential Improvement

Ideally, we should check that we can push prices and the pushed prices are received by the clients like vaultFactory. This is currently tested in z:acceptance. However, since we are not upgrading vat-priceAuthority, z:acceptance is not aware of any changes to vat-priceAuthority. So I see two options here;

  • We move upgrade vat-priceAuthority durably, EVAL or USE phase. This is sort of contrary to upgrade 19 plan as we comment out other core-evals.

OR;

  • We implement the case I described above in registry.test.js

Thoughts? @Chris-Hibbert

@Chris-Hibbert
Copy link
Contributor

Thanks for cross-checking this. Certainly we should add registry.test.js to test.sh. If the incarnation check shows that the upgrade isn't happening, then we'll add the evalBundle, but "vats/upgrade-paRegistry.js" is included in package.json, so the upgrade ought to be invoked by eval.sh.

I'll take care of this.

@Chris-Hibbert
Copy link
Contributor

{"vatName":"priceAuthority","vatID":"v8","incarnation":1,"bundleID":"b1-42cc2eec648914a9bd89beeb7888a2a4c7b0529a8705a9e5ddc106cbfdd7722e7ac3e6a3343728bd238f29dc4ad1724681594509d2036f7002df68467582bea0"}
  ✔ priceAuthorityRegistry upgrade
  ─

  1 test passed

@anilhelvaci
Copy link
Collaborator Author

but "vats/upgrade-paRegistry.js" is included in package.json, so the upgrade ought to be invoked by eval.sh.

Oh yeah, there's evalSubmission.js that takes care of this. Thanks Chris!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants