chore(vat-upgrade): upgrade agoricNames, test old values are preserved

[anilhelvaci]

closes: #10408

Description

As stated in #8158, we need to be able to upgrade any vat we want and make sure the system keeps working. This PR focuses on upgrading vat-agoricNames.

Security Considerations

I consider vat-agoricNames as a critical vat as it's the source trust for object identities. So it's crucial it keeps working after the upgrade. It's important to point out that if there were any keywords reserved before the upgrade, they will be lost after the upgrade as the reservations stored in ephemera. If this is important, we might consider re-reserving in the core eval.

Scaling Considerations

None.

Documentation Considerations

Don't think it's necessary as the functionality of nameHub is left untouched.

Testing Considerations

If we decide to re-reserve some keywords, we will need to add a test that verifies the core eval has indeed reserved those keywords. Please see below for the test plan I followed:

agoric-sdk/a3p-integration/proposals/p:upgrade-19/agoricNames.test.js

Lines 3 to 46 in 1e4beba

	/**
	* @file The goal of this file is to test different aspects of agoricNames to make sure
	* everything works after an upgrade. Here's the test plan;
	* 1. upgrade agoricNames
	* 2. send a core-eval that writes into children of agoricNames (brand, issuer, instance...)
	* 2b. expect a child nameHub of agoricNames will publish ALL its entries when a new item is written to it
	* 2c. check the values in the vstorage match before and after the upgrade
	* 2d. also check that new items are in the vstorage as well
	* 3. append new chain
	* CONTEXT: there are two new children introduced to agoricNames by orchestration and their
	* onUpdate callback isn't durable. So we must check that if we write a new chain info to those child
	* nameHubs, we should observe the new value in vstorage.
	* 3b. send a core-eval that writes new chain info to published.agoricNames.chain and published.agoricNames.chainConnection
	* 3c. wait until the expected data observed in vstorage
	*
	*
	* TESTING CODE THAT HOLDS ONTO 'agoricNames': smartWallet is one of the vats that depend on agoricNames to work properly the most.
	* smartWallet uses agoricNames to;
	* - create purses for known brands
	* - looks for the brand in agoricNames.brand
	* - creates a purse for the brand using the issuer in agoricNames.issuer
	* - create invitations for a from the publicFacet of a given instance (agoricNames.instance)
	*
	* So the fact that a user can complete an offer successfully means;
	* - smartWallet can find the instance on agoricNames.instance (for invitationSource = 'agoricContract')
	* - smartWallet can find, if not present create, a purse for known brand, agoricNames.brand
	* and agoricNames.issuer returned correct values
	*
	*
	* 4. add a new PSM and swap against it
	* 4b. adding the new PSM requires introducing a new asset to the chain and writing
	* the PSM instance to agoricNames.instance
	* 4c. being able to deposit the new asset to a user means that smartWallet created a purse
	* for the new brand
	* 4d. being able to send the offer to the PSM instance means smartWallet can find the instance
	* in agoricNames.instance
	*
	* 5. we want to make sure objects that were already in agoricNames works as well, so open a vault
	* in an existing collateralManager
	* 5a. fund GOV1 with ATOM
	* 5b. open a vault
	* 5c. check the vault is opened successfully
	*
	*/

EDIT

2195ace removes write-chain-info.js from f:fast-usdc. So we simulate what is does (relative to agoricNames) in order to test ephemeral onUpdate callbacks keep working after an agoricNames upgrade.

Upgrade Considerations

This PR itself upgrade vat-agoricNames and comes with a3p tests to verify the upgrade goes through and agoricNames keep working.

[cloudflare-workers-and-pages]

Deploying agoric-sdk with    Cloudflare Pages

Latest commit:	cd35800
Status:	✅  Deploy successful!
Preview URL:	https://4e86578e.agoric-sdk.pages.dev
Branch Preview URL:	https://anil-10408-agoricnames-upgra.agoric-sdk.pages.dev

View logs

[anilhelvaci]

Could you please take a look at this file and let me know the test coverage (including the skipped one) here makes sense and is enough? @Chris-Hibbert

[Chris-Hibbert]

I think this only tests reading via vstorage. I'd like to ensure that objects (issuers, brands, etc.) retrieved from it after upgrade are still valid unbroken objects. I presume this can be done from inside a proposal.

Is there anything here that tests onUpdate?

[anilhelvaci]

