backport walletFactory improvements on top of upgrade-11 branch #8311

dckc · 2023-09-06T18:30:12Z

closes: #8305
refs: #8308 (alternative approach), #8156

Description / Testing / Upgrade Considerations

back-port #8071, #8147 to the release-mainnet1B branch in preparation for upgrading walletFactory by way of a core-eval on top of agoric-upgrade-11.

The first 6 commits are cherry-picked from feat(smart-wallet): trading in well-known non-vbank assets #8071 directly.
The 7th comes from feat: upgrade walletFactory to go beyond vbank assets #8147 but with non-trivial changes because, for example, it has files in the builders package that doesn't exist in the mainnet branch.
The last few commits are administrative details such as a yarn.lock update due to an additional devDependency in packages/smart-wallet: import-meta-resolve, and some changes in our coding style.

Note: This is a long way from testing that every aspect of walletFactory remains working after an upgrade. It's just the same tests that were judged sufficient to address #8156.

Security / Scaling Considerations

Nothing new here.

Documentation Considerations

A discussion of the upgrade proposal should be shared via community.agoric.com.

dckc · 2023-09-06T20:49:28Z

I don't understand why yarn.lock is changing. I didn't make any package dependency changes, did I?

Changes not staged for commit:
	modified:   yarn.lock

https://github.com/Agoric/agoric-sdk/actions/runs/6101013890/job/16556480500?pr=8311#step:3:525

mhofman · 2023-09-06T20:56:00Z

I don't understand why yarn.lock is changing. I didn't make any package dependency changes, did I?

It's possibly an artifact of the cherry-pick that happened previously in this branch

Edit: nope there is a dep change

mhofman · 2023-09-06T20:57:35Z

packages/smart-wallet/package.json

    "@endo/captp": "^3.1.1",
    "@endo/init": "^0.5.56",
-    "ava": "^5.2.0"
+    "ava": "^5.2.0",
+    "import-meta-resolve": "^2.2.1"


Here is the dependency change

packages/smart-wallet/package.json

mhofman · 2023-09-06T21:09:43Z

packages/smart-wallet/test/test-addAsset.js

+const importSpec = spec =>
+  importMetaResolve(spec, import.meta.url).then(u => new URL(u).pathname);


I think it would be nice to avoid taking on an import-meta-resolve dependency just for this.

I'm not aware of an alternative. It's pretty normal for tests that use bundleSource, no?

In master, I think the dependency was/is already there.

iomekam · 2023-09-07T16:28:13Z

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

@@ -0,0 +1,2 @@
+upgrade-walletFactory-permit.json


Instead of adding these as ignores, can we move them to the tmp directory after agoric run produces the files?

I suppose we could. But since bash lacks stuff like "find all references", it's sort of a pain to do. A few hours of trial-and-error. Does it seem worthwhile?

not a blocker; something to revisit later. may fall out of other rearrangements.

turadg

The upgrade tests looks like it's testing what it needs to.

The rest of the code has been reviewed to get into master. I skimmed it again and left some comments. I leave to you whether they raise anything that requires a change before landing.

turadg · 2023-09-07T23:13:33Z

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

@@ -0,0 +1,2 @@
+upgrade-walletFactory-permit.json


not a blocker; something to revisit later. may fall out of other rearrangements.

turadg · 2023-09-07T23:14:06Z

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

-. ./upgrade-test-scripts/env_setup.sh
+# Dockerfile in upgrade-test sets:
+# WORKDIR /usr/src/agoric-sdk/
+# Overriding it during development has occasionally been useful.


thanks for explanation

turadg · 2023-09-07T23:15:08Z

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

+
+# Pay 0.25IST join the game and get some places
+node ./wallet-all-ertp/gen-game-offer.mjs Shire Mordor >/tmp/,join.json
+agops perf satisfaction --from $GOV1ADDR --executeOffer /tmp/,join.json --keyring-backend=test


NTS: with new Platform PM take stock of our CLI tools

turadg · 2023-09-07T23:15:31Z

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

+node ./wallet-all-ertp/gen-game-offer.mjs Shire Mordor >/tmp/,join.json
+agops perf satisfaction --from $GOV1ADDR --executeOffer /tmp/,join.json --keyring-backend=test
+
+cd $SDK


is this necessary?

no.

it was when the next step was something to do with upgrading zoe.

I'm not sure why I left it in. But doesn't seem worth another ci spin.

turadg · 2023-09-07T23:16:09Z

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

