Wallet factory upgrade 2 branch #8593

mhofman · 2023-12-01T17:23:12Z

Description

This prepares the dev-upgrade-wallet-factory-2 branch for the wallet factory core eval. The branch is based on release-mainnet1B after agoric-upgrade-13.

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Docker upgrade tests are empty in this branch for lack of core eval testing framework

Upgrade Considerations

The dev branch should be merged back to the release-mainnet1B branch before generating the release artifacts.

turadg · 2023-12-01T17:28:47Z

packages/deployment/upgrade-test/Dockerfile

@@ -1,27 +1,20 @@
 # Defaults
-ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals@sha256:ac10c09b5927d759d37b1525b5a0bc4aeb3034df8d0f0fbb15b51203555ffb1b
+ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals:pr33


Agoric/agoric-3-proposals#33 merged. I guess this isn't main because that flow is failing?

Please leave a comment here about why it isn't main as it should be. We should unbreak that.

Suggested change

ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals:pr33

// Once https://github.com/Agoric/agoric-3-proposals/pull/37 merges, s/pr33/main

ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals:pr33

Even if it was pushed to the main tag, I do not feel confident using main until Agoric/agoric-3-proposals#25 is resolved.

I think your concern is about this CI starting to fail once main includes the upgrade being tested here.

So even after 25, you wouldn't want :main, right? You'd want :upgrade12 or something like that.

Given that I propose a different comment.

Suggested change

ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals:pr33

# Don't use `:main` because once this upgrade is incorporated into it, this test would break

ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals:pr33

turadg · 2023-12-01T17:29:57Z

packages/deployment/upgrade-test/Dockerfile


 # DEST (TEST)
-#this is agoric-upgrade-12 / multi-collateral, etc.
+#this is wallet-factory-upgrade


we don't need to distinguish now that there's one DEST

Suggested change

#this is wallet-factory-upgrade

turadg · 2023-12-01T17:31:51Z

packages/deployment/upgrade-test/Dockerfile

 ARG DEST_IMAGE
-FROM ${DEST_IMAGE} as agoric-upgrade-12
+FROM ${DEST_IMAGE} as wallet-factory-upgrade


similarly, this stage doesn't need a name now

Suggested change

FROM ${DEST_IMAGE} as wallet-factory-upgrade

FROM ${DEST_IMAGE}

I think the name is still used as a target for the Makefile, which I haven't changed that drastically.

I see. We can clean this up when migrating on top of #8635

turadg · 2023-12-01T17:32:47Z

packages/deployment/upgrade-test/Dockerfile

+# BASE
+FROM ${BASE_IMAGE} as base-wallet-factory-upgrade


this isn't doing any work now. better to inline…

Inlining didn't work. Apparently COPY --from doesn't like variables...

ah. then how about,

Suggested change

# BASE

FROM ${BASE_IMAGE} as base-wallet-factory-upgrade

# BASE stage for the `COPY --from` below

FROM ${BASE_IMAGE} as base

packages/deployment/upgrade-test/Dockerfile

turadg · 2023-12-01T17:33:58Z

packages/deployment/upgrade-test/Makefile

-agoric-upgrade-12: propose-agoric-upgrade-12
-	$(BUILD) --target agoric-upgrade-12 -t $(REPOSITORY):agoric-upgrade-12$(TAG_SUFFIX)
+wallet-factory-upgrade:
+	$(BUILD) --target wallet-factory-upgrade -t $(REPOSITORY):wallet-factory-upgrade


being last, it's the default target

Suggested change

$(BUILD) --target wallet-factory-upgrade -t $(REPOSITORY):wallet-factory-upgrade

$(BUILD) -t $(REPOSITORY):wallet-factory-upgrade

turadg · 2023-12-01T17:35:35Z

packages/deployment/upgrade-test/upgrade-test-scripts/wallet-factory-upgrade/actions.test.js

+test.before(async t => {
+  await mintIST(GOV1ADDR, 12340000000, 10000, 2000);
+});
+
+test.skip('Open Vaults', async t => {
+  const currentVaults = await agops.vaults('list', '--from', GOV1ADDR);
+  t.is(currentVaults.length, 5);
+
+  // TODO get as return value from openVault
+  const vaultId = 'vault6';
+  await openVault(GOV1ADDR, 7, 11);
+  await adjustVault(GOV1ADDR, vaultId, { giveMinted: 1.5 });
+  await adjustVault(GOV1ADDR, vaultId, { giveCollateral: 2.0 });
+  await closeVault(GOV1ADDR, vaultId, 5.75);
+});


there are no actions to perform.

this test and its prep should move to post.test.js

dckc

I think I haven't learned how Dockerfiles work - especially this one - well enough to review this.

If it's important that I review this more thoroughly, let's please discuss it.

dckc · 2023-12-01T17:28:31Z

packages/deployment/upgrade-test/Dockerfile

-ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals@sha256:ac10c09b5927d759d37b1525b5a0bc4aeb3034df8d0f0fbb15b51203555ffb1b
+ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals:pr33


:main isn't on target for this?

pr33, while more readable than a hash, seems somewhat fragile. "pr33" in a comment and a hash in the code would make more sense to me.

not critical

packages/deployment/upgrade-test/Dockerfile

mhofman · 2023-12-14T22:27:16Z

packages/deployment/upgrade-test/Dockerfile

@@ -1,25 +1,23 @@
 # Defaults
+# Use a stable base image to prevent this test from breaking when
+# new upgrade proposals are added to a3p.
+# TODO: pr-33 is upgrade-12, use the upgrade-13 PR once it exists


Still working on the upgrade-13 PR

turadg

No concerns here. I expect this will reach a3p before being proposed on agoric-3, and that will use the design in #8635

turadg · 2023-12-14T23:09:51Z

packages/deployment/upgrade-test/Dockerfile

 ARG DEST_IMAGE
-FROM ${DEST_IMAGE} as agoric-upgrade-12
+FROM ${DEST_IMAGE} as wallet-factory-upgrade


I see. We can clean this up when migrating on top of #8635

mhofman · 2023-12-14T23:17:22Z

I expect this will reach a3p before being proposed on agoric-3, and that will use the design in #8635

We would need to cherry-pick #8635 into the release branch, which I'm totally ok with, but right now I don't want to block on that. So I expect this to land on the release branch first.

turadg

I clicked the wrong radio button somehow.

No concerns here

That means I approve :)

mhofman added the force:integration Force integration tests to run on PR label Dec 1, 2023

mhofman requested review from turadg and dckc December 1, 2023 17:23

mhofman changed the title ~~Mhofman/8578 wallet factory 2~~ Wallet factory upgrade 2 branch Dec 1, 2023

turadg requested changes Dec 1, 2023

View reviewed changes

dckc reviewed Dec 1, 2023

View reviewed changes

This was referenced Dec 4, 2023

smartWallet stops updating purse balance on upgrade #8293

Closed

smartWallet exits offers (e.g. bids) on zoe upgrade #8292

Closed

mhofman force-pushed the mhofman/8578-wallet-factory-2 branch from e00e2c9 to ba752a6 Compare December 14, 2023 22:26

mhofman commented Dec 14, 2023

View reviewed changes

mhofman requested a review from turadg December 14, 2023 22:48

turadg reviewed Dec 14, 2023

View reviewed changes

turadg approved these changes Dec 14, 2023

View reviewed changes

ci: test loadgen on node 18.18

845360e

mhofman mentioned this pull request Dec 15, 2023

Upgrade 13 Agoric/agoric-3-proposals#47

Merged

mhofman force-pushed the mhofman/8578-wallet-factory-2 branch from 9476fb0 to 552879a Compare December 15, 2023 03:16

chore(upgrade-test): rename upgrade and remove unneeded actions

f1732a4

mhofman force-pushed the mhofman/8578-wallet-factory-2 branch from 552879a to f1732a4 Compare December 15, 2023 15:06

mhofman merged commit c044454 into dev-upgrade-wallet-factory-2 Dec 15, 2023
60 checks passed

mhofman deleted the mhofman/8578-wallet-factory-2 branch December 15, 2023 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wallet factory upgrade 2 branch #8593

Wallet factory upgrade 2 branch #8593

mhofman commented Dec 1, 2023 •

edited

Loading

turadg Dec 1, 2023

mhofman Dec 1, 2023

turadg Dec 1, 2023

turadg Dec 1, 2023

turadg Dec 1, 2023

mhofman Dec 1, 2023

turadg Dec 14, 2023

turadg Dec 1, 2023

mhofman Dec 1, 2023

turadg Dec 1, 2023

turadg Dec 1, 2023

turadg Dec 1, 2023

dckc left a comment

dckc Dec 1, 2023

mhofman Dec 14, 2023

turadg left a comment

turadg Dec 14, 2023

mhofman commented Dec 14, 2023

turadg left a comment

	ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals:pr33
	// Once https://github.com/Agoric/agoric-3-proposals/pull/37 merges, s/pr33/main
	ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals:pr33

	ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals:pr33
	# Don't use `:main` because once this upgrade is incorporated into it, this test would break
	ARG BASE_IMAGE=ghcr.io/agoric/agoric-3-proposals:pr33

	FROM ${DEST_IMAGE} as wallet-factory-upgrade
	FROM ${DEST_IMAGE}

	$(BUILD) --target wallet-factory-upgrade -t $(REPOSITORY):wallet-factory-upgrade
	$(BUILD) -t $(REPOSITORY):wallet-factory-upgrade

Wallet factory upgrade 2 branch #8593

Wallet factory upgrade 2 branch #8593

Conversation

mhofman commented Dec 1, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Dec 14, 2023

turadg left a comment

Choose a reason for hiding this comment

mhofman commented Dec 1, 2023 •

edited

Loading