I think this only tests reading via vstorage. I'd like to ensure that objects (issuers, brands, etc.) retrieved from it after upgrade are still valid unbroken objects. I presume this can be done from inside a proposal.

Yeah we check the vstorage but the way nameHub works is that when a hub is written new data, ALL of its entries are published to vstorage. See;

agoric-sdk/packages/vats/src/nameHub.js

Lines 113 to 122 in e596a01

	const updated = (updateCallback, hub, _newValue = undefined) => {
	if (!updateCallback) {
	return;
	}
	// wait for values to settle before writing
	return E.when(deeplyFulfilledObject(hub.entries()), settledEntries =>
	E(updateCallback).write(settledEntries),
	);
	};

So what we do is;

• we read the vstorage before writing new data to agoricNames' children (brand, issuer, instance...)
• write data to agoricNames via a core-eval
• expect that the child hubs of agoricNames publish ALL their entries to vstorage along with the new data
• read vstorage again and make sure the agoricNames read from the vstorage BEFORE the core-eval is the same as the one read AFTER the core-eval (with the addition of newly written data of course).

...still valid unbroken objects

My plan for making sure the objects are unbroken is to go over the contracts that use agoricNames and make sure they still function as expected. For example, in order to open a vault there needs to be a valid instance object in published.agoricNames.instance for VaultFactory.

Is there anything here that tests onUpdate?

Test named check we can add new chains appends a new chain to published.agoricNames.chain and published.agoricNames.chainConnection. Both of these paths have their onUpdate callback registered here:

agoric-sdk/packages/orchestration/src/proposals/init-chain-info.js

Lines 46 to 67 in e596a01

	await E(nameAdmin).onUpdate(
	// XXX will live on the heap in the bootstrap vat. When we upgrade or kill
	// that this handler will sever and vat-agoricNames will need to be upgraded
	// to allow changing the handler, or to use pubsub mechanics instead.
	Far('chain info writer', {
	write(entries) {
	// chainInfo has no cap data but we need to marshal bigints
	const marshalData = makeMarshal(_val => Fail`data only`);
	for (const [chainName, info] of entries) {
	const value = JSON.stringify(marshalData.toCapData(info));
	if (prev[chainName] === value) {
	continue;
	}
	const chainNode = E(chainNamesNode).makeChildNode(chainName);
	prev[chainName] = value;
	void E(chainNode)
	.setValue(value)
	.catch(() => delete prev[chainName]);
	}
	},
	}),
	);

The fact that we are able to append and observe new chain info after an upgrade shows that onUpdate callback above works.

[Chris-Hibbert]

in order to open a vault there needs to be a valid instance object in published.agoricNames.instance for VaultFactory.

sounds good.

The fact that we are able to append and observe new chain info after an upgrade shows that onUpdate callback above works.

Great!

[anilhelvaci]

Looks like 2195ace disables init-chain-info.js. So made this commit 47c9309 to test ephemeral onUpdate calls.

[Chris-Hibbert]

I think this only tests reading via vstorage. I'd like to ensure that objects (issuers, brands, etc.) retrieved from it after upgrade are still valid unbroken objects. I presume this can be done from inside a proposal.

Is there anything here that tests onUpdate?

[Chris-Hibbert]

Your approach seems like it covers the bases. I think you're not done yet, so I didn't do a complete review. Let me know if/when it's ready.

[Chris-Hibbert]

in order to open a vault there needs to be a valid instance object in published.agoricNames.instance for VaultFactory.

sounds good.

The fact that we are able to append and observe new chain info after an upgrade shows that onUpdate callback above works.

Great!

[anilhelvaci]

Your approach seems like it covers the bases. I think you're not done yet, so I didn't do a complete review. Let me know if/when it's ready.

Oh actually I’ve got nothing to add (other than fixing the CI of course) assuming you’ve read the parts under Testing Considerations and Security Considerations where I talk about reserved keywords.

[Chris-Hibbert]

Oh actually I’ve got nothing to add (other than fixing the CI of course)
I'll wait till CI is passing to do a thorough review.

assuming you’ve read the parts under Testing Considerations and Security Considerations where I talk about reserved keywords.

Yep. I've seen that. I don't believe that's a problem.

[Chris-Hibbert]

looks fine. One question.

[Chris-Hibbert]

Why is it okay to drop this test?

[anilhelvaci]

2195ace removes write-chain-info.js from f:fast-usdc. So we simulate what is does (relative to agoricNames) in order to test ephemeral onUpdate callbacks keep working after an agoricNames upgrade.

Because of this.