chore(a3p-integration): Initialize dev-upgrade-15 #9242

gibson042 · 2024-04-16T19:43:39Z

Description

Bumps the cosmos upgrade handler and a3p-integration starting point.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

To be included in the full release.

Testing Considerations

a3p-integration

Upgrade Considerations

I'm pushing commits discretely to see how each interacts with CI.

Ref https://github.com/Agoric/agoric-sdk/releases/tag/agoric-upgrade-14

cloudflare-workers-and-pages · 2024-04-16T20:13:50Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`f447c44`
Status:	🚫 Build failed.

View logs

dckc

hm... I'd like to think / talk about where the tests go

dckc · 2024-04-18T16:06:16Z

a3p-integration/proposals/a:upgrade-14/ante-fees.test.js

-
-import { CHAINID, GOV1ADDR, GOV2ADDR, agd } from '@agoric/synthetic-chain';
-
-test(`ante handler sends fee only to vbank/reserve`, async t => {


we lose this test? that seems unfortunate

it seems a shame to go backwards w.r.t.

acceptance test of upgrade #8961

so it looks like last time (#8755) we were content to have the tests in agoric-3-proposals: https://github.com/Agoric/agoric-3-proposals/blob/main/proposals/71%3Aupgrade-14/ante-fees.test.js

But that only tests that the ante handler works in -14.
This feature is still relevant in 15, so it would be nice to keep it.

After an upgrade the tests move to agoric-3-proposal repo. In particular https://github.com/Agoric/agoric-3-proposals/blob/main/proposals/71%3Aupgrade-14/ante-fees.test.js

Acceptance tests are somewhat orthogonal

dckc · 2024-04-18T16:07:29Z

a3p-integration/proposals/a:upgrade-14/core-eval.test.js

-  return obj.values[0];
-};
-
-test(`core eval works`, async t => {


we lose this test too?

dckc · 2024-04-18T16:08:36Z

a3p-integration/proposals/a:upgrade-14/create-kread-item-test.sh

-
-agd query vstorage data published.wallet.$GOV1ADDR.current  -o json >& gov1.out
-name=`jq '.value | fromjson | .values[2] | fromjson | .body[1:] | fromjson | .purses[1].balance.value.payload[0][0].name ' gov1.out`
-test_val $name \"ephemeral_Ace\" "found KREAd character"


why is it OK to lose this test?

dckc · 2024-04-18T16:09:59Z

a3p-integration/proposals/a:upgrade-14/wallet-repairs.test.js

-  await writeFile(`${fileName}.js`, script);
-};
-
-test('smartWallet repairs', async t => {


hm. losing another test

dckc · 2024-04-18T16:20:21Z

a3p-integration/proposals/a:upgrade-15/test.sh

@@ -4,5 +4,3 @@
 # The effects of this step are not persisted in further proposal layers.

 yarn ava
-
-./create-kread-item-test.sh


as noted earlier, it's a shame to regress on #8961 like this

dckc · 2024-04-18T16:26:51Z

a3p-integration/proposals/a:upgrade-15/package.json

-    "releaseNotes": "https://github.com/Agoric/agoric-sdk/releases/tag/agoric-upgrade-14",
+    "releaseNotes": false,


so we're not ready to validate upgrade info. got it.

dckc · 2024-04-18T16:30:01Z

golang/cosmos/app/app.go

-	"agoric-upgrade-14":       true,
-	"agorictest-upgrade-14":   true,
-	"agorictest-upgrade-14-2": true,
+	"agoric-upgrade-15":     true,
+	"agorictest-upgrade-15": true,


this looks like the gist of this PR

dckc

after discussion with @gibson042 @Chris-Hibbert and co, I'm content to leave #8961 for another day

JimLarson

Reviewing only for the upgrade handler.

JimLarson · 2024-04-18T16:42:06Z

golang/cosmos/app/app.go

-				// Next revive KREAd characters
-				vm.CoreProposalStepForModules("@agoric/vats/scripts/revive-kread.js"),
-			}
+			CoreProposalSteps = []vm.CoreProposalStep{}
 		}



Commenting on lines 877-881 in the new version (silly github won't allow the comment to be added there). This seems like a leftover from upgrade-14 and should be removed, but you'll need to check with whoever added it. In general, we should probably maintain a core function of what should be in every upgrade handler, then the upgrade-N handler calls that and has its own functionality.

m := swingsetkeeper.NewMigrator(app.SwingSetKeeper) err = m.MigrateParams(ctx) if err != nil { return mvm, err }

I'm also not a fan of the "is first time" logic as it gets us into the dangerous game of not testing what will actually happen on mainnet, but I'll discuss that outside of this PR.

Commenting on lines 877-881 in the new version (silly github won't allow the comment to be added there). This seems like a leftover from upgrade-14 and should be removed, but you'll need to check with whoever added it.
…

m := swingsetkeeper.NewMigrator(app.SwingSetKeeper) err = m.MigrateParams(ctx) if err != nil { return mvm, err }

That block was introduced in upgrade-13 and preserved in upgrade-14, and would be relevant for any future parameter changes. I'm inclined to keep it.

In general, we should probably maintain a core function of what should be in every upgrade handler, then the upgrade-N handler calls that and has its own functionality.

💯

I'm also not a fan of the "is first time" logic as it gets us into the dangerous game of not testing what will actually happen on mainnet, but I'll discuss that outside of this PR.

Agreed, but it was necessary for upgrade-14-rc1 and I want to keep this block in case it comes up again.

I'm also not a fan of the "is first time" logic as it gets us into the dangerous game of not testing what will actually happen on mainnet, but I'll discuss that outside of this PR.

Agreed, but it was necessary for upgrade-14-rc1 and I want to keep this block in case it comes up again.

There is an issue to remove the need for this hack: #9101

I'm satisfied that the parameter migration is intentional - though other modules don't do this, so it would be good if x/swingset didn't need special treatment. I'll file an issue for that but approve this PR.

Filed #9255 to eventually remove the need for this.

mhofman

Couple nits, but the gist of it looks good!

mhofman · 2024-04-18T18:19:01Z

a3p-integration/proposals/a:upgrade-15/sanity.test.js

Consider porting the initial.test.js from #9204

Good idea; done.

mhofman · 2024-04-18T18:21:08Z

golang/cosmos/app/app.go

-				// Next revive KREAd characters
-				vm.CoreProposalStepForModules("@agoric/vats/scripts/revive-kread.js"),
-			}
+			CoreProposalSteps = []vm.CoreProposalStep{}


Consider adding an // Empty for now comment inside the struct to reduce future cherry-pick conflict resolution

initial.test.js copied from #9204, minus vats that are missing in the release-mainnet1B image.

chore(a3p-integration): Use the upgrade-14 Docker image tag

43c6d14

Ref https://github.com/Agoric/agoric-sdk/releases/tag/agoric-upgrade-14

gibson042 added the force:integration Force integration tests to run on PR label Apr 16, 2024

gibson042 force-pushed the gibson-init-upgrade-15 branch 4 times, most recently from d4a7313 to 7a7254c Compare April 17, 2024 02:55

gibson042 marked this pull request as ready for review April 17, 2024 14:31

gibson042 requested a review from mhofman April 17, 2024 14:31

gibson042 force-pushed the gibson-init-upgrade-15 branch 3 times, most recently from 8badc2c to 24d6ba5 Compare April 18, 2024 15:04

dckc reviewed Apr 18, 2024

View reviewed changes

dckc approved these changes Apr 18, 2024

View reviewed changes

JimLarson suggested changes Apr 18, 2024

View reviewed changes

mhofman reviewed Apr 18, 2024

View reviewed changes

JimLarson approved these changes Apr 18, 2024

View reviewed changes

gibson042 added 2 commits April 18, 2024 14:29

feat(cosmos): Next upgrade is agoric-upgrade-15

6a84bb1

ci: Initialize empty a3p tests for upgrade-15

e9cbd43

gibson042 force-pushed the gibson-init-upgrade-15 branch 2 times, most recently from 7511e7f to 5b39a30 Compare April 18, 2024 20:30

ci: Test incarnation number for multiple vats

f447c44

initial.test.js copied from #9204, minus vats that are missing in the release-mainnet1B image.

gibson042 force-pushed the gibson-init-upgrade-15 branch from 5b39a30 to f447c44 Compare April 18, 2024 20:46

gibson042 merged commit 92ce02b into dev-upgrade-15 Apr 18, 2024
63 of 65 checks passed

gibson042 deleted the gibson-init-upgrade-15 branch April 18, 2024 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(a3p-integration): Initialize dev-upgrade-15 #9242

chore(a3p-integration): Initialize dev-upgrade-15 #9242

gibson042 commented Apr 16, 2024

cloudflare-workers-and-pages bot commented Apr 16, 2024 •

edited

Loading

dckc left a comment

dckc Apr 18, 2024

dckc Apr 18, 2024

dckc Apr 18, 2024

mhofman Apr 18, 2024

dckc Apr 18, 2024

dckc Apr 18, 2024

dckc Apr 18, 2024

dckc Apr 18, 2024

dckc Apr 18, 2024

dckc Apr 18, 2024

dckc left a comment

JimLarson left a comment

JimLarson Apr 18, 2024 •

edited by gibson042

Loading

gibson042 Apr 18, 2024 •

edited

Loading

mhofman Apr 18, 2024

JimLarson Apr 18, 2024

JimLarson Apr 18, 2024

mhofman left a comment

mhofman Apr 18, 2024

gibson042 Apr 18, 2024

mhofman Apr 18, 2024


		import { CHAINID, GOV1ADDR, GOV2ADDR, agd } from '@agoric/synthetic-chain';

		test(`ante handler sends fee only to vbank/reserve`, async t => {

		"releaseNotes": "https://github.com/Agoric/agoric-sdk/releases/tag/agoric-upgrade-14",
		"releaseNotes": false,

chore(a3p-integration): Initialize dev-upgrade-15 #9242

chore(a3p-integration): Initialize dev-upgrade-15 #9242

Conversation

gibson042 commented Apr 16, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Apr 16, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

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

JimLarson left a comment

Choose a reason for hiding this comment

JimLarson Apr 18, 2024 • edited by gibson042 Loading

Choose a reason for hiding this comment

gibson042 Apr 18, 2024 • edited Loading

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

cloudflare-workers-and-pages bot commented Apr 16, 2024 •

edited

Loading

JimLarson Apr 18, 2024 •

edited by gibson042

Loading

gibson042 Apr 18, 2024 •

edited

Loading