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

8722 provisionPool re-register notifiers #9481

Merged
merged 5 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions a3p-integration/proposals/a:upgrade-next/agd-tools.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import {
agd,
agops,
agopsLocation,
CHAINID,
executeCommand,
VALIDATORADDR,
executeOffer,
GOV1ADDR,
GOV2ADDR,
GOV3ADDR,
newOfferId,
CHAINID,
VALIDATORADDR,
} from '@agoric/synthetic-chain';

const ORACLE_ADDRESSES = [GOV1ADDR, GOV2ADDR, GOV3ADDR];
Expand Down Expand Up @@ -137,3 +137,8 @@ export const bankSend = (addr, wanted) => {

return agd.tx('bank', 'send', VALIDATORADDR, addr, wanted, ...noise);
};

export const getProvisionPoolMetrics = async () => {
Copy link
Member

Choose a reason for hiding this comment

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

not worth the abstraction imo

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 hides two things: getQuoteBody() is not exported, and the callers would have to share a definition of the path. I'll make the change if you insist, but you'll have to ask a second time. :-)

Copy link
Member

Choose a reason for hiding this comment

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

ah, I didn't notice the export privacy aspect

const path = `published.provisionPool.metrics`;
return getQuoteBody(path);
};
33 changes: 0 additions & 33 deletions a3p-integration/proposals/a:upgrade-next/localchain.test.js
Copy link
Member

Choose a reason for hiding this comment

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

Why is this test being removed?

This file was deleted.

6 changes: 4 additions & 2 deletions a3p-integration/proposals/a:upgrade-next/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
"coreProposals": []
},
"sdk-generate": [
"vats/probe-zcf-bundle.js probe-submission",
"vats/test-localchain.js localchaintest-submission"
"vats/upgrade-bank.js upgrade-bank",
"vats/upgrade-provisionPool.js upgrade-provisionPool",
"testing/add-LEMONS.js add-LEMONS",
"testing/add-OLIVES.js add-OLIVES"
],
"type": "Software Upgrade Proposal"
},
Expand Down

This file was deleted.

This file was deleted.

102 changes: 102 additions & 0 deletions a3p-integration/proposals/a:upgrade-next/provisionPool.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// @ts-check

import test from 'ava';

import {
evalBundles,
getIncarnation,
waitForBlock,
} from '@agoric/synthetic-chain';

import { bankSend, getProvisionPoolMetrics } from './agd-tools.js';

const NULL_UPGRADE_BANK_DIR = 'upgrade-bank';
const UPGRADE_PP_DIR = 'upgrade-provisionPool';
const ADD_LEMONS_DIR = 'add-LEMONS';
const ADD_OLIVES_DIR = 'add-OLIVES';

const USDC_DENOM =
'ibc/295548A78785A1007F232DE286149A6FF512F180AF5657780FC89C009E2C348F';
const PROVISIONING_POOL_ADDR = 'agoric1megzytg65cyrgzs6fvzxgrcqvwwl7ugpt62346';

/**
* @file
* The problem to be addressed is that provisionPool won't correctly handle
* (#8722) deposit of assets after it (provisionPool) is upgraded or (#8724)
* new asset kinds after vat-bank is upgraded.
*
* To test this, we will
*
* 1. See that we can add USDC.
*
* 2. Null upgrade vat-bank, and see that we can add a new collateal.
*
* 2a. Not null upgrade provisionPool, since it would fail. If it had succeeded,
* we would have been able to observe the effect of #8724, which would have
* caused addition of new currencies to be ignored.
*
* 3. Do a full upgrade of provisionPool; then deposit USDC, and see IST
* incremented in totalMintedConverted.
*
* 4. Null upgrade vat-bank again, and then see (in logs) that adding a new
Copy link
Member

Choose a reason for hiding this comment

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

why not make a deposit to verify that ability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only have assets for currencies that provisionPool already knows about. We can add new currencies, but I haven't found a reasonable way to mint them to be able to deposit them.

Copy link
Member

@turadg turadg Jun 12, 2024

Choose a reason for hiding this comment

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

it is trickier since it's a synthetic-chain test. I think you could do it with core-eval that both adds a new currency and with the available powers mints then deposits. But this comment suffices.

* asset type gives the ability to make deposits. We don't actually add it
* because it would be too much work to add a faucet or other ability to mint
* the new collateral.
*/

const contributeToPool = async (t, asset, expectedToGrow) => {
const metricsBefore = await getProvisionPoolMetrics();
console.log('PPT pre', metricsBefore);

await bankSend(PROVISIONING_POOL_ADDR, asset);

const metricsAfter = await getProvisionPoolMetrics();
console.log('PPT post', metricsAfter);
t.is(
metricsAfter.totalMintedConverted.brand,
metricsBefore.totalMintedConverted.brand,
'brands match',
);
if (expectedToGrow) {
// I couldn't import AmountMath. dunno why.
Copy link
Member

Choose a reason for hiding this comment

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

you'd have to add @agoric/ertp as a dependency of this proposal package. Did you try that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try that. Lemme try again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I do, I get an error at runtime

  ReferenceError: assert is not defined

  › file://node_modules/@endo/eventual-send/src/E.js:3:40

without anything I can recognize as indicating where the problem manifested.

Copy link
Member

Choose a reason for hiding this comment

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

oh yeah, that durn dependency on SES.

You'll also need import "@endo/init/debug.js" before that import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not familiar with that one (or I've forgotten). Do you have a pointer to an issue or discussion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it turns out that what we get from vstorage isn't actually an Amount. The brand field contains stringified captp representations. We can compare them for equality, but they're not the brands that AmountMath recognizes.

t.true(
metricsAfter.totalMintedConverted.value >
metricsBefore.totalMintedConverted.value,
'brands match',
);
} else {
t.equal(
metricsAfter.totalMintedConverted.value,
metricsBefore.totalMintedConverted.value,
);
}
};

test('upgrading provisionPool and vat-bank', async t => {
t.log('add assets before');
await contributeToPool(t, `10000${USDC_DENOM}`, true);

t.log(`upgrade Bank`);
await evalBundles(NULL_UPGRADE_BANK_DIR);

const firstIncarnation = await getIncarnation('bank');
t.is(firstIncarnation, 1);

await evalBundles(ADD_LEMONS_DIR);

t.log('full upgrade ProvisionPool');
await evalBundles(UPGRADE_PP_DIR);
const ppIncarnation = await getIncarnation('db93f-provisionPool');
t.is(ppIncarnation, 1);

await contributeToPool(t, `30000${USDC_DENOM}`, true);

t.log(`Add assets after bank upgrade`);
await evalBundles(NULL_UPGRADE_BANK_DIR);
await waitForBlock(2);

const secondIncarnation = await getIncarnation('bank');
t.is(secondIncarnation, 2);

await evalBundles(ADD_OLIVES_DIR);
});
51 changes: 0 additions & 51 deletions a3p-integration/proposals/a:upgrade-next/provisioning.test.js

This file was deleted.

134 changes: 0 additions & 134 deletions a3p-integration/proposals/a:upgrade-next/upgradeVaults.test.js

This file was deleted.

Loading
Loading