test: separate upgrade-12 #8254

turadg · 2023-08-25T19:29:43Z

Description

upgrade-11 is on the release train with less than master it testing as part of upgrade 11. In particular, #8147 added to the agoric-upgrade-11 test that smartWallet gets upgraded, but it won't be until upgrade 12. Additionally, the Zoe changes were in the actions, though they were disabled in the test (until #8121)

This trims down the upgrade-11 test to only what went out in upgrade-11. The rest it bumps up to upgrade-12.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

dckc

I started reviewing before I realized this was draft. I hope early feedback is helpful.

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-11/test.sh

dckc · 2023-08-25T19:31:36Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/env_setup.sh

+# agoric-upgrade-12 specific env here...
+export USER2ADDR=$($binary keys show user2 -a --keyring-backend="test" 2> /dev/null)
+
+printKeys() {


more bash. le sigh

dckc · 2023-08-25T19:34:23Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/env_setup.sh

+  echo "========== GOVERNANCE KEYS =========="
+}
+
+pushPrice () {


note for @iomekam :

I think we have agops oracle setPrice --keys=gov1,gov2 in this context. It obviates the weird stuff this code does.

dckc · 2023-08-25T19:34:54Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/env_setup.sh

+  fi
+}
+
+export_genesis() {


I kinda thought this was an 11 thing

we may not need to perform these tests again in upgrade 12, in which case we can remove.

dckc · 2023-08-25T19:35:11Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/env_setup.sh

+  agd export --export-dir "$GENESIS_EXPORT_DIR" $GENESIS_HEIGHT_ARG "$@"
+}
+
+make_swing_store_snapshot() {( set -euo pipefail


mhofman

preliminary review

mhofman · 2023-08-25T20:00:31Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/env_setup.sh

+  fi
+}
+
+export_genesis() {


we may not need to perform these tests again in upgrade 12, in which case we can remove.

...t/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/zoe-full-upgrade/zcf-upgrade-script.js

mhofman · 2023-08-25T20:10:56Z

packages/deployment/upgrade-test/Dockerfile

-#this is agoric-upgrade-11 / vaults+1
-FROM ${DEST_IMAGE} as agoric-upgrade-11
+# this is agoric-upgrade-12 supporting wallets with non-vbank assets
+FROM ${DEST_IMAGE} as agoric-upgrade-12
 ARG BOOTSTRAP_MODE
-ENV THIS_NAME=agoric-upgrade-11 BOOTSTRAP_MODE=${BOOTSTRAP_MODE}
+ENV THIS_NAME=agoric-upgrade-12 BOOTSTRAP_MODE=${BOOTSTRAP_MODE}


don't we need to add FROM for agoric-upgrade-11 ? I think ghcr.io/agoric/agoric-sdk:35 is the upgrade-11 rc0 right now.

iomekam · 2023-08-25T20:14:02Z

packages/deployment/upgrade-test/Dockerfile

-#this is agoric-upgrade-11 / vaults+1
-FROM ${DEST_IMAGE} as agoric-upgrade-11
+# this is agoric-upgrade-12 supporting wallets with non-vbank assets
+FROM ${DEST_IMAGE} as agoric-upgrade-12


This change seems to be removing agoric-upgrade-11 entirely, when I'd imagine we want to agoric-upgrade-12 to build on top of it like the other upgrades?

packages/deployment/upgrade-test/Dockerfile

mhofman · 2023-08-28T22:41:27Z

packages/deployment/upgrade-test/Dockerfile

+
+# UPGRADE block
+ARG DEST_IMAGE
+FROM ghcr.io/agoric/agoric-sdk:36 as agoric-upgrade-11-to-12


Shouldn't this use DEST_IMAGE ?

Oh my bad, I talked to @michaelfig and he explained to me that this is just running the governance proposal for the upgrade, which does need to run in the previous version. Maybe a comment to that effect would help future readers?

@turadg, if you're still working on this, maybe the right thing to do is to globally rename agoric-upgrade-10-to-11 to propose-agoric-upgrade-11 and similar for 11-to-12?

I like that!

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/pre_test.sh

mhofman · 2023-08-28T22:45:15Z

golang/cosmos/app/app.go

+	const (
+		upgradeName     = "agoric-upgrade-12"
+		upgradeNameTest = "agorictest-upgrade-12"
+	)
+


Not a golang specialist, but I think const are preferred declared top of a module

If it were just stylistic I agree. In this case the file is quite large and the upgrade handler is just one small part of it. I think the handler should be factored out but that’s out of scope. Meanwhile it’s helpful for the reader to see that those constants are just for the two places in use and not general configuration constants. Ie you can’t change them independently of the code.

I see the change has happened. Not worth spinning CI again.

mhofman · 2023-08-28T22:47:10Z

I'd like either @raphdev or @michaelfig to take a look at the Dockerfile changes as I don't feel qualified enough.

dckc · 2023-08-29T00:11:16Z

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/test.sh

+}
+
+# Check brand aux data for more than just vbank assets
+testMinChildren published.boardAux 3


I don't see an actions thing for upgrade-12. I suspect this will fail :)

packages/deployment/upgrade-test/Dockerfile

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/actions.sh

packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/pre_test.sh

dckc · 2023-08-31T17:05:44Z

@iomekam notes this is blocked on ci failure in master but otherwise ready to go.

turadg · 2023-08-31T17:12:11Z

Thanks @iomekam for taking this over. I've reviewed the current state and Approve. Though consider reverting the constants move since you have to rebase and run CI anyway.

mhofman · 2023-08-31T17:54:34Z

@iomekam notes this is blocked on ci failure in master but otherwise ready to go.

I believe master is green, please update the branch.

mhofman · 2023-08-31T20:14:31Z

packages/deployment/upgrade-test/Dockerfile

 ARG DEST_IMAGE
 #this is agoric-upgrade-10 upgrading to 11
 #it's a separate target because agoric-upgrade-10 takes so long to test
-FROM ghcr.io/agoric/agoric-sdk:35 as agoric-upgrade-10-to-11
+FROM ghcr.io/agoric/agoric-sdk:35 as propose-agoric-upgrade-11
 # This default UPGRADE_INFO_11 is to test core proposals like the network vat.
 # TODO: Maybe replace with a Zoe core proposal, or remove when other paths test it.
 ARG BOOTSTRAP_MODE UPGRADE_INFO_11='{"coreProposals":["@agoric/builders/scripts/vats/init-network.js"]}'


Just noticed UPGRADE_INFO_11 should have been restored to empty.

test: separate upgrade-12

turadg requested review from dckc and iomekam August 25, 2023 19:29

dckc reviewed Aug 25, 2023

View reviewed changes

mhofman reviewed Aug 25, 2023

View reviewed changes

iomekam reviewed Aug 25, 2023

View reviewed changes

turadg mentioned this pull request Aug 25, 2023

refactor: clarify upgrade builds #8259

Draft

turadg force-pushed the ta/upgrade-12 branch 2 times, most recently from 03933a9 to 125a9ce Compare August 28, 2023 21:47

turadg added the force:integration Force integration tests to run on PR label Aug 28, 2023

turadg marked this pull request as ready for review August 28, 2023 22:15

turadg requested review from mhofman, dckc and iomekam August 28, 2023 22:34

mhofman reviewed Aug 28, 2023

View reviewed changes

mhofman removed the force:integration Force integration tests to run on PR label Aug 28, 2023

turadg force-pushed the ta/upgrade-12 branch from 125a9ce to a2d0b28 Compare August 28, 2023 22:59

mhofman added the force:integration Force integration tests to run on PR label Aug 28, 2023

dckc reviewed Aug 29, 2023

View reviewed changes

iomekam reviewed Aug 29, 2023

View reviewed changes

dckc assigned iomekam Aug 31, 2023

iomekam force-pushed the ta/upgrade-12 branch from c873439 to 82ae4cb Compare August 31, 2023 18:00

test: separate upgrade-12

b055047

iomekam force-pushed the ta/upgrade-12 branch from f828dcf to b055047 Compare August 31, 2023 18:03

iomekam approved these changes Aug 31, 2023

View reviewed changes

iomekam added the automerge:rebase Automatically rebase updates, then merge label Aug 31, 2023

mergify bot merged commit 9f085d3 into master Aug 31, 2023
79 checks passed

mergify bot deleted the ta/upgrade-12 branch August 31, 2023 19:20

mhofman reviewed Aug 31, 2023

View reviewed changes

dckc mentioned this pull request Sep 1, 2023

chore: clean up UPGRADE_INFO_11 copy-pasta #8295

Closed

mhofman pushed a commit that referenced this pull request Nov 8, 2023

Merge pull request #8254 from Agoric/ta/upgrade-12

ae6e6d5

test: separate upgrade-12

mhofman pushed a commit that referenced this pull request Nov 10, 2023

Merge pull request #8254 from Agoric/ta/upgrade-12

c88f00b

test: separate upgrade-12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: separate upgrade-12 #8254

test: separate upgrade-12 #8254

turadg commented Aug 25, 2023

dckc left a comment

dckc Aug 25, 2023

dckc Aug 25, 2023

dckc Aug 25, 2023

mhofman Aug 25, 2023

dckc Aug 25, 2023

mhofman left a comment

mhofman Aug 25, 2023

mhofman Aug 25, 2023

iomekam Aug 25, 2023

mhofman Aug 28, 2023

mhofman Aug 28, 2023

michaelfig Aug 29, 2023

turadg Aug 29, 2023

mhofman Aug 28, 2023

turadg Aug 31, 2023

mhofman commented Aug 28, 2023

dckc Aug 29, 2023

dckc commented Aug 31, 2023

turadg commented Aug 31, 2023

mhofman commented Aug 31, 2023

mhofman Aug 31, 2023

test: separate upgrade-12 #8254

test: separate upgrade-12 #8254

Conversation

turadg commented Aug 25, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

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

mhofman 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

mhofman commented Aug 28, 2023

Choose a reason for hiding this comment

dckc commented Aug 31, 2023

turadg commented Aug 31, 2023

mhofman commented Aug 31, 2023

Choose a reason for hiding this comment