Add AssetReserve to Upgrade 19 with A3P test coverage #10541
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
95f6cf3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c7c1c1f0.agoric-sdk.pages.dev |
| Branch Preview URL: | https://iomekam-ar-upgrade-tests.agoric-sdk.pages.dev |
There was a problem hiding this comment.
That's a helpful test, but we shouldn't close #10399 until we have an upgrade set up in upgrade.go, and tests in a3p-integration.
One of the things I'm looking forward to seeing in a3p-integration is that the vstorage records look reasonable after the upgrade.
iomekam
changed the title
…agoric-sdk into iomekam-ar-upgrade-tests
| @@ -5,3 +5,4 @@ addUsdOlives/ | |||
| upgradeProvisionPool/ | |||
| upgradeAgoricNames/ | |||
| publishTestInfo/ | |||
| upgradeAssetReserve/ | |||
There was a problem hiding this comment.
This file should end with a linefeed.
packages/inter-protocol/test/swingsetTests/reserve/bootstrap-assetReserve-upgrade.js
Outdated
Show resolved
Hide resolved
| const { assetReserveRef } = options.options; | ||
|
|
||
| assert(assetReserveRef.bundleID); | ||
| tracer(`ASSET RESERBE BUNDLE ID: `, assetReserveRef); |
| reserveKitWriter.reset(); | ||
| reserveKitWriter.resolve( | ||
| harden({ | ||
| ...reserveKit, | ||
| adminFacet, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
As long as you're doing this, might as well clean up the original, (we want those proposals to be usable to start a new chain, or to copy, even if we won't run them again on mainNet) and add a comment here as to why this fix is necessary.
| test('test upgraded board', async t => { | ||
| // agoricProposal.sdk-generate in package.json generates this proposal | ||
| await evalBundles('testUpgradedBoard'); |
There was a problem hiding this comment.
Ah this file should be deleted entirely!
There was a problem hiding this comment.
This proposal file often goes in agoric-sdk/packages/vats, and the package.json tells it to construct this directory.
A more recent model for tests, followed by valueVow, is to put the script and proposal in the same file in ...packages/builders/scripts/testing.
There was a problem hiding this comment.
This is a proposal specific to testing the upgrade. I followed the similar pattern of depositLemons, which also lives within the p:upgrade-19 directory
| errorMessage: 'psm-IST-USD_LEMONS instance not observed.', | ||
| }); | ||
|
|
||
| await evalBundles(ADD_COLLAOTRAL_DIR); |
| }); | ||
|
|
||
| await evalBundles(ADD_COLLAOTRAL_DIR); | ||
| // await evalBundles(UPGRADE_AR_DIR); |
There was a problem hiding this comment.
This should be dropped, right? The actual upgrade is happening on line 79.
|
tests are failing |
| const { feeMint } = await provideAll(baggage, { | ||
| feeMint: () => zcf.registerFeeMint('Fee', privateArgs.feeMintAccess), | ||
| }); | ||
|
|
There was a problem hiding this comment.
Assuming the feeMint wasn't already in the baggage, which is why takeFeeMint() was failing, how did provideAll solved the "no contact with other vats during the first crank of the new incarnation"? Does provideAll somehow delay the execution of zcf.registerFeeMint to another crank?
There was a problem hiding this comment.
feeMint was already in baggage. It was failing because we were upgrading the governor contract instead of the reserve contract, which I assume has a different baggage attached to it. provideAll just abstracts the logic of creating the item in baggage on the first incarnation and then checking the value on future upgrades.
|
|
||
| t.log(vatDetailsAfter); | ||
| t.is(incarnation, 1, 'incorrect incarnation'); | ||
| t.pass(); |
There was a problem hiding this comment.
t.pass() is only necessary when there are no tests. There are a few t.is(), and a t.truthy(), so this isn't helpful.
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks: You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
closes: #10399
Description
Adds the upgrade of the reserve to upgrade 19. To ensure that this upgrade will succeed, we've also added test coverage in A3P. One caveat we discovered while testing was that the
adminFacetthat's produced with thereserveKitwas referencing theadminFacetof the governor. Luckily, we were able to get theadminFacetof the reserve contract by callinggetAdminFaceton the governor'screatorFacet.Upgrade Considerations
This PR adds the reserve to the list of vat upgrades for upgrade-19. These upgrades are currently commented out as we are still in the process of getting upgrade-18 out the door. We've added A3P tests to ensure that the upgrade is successful and that assets are still in the reserve after the upgrade.