Repair u19 tests in a3p-integration #10947

Chris-Hibbert · 2025-02-05T17:31:17Z

Description

In clean-up after upgrade-18, the upgrade 19 tests were left in an incomplete state. This ensures they are running again.

Security Considerations

Not a security issue.

Scaling Considerations

No implications.

Documentation Considerations

N/A

Testing and Upgrade Considerations

Our practice is to have a3p-integration represent upcoming upgrades to test them in the context of previous upgrades on chain. There were issues getting the U19 tests in proper condition before U18, and so revising them once U18 had been added to the chain was a little wonky. After this PR, The tests demonstrate that U19 will succeed, and the acceptance tests will verify the state of the chain after the upgrade.

cloudflare-workers-and-pages · 2025-02-05T21:00:44Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`05245ca`
Status:	✅ Deploy successful!
Preview URL:	https://b58413a6.agoric-sdk.pages.dev
Branch Preview URL:	https://10901-repair-u19.agoric-sdk.pages.dev

View logs

Chris-Hibbert · 2025-02-05T23:18:12Z

@turadg, j@dckc, I've added you both as reviewers but it should only take one of you. Do you want to decide which is more available?

a3p-integration/proposals/n:upgrade-next/assetReserve.test.js

turadg · 2025-02-06T02:00:01Z

a3p-integration/proposals/n:upgrade-next/initial.test.js