@@ -23,3 +23,45 @@ mv $TMP_GENESIS_DIR/priv_validator_state.json $HOME/.agoric/data
 mv $TMP_GENESIS_DIR/* $HOME/.agoric/config/
 rm -rf $EXPORT_DIR
 startAgd
+
+testMinChildren() {


I see why you wanted to use @iomekam 's JS functions instead!

turadg · 2023-09-07T23:19:09Z

packages/smart-wallet/src/proposals/upgrade-walletFactory-proposal.js

+  console.log('upgradeWalletFactory: config', config);
+  const { walletFactoryRef } = config.options;
+
+  // console.log('upgradeWalletFactory: awaiting instances etc.');


We should try not to have commented-out code.

Please uncomment or remove. I suggest uncomment as its appropriate for proposals to be verbose.

turadg · 2023-09-07T23:22:37Z

packages/smart-wallet/src/proposals/upgrade-walletFactory-proposal.js

+  });
+  // console.log('upgradeWalletFactory: awaiting walletReviver');
+  const walletReviver = await E(ppKit.creatorFacet).getWalletReviver();
+  const newPrivateArgs = harden({


shouldn't these be the same privateArgs as the running instance?

you can get those from instancePrivateArgs in bootstrap promise space.

seems like I tried to do that... trying to remember why I went another way...

I think it was about instancePrivateArgs being only for diagnostic / testing purposes.

But maybe using it in production for upgrade purposes is appropriate?

I wonder about divergence from master, but I think I'll give it a try...

hm...

2023-09-08T01:05:40.125Z SwingSet: vat: v1: upgradeWalletFactory: upgrading with privateArgs { storageNode: Object [Alleged: ChainStorageNode] {}, walletBridgeManager: Object [Alleged: BridgeScopedManager] {}, walletReviver: Promise [Promise] {} } ... 2023-09-08T01:05:40.347Z SwingSet: ls: v2: Error: D() arguments cannot include a Promise at construct ()

https://github.com/Agoric/agoric-sdk/actions/runs/6116183603/job/16601003319?pr=8311#step:6:4288

mhofman · 2023-09-08T17:24:41Z

@dckc mergify is not configured on non-master branches

mhofman · 2023-09-08T17:57:21Z

packages/internal/src/lib-chainStorage.js

Why was this file updated? It has impact on other bundles that won't be published.

And most critically, does anything depend upon this behavior change appearing in a vat that isn't being updated? One concern with packages/internal is that it implies we can change both sides of an interface at the same time, but those changes must be deployed to be available, and deployment isn't automatic.

most critically, does anything depend upon this behavior change appearing in a vat that isn't being updated?

no.

Why was this file updated?

It was based on feedback from @turadg in the context of #8071 (I'm struggling to find the exact comment; might have been over audio).

It has impact on other bundles that won't be published.

Since other bundles aren't being deployed, that's in some ways not a problem, at least not for this particular walletFactory upgrade.

But I'm sympathetic to the concern, and the change is largely stylistic, so I suppose I'm willing to undo, provided you and @turadg agree that's best.

I don't recall the specifics of the conversation. Probably something about not being surprised by the default value.

I support reverting this change. The ergonomics aren't worth the API break.

I'm backing out the change in this file (and related downstream stuff).

But I do need the fakeStorage.getBody() change in storage-test-utils.js below.

np, storage-test-utils is never used in vat code.

This performance optimization avoids all the work of getting an invitation if there aren't sufficient funds for `proposal.give`. fixes: #7098

WIP: note limitation on depositFacet assetKind - feat(smart-wallet): purses for well-known brands - use side table for new purse storage - issuer/brand mutual check - storage faciliteates transition to decentralized issuer introduction - take care with .init() vs. .set()! - diagnosis complicated by obscure "no ordinal" diagnostic - non-vbank asset test tells story in t.log - test: demonstrate how to share brand displayInfo - test: spend before receive non-vbank asset build(smart-wallet): bundle-source devdep SQUASHME: test: bundle handling, IST_UNIT, ... SQUASHME: refactor based on review feedback - XXX move? -> TODO - mutualCheck -> assertMutual with independent failures - hoist getBrandToPurses() - rename to getPurseIfKnownBrand() - inline addIssuer (aka acceptIssuer) - throw on mutualCheck failure SQUASHME: xP fixup receive throw fixup getPurseIfKnownBrand

- mint-ist.sh to pay for publish-bundle adapted from auctioneer-private-args branch - ensure GOV1ADDR is set so we don't give agd too few args and get a weird diagnostic - chore(walletFactory): initHandler / setHandler / upgrading - chore(walletFactory): start -> prepare for compat - chore(game contract): atomicRearrange -> zcf.reallocate() for compat - feat(smart-wallet): publishAgoricBrandsDisplayInfo to vstorage - test(smart-wallet): upgraded walletFactory handles game NFT - install game contract before walletFactory upgrade - move vat-status.mjs to tools/ - move mint-ist, parseProposals to tools/ - chore: split game1 proposal from walletFactory upgrade - chore: move build-walletFactory-upgrade from builders/ to vats/ since vats/ already depends on deploy-script-support - chore: punt zoe noop actions in upgrade-11 - chore: use instancePrivateArgs in upgradeWalletFactory

dckc · 2023-09-08T23:57:38Z

rebasing on release-mainnet1B, in discussion with @warner

(I didn't realize that release-mainnet1B is 1 later than agoric-upgrade-11 until now)

mhofman reviewed Sep 6, 2023

View reviewed changes

packages/smart-wallet/package.json Outdated Show resolved Hide resolved

dckc force-pushed the dc-sw-backport branch from c2b540b to 3e10122 Compare September 6, 2023 21:08

mhofman reviewed Sep 6, 2023

View reviewed changes

dckc requested a review from iomekam September 6, 2023 21:54

dckc marked this pull request as ready for review September 6, 2023 22:51

dckc requested a review from mhofman September 6, 2023 22:51

dckc mentioned this pull request Sep 6, 2023

test walletFactory upgrade on top of upgrade-11 #8308

Closed

15 tasks

iomekam reviewed Sep 7, 2023

View reviewed changes

turadg approved these changes Sep 7, 2023

View reviewed changes

dckc force-pushed the dc-sw-backport branch from 1dd3ba4 to 4ad9814 Compare September 8, 2023 14:28

dckc added the automerge:rebase Automatically rebase updates, then merge label Sep 8, 2023

mhofman removed the automerge:rebase Automatically rebase updates, then merge label Sep 8, 2023

mhofman added the force:integration Force integration tests to run on PR label Sep 8, 2023

mhofman reviewed Sep 8, 2023

View reviewed changes

dckc force-pushed the dc-sw-backport branch from 4ad9814 to 213f901 Compare September 8, 2023 22:04

turadg approved these changes Sep 8, 2023

View reviewed changes

dckc and others added 9 commits September 8, 2023 18:53

feat(internal): fakeStorage.getBody() supports index other than -1

d2e05c4

chore: more FarRef<Payment> lint

1fdb26d

feat(smart-wallet): withdraw payments before getting invitation

82ed64e

This performance optimization avoids all the work of getting an invitation if there aren't sufficient funds for `proposal.give`. fixes: #7098

test(inter-protocol): update "no purse" test

8f7bd2f

style: convert forEach

f6ca5e2

build(smart-wallet): sync bundle-source version

91d3644

style(smart-wallet): backported code conforms to newer coding style

41915e7

dckc force-pushed the dc-sw-backport branch from 213f901 to 41915e7 Compare September 8, 2023 23:54

warner changed the title ~~test: backport walletFactory upgrade to upgrade-11~~ backport walletFactory improvements on top of upgrade-11 branch Sep 9, 2023

dckc merged commit e669bb1 into release-mainnet1B Sep 9, 2023
59 checks passed

dckc deleted the dc-sw-backport branch September 9, 2023 00:59

This was referenced Sep 9, 2023

test of walletFactory contract upgrade on top of upgrade-11 #8305

Closed

fix(walletFactory): move upgrading check before baggage is populated #8322

Merged

walletFactory - migrate chain upgrade test to agoric-3-proposals #8334

Closed

This was referenced Jan 22, 2024

fix(provisioning): export useful namesByAddress (release branch) #8786

Closed

can't build bundles compatible with mainnet due to endo patterns breaking change #8826

Closed

mhofman mentioned this pull request Feb 13, 2024

feat(cosmos): upgrade provisioning vat #8901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backport walletFactory improvements on top of upgrade-11 branch #8311

backport walletFactory improvements on top of upgrade-11 branch #8311

dckc commented Sep 6, 2023 •

edited

Loading

dckc commented Sep 6, 2023

mhofman commented Sep 6, 2023 •

edited

Loading

mhofman Sep 6, 2023

mhofman Sep 6, 2023

dckc Sep 7, 2023

iomekam Sep 7, 2023

dckc Sep 7, 2023

turadg Sep 7, 2023

turadg left a comment

turadg Sep 7, 2023

turadg Sep 7, 2023

turadg Sep 7, 2023

turadg Sep 7, 2023

dckc Sep 7, 2023

turadg Sep 7, 2023

turadg Sep 7, 2023

turadg Sep 7, 2023

dckc Sep 7, 2023

dckc Sep 8, 2023

dckc Sep 8, 2023 •

edited

Loading

mhofman commented Sep 8, 2023

mhofman Sep 8, 2023

warner Sep 8, 2023

dckc Sep 8, 2023

turadg Sep 8, 2023

dckc Sep 8, 2023

turadg Sep 8, 2023

dckc commented Sep 8, 2023

		const importSpec = spec =>
		importMetaResolve(spec, import.meta.url).then(u => new URL(u).pathname);

backport walletFactory improvements on top of upgrade-11 branch #8311

backport walletFactory improvements on top of upgrade-11 branch #8311

Conversation

dckc commented Sep 6, 2023 • edited Loading

Description / Testing / Upgrade Considerations

Security / Scaling Considerations

Documentation Considerations

dckc commented Sep 6, 2023

mhofman commented Sep 6, 2023 • 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

turadg 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 Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

mhofman commented Sep 8, 2023

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 Sep 8, 2023

dckc commented Sep 6, 2023 •

edited

Loading

mhofman commented Sep 6, 2023 •

edited

Loading

dckc Sep 8, 2023 •

edited

Loading