Harmonise `core/boot-psm.js` with `core/boot.js` #6568

michaelfig · 2022-11-15T02:13:55Z

closes: #6490
#loadgen-branch: mfig-diagnostics

Description

Merge the changes from the core/boot-psm.js mechanism used in the first Agoric VM deployment with the manifest-based core/boot.js system.

Security Considerations

Documentation Considerations

Testing Considerations

The devnet configuration is now tested by the cosmic-swingset integration tests.

dckc · 2022-11-15T03:02:56Z

I tried the steps to reproduce from #6490 ; it doesn't look fixed.

command[9] E(home.agoricNames).lookup('instance', 'psm')
history[9] Promise.reject("Error: \"nameKey\" not found: (a string)")

michaelfig · 2022-11-15T15:43:32Z

I tried the steps to reproduce from #6490 ; it doesn't look fixed.
command[9] E(home.agoricNames).lookup('instance', 'psm')
history[9] Promise.reject("Error: \"nameKey\" not found: (a string)")

Ahh, yes. That's because there's an instance per brand pair. Try E(E(home.agoricNames).lookup('instance')).entries()

packages/cosmic-swingset/Makefile

turadg · 2022-11-15T19:20:11Z

packages/inter-protocol/scripts/init-core.js

+ options = {},
+ { env = process.env } = {},
+) => {
+ const { ROLE = env.ROLE || 'chain', econCommitteeOptions } = options;


better to pass process.env than rely on ambient authority, but why pass it at all if it can come in through options.ROLE? Multiple ways to express the same configuration makes the code harder to follow. It's not apparent without reading committeeProposalBuilder that env.ROLE is valid or takes precedence over options.ROLE.

Turns out ROLE was unnecessary. Removed.

Seems to still be there as of 2a2b288

Seems to still be there

I need to push. I'll rerequest your review when I do.

turadg · 2022-11-15T19:21:46Z

packages/inter-protocol/scripts/init-core.js

@@ -71,15 +71,16 @@ const installKeyGroups = {
 * @param {(m: string, b: string, opts?: any) => I} opts.install
 * @param {<T>(f: T) => T} [opts.wrapInstall]
 *
+ * @param options


Suggested change

* @param options

* @param {{ ROLE: 'chain' | 'whatever else', econCommitteeOptions: ??? }} options

turadg · 2022-11-15T19:22:19Z

packages/inter-protocol/src/proposals/committee-proposal.js

- [voterInvitation, nullInvitation],
+ [
+ invitationP,
+ // TODO: This null invitation is used just to identify the


TODO by when? Ticket?

turadg · 2022-11-15T19:24:26Z

packages/inter-protocol/src/proposals/startPSM.js

+ { restoreRef },
+ { installKeys },
+) => {
+ const { [installGovAndPSMContracts.name]: _toss, ...manifest } =


isn't simply _ the convention here for ignoring a param?

Suggested change

const { [installGovAndPSMContracts.name]: _toss, ...manifest } =

const { [installGovAndPSMContracts.name]: _, ...manifest } =

turadg · 2022-11-15T19:25:15Z

packages/inter-protocol/src/proposals/utils.js

+ payments.map(async (paymentP, i) => {
+ const payment = await paymentP;
+ await E(depositFacet).receive(payment);
+ console.info(


nice diagnostic info

turadg · 2022-11-15T19:28:08Z

packages/smart-wallet/src/walletFactory.js


- return E(namesByAddressAdmin).update(address, nameHub, myAddressNameAdmin);
+ // Don't clobber an existing myAddressNameAdmin or deposit facet.


this will silently fail if publishDepositFacet is called a second time and wallet.getDepositFacet() changes.

This does seem like an improvement to make it immutable, but shouldn't we error if something tries to change it? At least log?

More like it silently succeeds, yes?
This is saying "if there's already one there, leave it alone. Otherwise, use this one"

Success or failure depends on the intent ;-)

My point is: if what's already there doesn't match, isn't that a surprise? In what cases would you expect the existing value to be different than the new one?

In what cases would you expect the existing value to be different than the new one?

In case an ag-solo already registered it.

isn't that a surprise?

Not any longer, IIUC.

I'll add documentation to this effect.

turadg · 2022-11-15T19:32:14Z

packages/vats/scripts/init-core.js

@@ -0,0 +1,30 @@
+import { makeHelpers } from '@agoric/deploy-script-support';
+
+export const defaultProposalBuilder = async ({ publishRef, install }) =>


is "default" here meant like "default export" or are there expected to be multiple proposal builders in a module like this?

The coreProposals swingset config feature uses defaultProposalBuilder by default (i.e. if the config file does not specify a different entrypoint).

turadg · 2022-11-15T19:34:20Z

packages/vats/src/core/chain-behaviors.js

+ BRIDGE_ID.PROVISION_SMART_WALLET,
+ obj,
+ );
+ } else {


this is assuming the if SMART_WALLET isn't in the powerFlags that it should do this. why not validate the contents? I'm thinking the final else should be an error for an undefined case

Can you please be more specific? What kind of validation do you expect, since the Golang side of things already enforces that powerFlags is an array of strings?

dckc · 2022-11-15T20:27:40Z

Try entries()

I should have been clear: yes, I did that too. There are no PSMs

command[5] E(lookup('agoricNames', 'instance')).entries()
history[5] [...]

command[8] h[5].map(([n, _v]) => n)
history[8] ["economicCommittee","amm","ammGovernor","VaultFactory","feeDistributor","Treasury","VaultFactoryGovernor","stakeFactory","stakeFactoryGovernor","Pegasus","reserve","reserveGovernor","psmCharter","interchainPool","provisionPool","walletFactory"]

dckc · 2022-11-19T00:02:29Z

I see PSMs now:

command[0]
E(E(home.agoricNames).lookup('instance')).keys()
history[0]
["economicCommittee","amm","ammGovernor","VaultFactory","feeDistributor","Treasury","VaultFactoryGovernor","stakeFactory","stakeFactoryGovernor","Pegasus","reserve","reserveGovernor","psmCharter","interchainPool","provisionPool","walletFactory","psm-IST-USDC_axl","psm-IST-USDC_grv","psm-IST-USDT_axl","psm-IST-USDT_grv","psm-IST-AUSD"]

fyi, @thisispalash

dckc

In the interest of addressing #6490 , I'm ready to land this.

It relaxes some constraints that we introduced in boot-psm.js, so I re-opened #4165 and #5965 to re-consider our approach to those.

dckc · 2022-11-28T02:59:08Z

packages/agoric-cli/src/start.js

+ const coreConfigPath = path.resolve(
+ 'node_modules/@agoric/vats/decentral-core-config.json',


This seems to assume that the current working directory has node_modules with the @agoric/vats package installed. That seems to be a new assumption. I suppose it's justified by the way agoric install works?

Elsewhere I've see a function that resolves paths like @agoric/vats/decentral-core-config.json. I'm not sure why we don't use it here.

But I don't feel strongly about it. The whole agoric-cli package seems to be a bit of a wild west w.r.t. ocap discipline.

The dependency on node_modules was ugly and unnecessary, so I've now abstracted it behind require.resolve.

dckc · 2022-11-28T03:04:14Z

packages/vats/decentral-devnet-config.json

+ "provisioning": {
+ "sourceSpec": "@agoric/vats/src/vat-provisioning.js"
+ },


I suppose this calls for re-opening #5965 to think it over again.

dckc · 2022-11-28T03:05:08Z

packages/vats/decentral-devnet-config.json

+ "priceAuthority": {
+ "sourceSpec": "@agoric/vats/src/vat-priceAuthority.js"
+ },


why does vat-priceAuthority.js belong here rather than in a vaults proposal?
(semi-rhetorical; I suppose I could figure it out if I looked around.)

vat-priceAuthority.js is not a Zoe contract, so doesn't easily fit into the current way proposals are implemented. But yes, ideally it would be part of a proposal, and we would make a separate decision whether to publish it to solo clients.

dckc · 2022-11-28T03:10:14Z

packages/vats/decentral-devnet-config.json

+ "ibc": {
+ "sourceSpec": "@agoric/vats/src/vat-ibc.js"
+ },
+ "network": {
+ "sourceSpec": "@agoric/vats/src/vat-network.js"
+ },


@ivanlei the list of vats/contracts that I put together Nov 9 for planning purposes didn't include these two (ibc, network). I suppose they should be added to the list of things to make upgradeable (#6553). Ah... we already have #5696 for the network vat. I don't know whether we included it in any plans.

dckc · 2022-11-28T03:17:00Z

packages/inter-protocol/scripts/init-core.js

+ options = {},
+ { env = process.env } = {},
+) => {
+ const { ROLE = env.ROLE || 'chain', econCommitteeOptions } = options;


Seems to still be there as of 2a2b288

dckc · 2022-11-28T03:21:24Z

packages/inter-protocol/src/proposals/startPSM.js

+ { restoreRef },
+ { installKeys },
+) => {
+ const { [installGovAndPSMContracts.name]: _toss, ...manifest } =


Why are we removing installGovAndPSMContracts from the manifest returned by getManifestForPsmGovernance? It seems particularly relevant.

The installations property below subsumes what installGovAndPSMContracts does.

dckc · 2022-11-28T03:26:11Z

packages/vats/src/core/startWalletFactory.js

@@ -108,7 +108,8 @@ const startGovernedInstance = async (
 * }>} powers
 * @param {{
 * options?: {
- * perAccountInitialValue?: bigint
+ * perAccountInitialValue?: bigint,
+ * walletBridgeId?: string


Suggested change

* walletBridgeId?: string

* walletBridgeId?: string,

…rter

dckc

still looks good.

I have a few wishes...

dckc · 2022-12-07T19:10:31Z

packages/vats/src/core/chain-behaviors.js

+ const { nickname, address, powerFlags: rawPowerFlags } = obj;
+ const powerFlags = rawPowerFlags || [];


don't we usually write it this way?

Suggested change

const { nickname, address, powerFlags: rawPowerFlags } = obj;

const powerFlags = rawPowerFlags || [];

const { nickname, address, powerFlags = [] } = obj;

wish: unit test

wish: pattern-based dynamic type check on obj

Turns out that obj.powerFlags can be null. The destructuring default values only work if obj.powerFlags is undefined.

Yeah, the dynamic checks would be better with patterns.

dckc · 2022-12-07T19:16:09Z

packages/vats/src/core/chain-behaviors.js

@@ -146,9 +147,24 @@ export const bridgeProvisioner = async ({
 async fromBridge(_srcID, obj) {


how does fromBridge here relate to the one in provisionPool.js? I'd like to see compare/contrast comments on each.

dckc · 2022-12-07T19:18:06Z

packages/solo/src/chain-cosmos-sdk.js

@@ -690,8 +690,7 @@ ${chainID} chain does not yet know of address ${clientAddr}${adviseEgress(
 console.debug(`helper said: ${stdout}`);
 const out = JSON.parse(stdout);

- out.code ||
- 0 ||
+ out.code === 0 ||


yikes. that looks like it was hairy to track down.

wish: unit test

mhofman · 2022-12-29T00:16:13Z

packages/vats/src/core/chain-behaviors.js

+ provisionP = E(bridgeManager).fromBridge(
+ BRIDGE_ID.PROVISION_SMART_WALLET,
+ obj,


Where does this message actually end up? I do not see any bridge handler registered for it since boot-psm sets walletBridgeId to BridgeId.PROVISION.

michaelfig added the cosmic-swingset package: cosmic-swingset label Nov 15, 2022

michaelfig requested a review from dckc November 15, 2022 02:13

michaelfig requested a review from turadg as a code owner November 15, 2022 02:13

michaelfig self-assigned this Nov 15, 2022

turadg reviewed Nov 15, 2022

View reviewed changes

dckc mentioned this pull request Nov 28, 2022

constrain mailbox to devnet config, to protect against c-list/liveslots DoS attacks #5965

Closed

dckc approved these changes Nov 28, 2022

View reviewed changes

michaelfig force-pushed the mfig-harmonise-pismo-boot branch from 06c2df6 to 65be26a Compare November 29, 2022 19:45

michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Nov 29, 2022

michaelfig force-pushed the mfig-harmonise-pismo-boot branch from 65be26a to 12ba2c6 Compare November 30, 2022 01:19

dckc mentioned this pull request Nov 30, 2022

Office Hours: Agoric Dev Agoric/documentation#576

Closed

michaelfig mentioned this pull request Dec 5, 2022

resolve conflicts between charter rework and devnet boot stuff #6617

Merged

mhofman added force:integration Force integration tests to run on PR and removed automerge:no-update (expert!) Automatically merge without updates labels Dec 5, 2022

michaelfig force-pushed the mfig-harmonise-pismo-boot branch 2 times, most recently from 81f7dbc to 77c5d31 Compare December 6, 2022 04:15

michaelfig added 10 commits December 7, 2022 10:33

feat(agoric-cli): generate decentral-economy-config.json

db88a76

feat(smart-wallet): preserve existing myAddressNameAdmin

8f283af

feat(vats): make walletFactory a coreProposal

ef1d6b9

feat(vats): harmonise core/boot.js and boot-psm.js

4f6635e

feat(cosmic-swingset): update scenario2 to post-Pismo

715a389

feat(vats): update devnet config to post-Pismo

3eca8d6

fix(smart-wallet): reenable provisioning options

55f3ab1

fix(cosmic-swingset): avoid overriding Makefile vat config

f0ce811

feat(inter-protocol): separate inviteCommitteMembers from inviteToCha…

4c5340d

…rter

feat(inter-protocol): add core proposal for psm governance

57f60de

michaelfig added 3 commits December 7, 2022 10:33

feat(inter-protocol): enable PSM core proposal

29e9f4f

test: add BridgeManager.fromBridge

d69dfa1

chore: PSMCharter->EconCharter

fa9c030

michaelfig force-pushed the mfig-harmonise-pismo-boot branch from 77c5d31 to 6759d4d Compare December 7, 2022 16:35

michaelfig added 2 commits December 7, 2022 11:12

fix(solo): unmangle assertion failure condition

0aafc66

fix(vats): tolerate null powerFlags

140986f

michaelfig force-pushed the mfig-harmonise-pismo-boot branch from ce038b9 to 140986f Compare December 7, 2022 17:13

dckc approved these changes Dec 7, 2022

View reviewed changes

michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Dec 7, 2022

mergify bot merged commit 8e71c07 into master Dec 7, 2022

mergify bot deleted the mfig-harmonise-pismo-boot branch December 7, 2022 19:36

turadg mentioned this pull request Dec 14, 2022

start-local-chain using manifest with economy-template #6671

Closed

dckc mentioned this pull request Dec 16, 2022

production bootstrap vat includes sim-chain tools such as connectFaucet #6687

Closed

2 tasks

mhofman reviewed Dec 29, 2022

View reviewed changes

dckc mentioned this pull request Jan 18, 2023

Governance charter deployment to existing chain #6598

Closed

	* @param options
	* @param {{ ROLE: 'chain' \| 'whatever else', econCommitteeOptions: ??? }} options

	const { [installGovAndPSMContracts.name]: _toss, ...manifest } =
	const { [installGovAndPSMContracts.name]: _, ...manifest } =


		return E(namesByAddressAdmin).update(address, nameHub, myAddressNameAdmin);
		// Don't clobber an existing myAddressNameAdmin or deposit facet.

		@@ -0,0 +1,30 @@
		import { makeHelpers } from '@agoric/deploy-script-support';

		export const defaultProposalBuilder = async ({ publishRef, install }) =>

		const coreConfigPath = path.resolve(
		'node_modules/@agoric/vats/decentral-core-config.json',

		const { nickname, address, powerFlags: rawPowerFlags } = obj;
		const powerFlags = rawPowerFlags \|\| [];

	const { nickname, address, powerFlags: rawPowerFlags } = obj;
	const powerFlags = rawPowerFlags \|\| [];
	const { nickname, address, powerFlags = [] } = obj;

		@@ -146,9 +147,24 @@ export const bridgeProvisioner = async ({
		async fromBridge(_srcID, obj) {

Harmonise core/boot-psm.js with core/boot.js #6568

Harmonise core/boot-psm.js with core/boot.js #6568

Conversation

michaelfig commented Nov 15, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

dckc commented Nov 15, 2022

michaelfig commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Nov 15, 2022

dckc commented Nov 19, 2022

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harmonise `core/boot-psm.js` with `core/boot.js` #6568

Harmonise `core/boot-psm.js` with `core/boot.js` #6568

michaelfig commented Nov 15, 2022 •

edited

Loading