-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
8722 provisionPool re-register notifiers #9481
Changes from all commits
ada1db8
f20b9da
6189733
7b3eea8
997d72c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this test being removed? |
This file was deleted.
This file was deleted.
This file was deleted.
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not make a deposit to verify that ability? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you'd have to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did try that. Lemme try again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I do, I get an error at runtime
without anything I can recognize as indicating where the problem manifested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh yeah, that durn dependency on SES. You'll also need There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); |
This file was deleted.
This file was deleted.
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.
not worth the abstraction imo
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.
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. :-)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.
ah, I didn't notice the export privacy aspect