@@ -10,7 +10,7 @@ const vats = {
  orchestration: { incarnation: 1 },
  transfer: { incarnation: 2 },
  walletFactory: { incarnation: 6 },
-  zoe: { incarnation: 4 },
+  zoe: { incarnation: 3 },


how did this number go down?

I don't know for sure how the test passed with this number. My guess would be that the code last ran in an environment where a Zoe upgrade was included with U18 or the proto U19.

The lower number is correct following U18, and in a U19 that doesn't upgrade Zoe.

I think because upgrade.go somehow kept some u18 proposals around when it was rebased on top of u18.

turadg · 2025-02-06T02:00:26Z

a3p-integration/proposals/n:upgrade-next/mint-payment/send-script.tjs

@@ -24,8 +24,6 @@ const sendBankAsset = async powers => {
  const valueStr = '{{VALUE}}';
  const value = BigInt(valueStr)

-  console.log(`Start sendBankAsset for ${label}`);


seems useful but you'd know better since you've been debugging

turadg · 2025-02-06T02:00:56Z

a3p-integration/proposals/n:upgrade-next/mintHolder.test.js

 } from './test-lib/mintHolder-helpers.js';
 import { networkConfig } from './test-lib/index.js';

-test('mintHolder contract is upgraded', async t => {
+test('verify mintHolder contract upgrade', async t => {


nit: "test" is already a verb

Hmm. I read the string argument as the name of the test, rather than a noun phrase describing what is to be tested. In this case the test used to perform the upgrade (because the proposal was a coreEval), and now only verifies it (because it's now a software upgrade).

a3p-integration/proposals/n:upgrade-next/mintHolder.test.js

turadg · 2025-02-06T02:09:57Z

a3p-integration/proposals/n:upgrade-next/package.json

      "testing/add-USD-OLIVES.js agoricNamesCoreEvals/addUsdOlives",
-      "testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo",
-      "vats/upgrade-mintHolder.js upgrade-mintHolder A3P_INTEGRATION"
+      "testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo"


all these proposals for testing should be under test/ so they don't invalidate the "use" image. see,

copy 'test' files only in TEST stage agoric-3-proposals#213

Making that change affects .gitignore and .tsconfig but if you put them under a path like test/submissions then you can use that pathname for those files and not have to manage a list of submissions.

We'll make this easier eventually,

remove sdk-generate build step #10537

~~Actually they should be in host, and~~ their core eval output should be in test

Something like

Suggested change

"testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo"

"testing/publish-test-info.js test/agoricNamesCoreEvals/publishTestInfo"

FYI @Chris-Hibbert , the host thing hasn't landed in a3p. Up to you whether to wait for it to be merged and released to NPM so you can include that synthetic-chain lib version in this PR.

feat: proposal pre/post test steps agoric-3-proposals#210

I like your test/generated approach. It still copies to the image unnecessarily, but it's only the test image so it doesn't invalid any extra. (If the proposal changes its build would anyway). I think it's better to keep test stuff together than to spread it into another path.

Yeah you're right, a change in the proposal building would likely invalidate everything anyway, but collocating test files together is cleaner.

I got confused about the host stuff however, I forgot these first argument is relative to the SDK packages.

I moved the tests to test/. I was unable to move the test library under test (lint interpreted it as including test code, which is forbidden), so it remains at top-level as n:upgrade-next/test-lib. The host stuff seems to be related to agoric-3-proposals, not script building proposals in agoric-sdk.

The host stuff seems to be related to agoric-3-proposals, not script building proposals in agoric-sdk.

Yes sorry I got confused with the host stuff.

This looks great now

turadg · 2025-02-06T02:13:43Z

golang/cosmos/app/upgrade.go

-// 	if variant == "" {
-// 		return nil, nil
-// 	}
+func restartFeeDistributorCoreProposal(targetUpgrade string) (vm.CoreProposalStep, error) {


I'm not reviewing any of the Golang in this PR

packages/builders/scripts/inter-protocol/replace-feeDistributor-combo.js

turadg · 2025-02-06T02:19:13Z

packages/builders/scripts/inter-protocol/replace-feeDistributor-combo.js

+/** @type {import('@agoric/deploy-script-support/src/externalTypes.js').DeployScriptFunction} */
+export default async (homeP, endowments) => {
+  const { scriptArgs } = endowments;
+  const variantOrConfig = scriptArgs?.[0];


I'm not familiar with this convention. Please factor it out into a reusable function with documentation.

mhofman

It's a little unfortunate we have more need for variants. But since that's already the case for the mintHolder I won't block on that.

mhofman · 2025-02-07T00:02:11Z

a3p-integration/proposals/n:upgrade-next/package.json

      "testing/add-USD-OLIVES.js agoricNamesCoreEvals/addUsdOlives",
-      "testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo",
-      "vats/upgrade-mintHolder.js upgrade-mintHolder A3P_INTEGRATION"
+      "testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo"


~~Actually they should be in host, and~~ their core eval output should be in test

Something like

Suggested change

"testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo"

"testing/publish-test-info.js test/agoricNamesCoreEvals/publishTestInfo"

mhofman · 2025-02-07T00:04:38Z

a3p-integration/proposals/n:upgrade-next/psm.test.js

@@ -29,38 +27,19 @@ test.before(async t => {
  };
 });

-test.serial('simulate trade of IST and USDC', async t => {
+test.serial('check stats pre-swap', async t => {


I won't block on this, but we've been abusing test.serial. Its only guarantee is "not serial" and we've been relying on it being sequential.

You mean "not concurrent". I honestly didn't realize it didn't guarantee sequential.

mhofman · 2025-02-07T00:09:32Z

golang/cosmos/app/upgrade.go

+		builderScript,
+		"defaultProposalBuilder",
+		map[string]any{
+			"variant": variant,


Really unfortunate we still have the need for variants in this upgrade. We need to find a way out of this pickle, but I'm not gonna try to figure out why this upgrade needs special casing per network now.

In the mean time, we can hopefully prioritize #10837

Edit: filed #10956

I've read #10837 and don't understand the path forward.

This distinction is because we want to be able to validate the feeDistributor, and waiting a full hour-long cycle for it seems infeasible. We have other timing-related variations for auctions. In order to get rid of variant configurations for test, we need another solution.

I think my core question was why these config have to be provided as core proposal argument, and cannot be stored in the config of the contract we're upgrading. That's what #10956 asks. We can punt on it for now.

mhofman · 2025-02-07T00:10:03Z

golang/cosmos/app/upgrade.go

+            	// because of #10794, we need to do at least a null upgrade of
+            	// the walletFactory on every software upgrade


Looks like the comments needs formatting (weird indentation)

mhofman · 2025-02-07T00:11:17Z

a3p-integration/proposals/n:upgrade-next/initial.test.js

@@ -10,7 +10,7 @@ const vats = {
  orchestration: { incarnation: 1 },
  transfer: { incarnation: 2 },
  walletFactory: { incarnation: 6 },
-  zoe: { incarnation: 4 },
+  zoe: { incarnation: 3 },


I think because upgrade.go somehow kept some u18 proposals around when it was rebased on top of u18.

packages/builders/scripts/inter-protocol/replace-feeDistributor-combo.js

mhofman · 2025-02-07T00:15:30Z

packages/builders/scripts/inter-protocol/replace-feeDistributor-combo.js

+import { getManifestForReplaceFeeDistributor } from '@agoric/inter-protocol/src/proposals/replace-fee-distributor.js';
+import { SECONDS_PER_HOUR } from '@agoric/inter-protocol/src/proposals/econ-behaviors.js';
+
+const configurations = {


So I'll bite. Why do these config need to be hard coded in the builder? Can't they be restored from whatever params are currently on the respective chain, removing the need for variants?

The feeDistributor in a3p-integration currently has the same parameter settings as on mainNet. We want to run tests that verify that fees are distributed. Without a change, the test would have to wait an hour. We make similar changes with changes to timing for tests for other vats, like auctions.

And we can't update these parameters outside of the core proposal, through a core eval or other governance vote, either before the upgrade, or after in the test?

FeeDistributor is not a governed contract. (!?!) The collectionInterval can only be modified by restarting/upgrading the contract.

Could the contract store the interval in baggage so it can have a value to start from if not in vat params?

Could the contract store the interval in baggage so it can have a value to start from if not in vat params?

It sounds like you're proposing a poor person's fallback to replace governed params. If it's worth making any change here, I'd just make it a governed contract. The contract has no state that we need to preserve.

turadg · 2025-02-07T23:25:56Z

a3p-integration/proposals/n:upgrade-next/test/test-lib/mintHolder-helpers.js

@@ -62,6 +62,12 @@ export const mintPayment = async (t, address, assetList, value) => {

  for (const asset of assetList) {
    const { label, denom } = asset;
+
+    // XXX I don't know what asset shows up like this, but let's ignore it.


please indicate why it's okay to ignore

It was BLD. I'll filter that out.

It wasn't BLD, it was testvbankAsset.

turadg · 2025-02-07T23:30:17Z

a3p-integration/proposals/n:upgrade-next/package.json

      "testing/add-USD-OLIVES.js agoricNamesCoreEvals/addUsdOlives",
-      "testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo",
-      "vats/upgrade-mintHolder.js upgrade-mintHolder A3P_INTEGRATION"
+      "testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo"


I like your test/generated approach. It still copies to the image unnecessarily, but it's only the test image so it doesn't invalid any extra. (If the proposal changes its build would anyway). I think it's better to keep test stuff together than to spread it into another path.

turadg · 2025-02-11T00:21:37Z

a3p-integration/proposals/n:upgrade-next/test-lib/mintHolder-helpers.js

+    labelList ||
+    Object.values(vbankAssets)
+      .map(asset => asset.issuerName)
+      .filter(name => name !== undefined); // testvbankAsset can be malformed.


this is idiomatic:

Suggested change

.filter(name => name !== undefined); // testvbankAsset can be malformed.

.filter(Boolean);

Chris-Hibbert · 2025-02-11T00:22:18Z

@mhofman wrote:

Actually they should be in host, and their core eval output should be in test

I moved the output under test. I'm unfamiliar with the host convention. The rule I was following is that code that is not itself a test, but is only used by test code can go in a testing directory adjacent to src. We don't actually have src in builders/scripts, but all the non-prod code is under testing. What kind of code would go in host?

generated proposals into .../generated

mergify · 2025-02-11T17:14:57Z

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #10947 has been dequeued. The pull request rule doesn't match anymore. The following conditions don't match anymore:

label=automerge:rebase
any of: [🔀 queue conditions]
- all of: [📌 queue conditions of queue rebase]
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-zoe-swingset (xs)
    - check-skipped = test-zoe-swingset (xs)
    - check-success = test-zoe-swingset (xs)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = lint-primary
    - check-skipped = lint-primary
    - check-success = lint-primary
  - any of: [🛡 GitHub branch protection]
    - check-neutral = lint-rest
    - check-skipped = lint-rest
    - check-success = lint-rest
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-cosmic-swingset (18.x)
    - check-skipped = test-cosmic-swingset (18.x)
    - check-success = test-cosmic-swingset (18.x)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-quick (18.x)
    - check-skipped = test-quick (18.x)
    - check-success = test-quick (18.x)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-quick2 (18.x)
    - check-skipped = test-quick2 (18.x)
    - check-success = test-quick2 (18.x)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-zoe-unit (18.x)
    - check-skipped = test-zoe-unit (18.x)
    - check-success = test-zoe-unit (18.x)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-zoe-swingset (18.x)
    - check-skipped = test-zoe-swingset (18.x)
    - check-success = test-zoe-swingset (18.x)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-inter-protocol (18.x)
    - check-skipped = test-inter-protocol (18.x)
    - check-success = test-inter-protocol (18.x)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-boot (18.x, 0, 4)
    - check-skipped = test-boot (18.x, 0, 4)
    - check-success = test-boot (18.x, 0, 4)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-boot (18.x, 1, 4)
    - check-skipped = test-boot (18.x, 1, 4)
    - check-success = test-boot (18.x, 1, 4)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-boot (18.x, 2, 4)
    - check-skipped = test-boot (18.x, 2, 4)
    - check-success = test-boot (18.x, 2, 4)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-boot (18.x, 3, 4)
    - check-skipped = test-boot (18.x, 3, 4)
    - check-success = test-boot (18.x, 3, 4)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-boot (xs, 0, 4)
    - check-skipped = test-boot (xs, 0, 4)
    - check-success = test-boot (xs, 0, 4)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-boot (xs, 1, 4)
    - check-skipped = test-boot (xs, 1, 4)
    - check-success = test-boot (xs, 1, 4)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-boot (xs, 2, 4)
    - check-skipped = test-boot (xs, 2, 4)
    - check-success = test-boot (xs, 2, 4)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-boot (xs, 3, 4)
    - check-skipped = test-boot (xs, 3, 4)
    - check-success = test-boot (xs, 3, 4)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-swingset (xs, 0, 5)
    - check-skipped = test-swingset (xs, 0, 5)
    - check-success = test-swingset (xs, 0, 5)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-swingset (xs, 1, 5)
    - check-skipped = test-swingset (xs, 1, 5)
    - check-success = test-swingset (xs, 1, 5)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-swingset (xs, 2, 5)
    - check-skipped = test-swingset (xs, 2, 5)
    - check-success = test-swingset (xs, 2, 5)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-swingset (xs, 3, 5)
    - check-skipped = test-swingset (xs, 3, 5)
    - check-success = test-swingset (xs, 3, 5)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-swingset (xs, 4, 5)
    - check-skipped = test-swingset (xs, 4, 5)
    - check-success = test-swingset (xs, 4, 5)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-swingset (18.x, 0, 5)
    - check-skipped = test-swingset (18.x, 0, 5)
    - check-success = test-swingset (18.x, 0, 5)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-swingset (18.x, 1, 5)
    - check-skipped = test-swingset (18.x, 1, 5)
    - check-success = test-swingset (18.x, 1, 5)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-swingset (18.x, 2, 5)
    - check-skipped = test-swingset (18.x, 2, 5)
    - check-success = test-swingset (18.x, 2, 5)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-swingset (18.x, 3, 5)
    - check-skipped = test-swingset (18.x, 3, 5)
    - check-success = test-swingset (18.x, 3, 5)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = test-swingset (18.x, 4, 5)
    - check-skipped = test-swingset (18.x, 4, 5)
    - check-success = test-swingset (18.x, 4, 5)
  - any of: [🛡 GitHub branch protection]
    - check-neutral = dependency-graph
    - check-skipped = dependency-graph
    - check-success = dependency-graph

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: @mergifyio requeue

Chris-Hibbert · 2025-02-11T17:17:26Z

I started the merge, but then noticed additional comments I hadn't seen before, so I dropped the merge tag. I hope I'll be able to review and address these comments this morning.

Chris-Hibbert added deployment Chain deployment mechanism (e.g. testnet) test Testnet contract-upgrade labels Feb 5, 2025

Chris-Hibbert self-assigned this Feb 5, 2025

Chris-Hibbert requested a review from a team as a code owner February 5, 2025 17:31

Chris-Hibbert requested a review from AgoricTriage February 5, 2025 17:31

Chris-Hibbert marked this pull request as draft February 5, 2025 17:31

Chris-Hibbert force-pushed the 10901-repair-U19 branch from 6b1d701 to f7dfbac Compare February 5, 2025 21:00

Chris-Hibbert added the force:integration Force integration tests to run on PR label Feb 5, 2025

Chris-Hibbert marked this pull request as ready for review February 5, 2025 21:01

Chris-Hibbert requested review from turadg and dckc February 5, 2025 23:16

turadg reviewed Feb 6, 2025

View reviewed changes

mhofman reviewed Feb 7, 2025

View reviewed changes

turadg mentioned this pull request Feb 7, 2025

feat: proposal pre/post test steps Agoric/agoric-3-proposals#210

Merged

turadg approved these changes Feb 7, 2025

View reviewed changes

dckc removed their request for review February 10, 2025 18:43

Chris-Hibbert force-pushed the 10901-repair-U19 branch from f351299 to d89dde5 Compare February 10, 2025 18:43

turadg reviewed Feb 11, 2025

View reviewed changes

Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Feb 11, 2025

Chris-Hibbert added 6 commits February 11, 2025 17:06

test: migrate u19 tests to upgrade-next; update upgrade.go

23defb7

test: repair feeDistributor test: allow config from upgrade.go

271c55f

test: repair mintHolder test: add sendScript & exclude fastUSDC

1435915

test: drop redundant bundles in assetReserve test

e00d6b7

test: update PSM test

871a62b

chore: drop an unused import

c480961

Chris-Hibbert added 9 commits February 11, 2025 17:06

test: move a3p-integration test files under .../test/

5b745b5

generated proposals into .../generated

refactor: clean-ups in proposals

6c59c3d

refactor: extract common method for parsing scriptArgs

e2ea272

chore: add configurations for Devnet and Emerynet for feeDistributor

4f87fa6

chore: gofmt upgrade.go

fc25266

test: move test/test-lib to test-lib. Lint dislikes test imports

1eb6ecd

chore: export parseScriptArgs from helpers.js

fab48f9

chore: remove optional chaining in code used by loadgen

d48bd2f

test: suppress empty assets at a better point

05245ca

Chris-Hibbert force-pushed the 10901-repair-U19 branch from 82ab9eb to 05245ca Compare February 11, 2025 17:07

Chris-Hibbert removed the automerge:rebase Automatically rebase updates, then merge label Feb 11, 2025

Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Feb 11, 2025

mergify bot merged commit 46061c3 into master Feb 11, 2025
101 of 115 checks passed

mergify bot deleted the 10901-repair-U19 branch February 11, 2025 18:17

Chris-Hibbert mentioned this pull request Feb 12, 2025

Excise reallocate() #10871

Open

mhofman mentioned this pull request Feb 12, 2025

Remove dependency on variants in proposal builders #10956

Open

	"testing/publish-test-info.js agoricNamesCoreEvals/publishTestInfo"
	"testing/publish-test-info.js test/agoricNamesCoreEvals/publishTestInfo"

		// because of #10794, we need to do at least a null upgrade of
		// the walletFactory on every software upgrade

	.filter(name => name !== undefined); // testvbankAsset can be malformed.
	.filter(Boolean);

Repair u19 tests in a3p-integration #10947

Repair u19 tests in a3p-integration #10947

Conversation

Chris-Hibbert commented Feb 5, 2025

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing and Upgrade Considerations

cloudflare-workers-and-pages bot commented Feb 5, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Chris-Hibbert commented Feb 5, 2025

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 Feb 7, 2025 • 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

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

mhofman Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman Feb 7, 2025 • 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

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

Chris-Hibbert commented Feb 11, 2025

mergify bot commented Feb 11, 2025

Chris-Hibbert commented Feb 11, 2025

cloudflare-workers-and-pages bot commented Feb 5, 2025 •

edited

Loading

mhofman Feb 7, 2025 •

edited

Loading

mhofman Feb 7, 2025 •

edited

Loading

mhofman Feb 7, 2025 •

edited

Loading