feat: upgrade walletFactory to go beyond vbank assets #8147
Conversation
manual testing stuffstart the container with I think there's an Then, inside the container: |
dckc
force-pushed
the
7226-sw-nft-issuer
branch
from
44e8a4b to
3c13938
Compare
dckc
force-pushed
the
7226-sw-nft-issuer
branch
2 times, most recently
from
ae13f1a to
e0fbdc1
Compare
...yment/upgrade-test/upgrade-test-scripts/agoric-upgrade-11/wallet-all-ertp/wf-game-propose.sh
Outdated
Show resolved
Hide resolved
dckc
force-pushed
the
7226-sw-nft-issuer
branch
from
15b4af5 to
02cd063
Compare
packages/smart-wallet/src/proposals/upgrade-walletFactory-proposal.js
Outdated
Show resolved
Hide resolved
packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-11/test.sh
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| /** | ||
| * Parse output of `agoric run proposal-builder.js` |
There was a problem hiding this comment.
An alternative is to teach agoric run to produce parseable output. This is the 2nd time I didn't opt to do that.
There was a problem hiding this comment.
Maybe someday. For testing purposes I think we'd just add a parse-smart version to upgradeHelper.js once that's available
| const { body, slots } = JSON.parse(txt); | ||
| const theEntries = zip(JSON.parse(body.slice(1)), slots).map( | ||
| ([[name, ref], boardID]) => { | ||
| const iface = ref.replace(/^\$\d+\./, ''); |
There was a problem hiding this comment.
this has got poor-man's-smallcaps-decoding mixed in. The smallcaps-related hacks could at least be moved into the const smallCaps = {...} widget.
| @@ -48,13 +48,17 @@ export const start = async zcf => { | |||
|
|
|||
| totalPlaces(want.Places) <= 3n || Fail`only 3 places allowed when joining`; | |||
|
|
|||
| atomicRearrange( | |||
There was a problem hiding this comment.
btw... @turadg another static typing nit: getCopyBagEntries returns any.
dckc
force-pushed
the
7226-sw-nft-issuer
branch
from
a94f564 to
d29bc7c
Compare
dckc
force-pushed
the
7226-sw-nft-issuer
branch
from
09a5602 to
7e67b16
Compare
There was a problem hiding this comment.
I have a few questions (most relevantly #8147 (comment) ), but the overall approach does work AFAICT.
| testHasChildren() { | ||
| path=$1 | ||
| min=$2 | ||
| line="$(agd query vstorage children $path -o jsonlines)" |
There was a problem hiding this comment.
What dictates when to use -o json vs. -o jsonlines?
There was a problem hiding this comment.
dunno. jsonlines is what I saw everywhere else.
packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-11/test.sh
Outdated
Show resolved
Hide resolved
packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-11/test.sh
Outdated
Show resolved
Hide resolved
| line="$(agoric follow -lF :published.agoricNames.brand -o text)" | ||
| # find brand by name, then corresponding slot | ||
| id=$(echo $line | jq -r '.slots as $slots | .body | gsub("^#";"") | fromjson | map(.[0]) | index("'$name'") | $slots[.]') | ||
| echo $name Id: $id |
There was a problem hiding this comment.
Was this intentionally left in without some kind of debugging condition?
There was a problem hiding this comment.
All over this PR I waffle about how much logging to do. This is one of the reasons I asked to discuss this with you.
There was a problem hiding this comment.
since it's test code in a framework that hasn't stabilized and everyone is still learning, I think we should bias towards verbosity
|
|
||
| line="$(agoric follow -lF :published.boardAux.$id -o jsonlines)" | ||
| displayInfo="$(echo $line | jq -c .displayInfo)" | ||
| echo test_val "$displayInfo" "$expected" "$name displayInfo from boardAux" |
There was a problem hiding this comment.
| echo test_val "$displayInfo" "$expected" "$name displayInfo from boardAux" | |
| test_val "$displayInfo" "$expected" "$name displayInfo from boardAux" |
packages/smart-wallet/src/proposals/upgrade-walletFactory-proposal.js
Outdated
Show resolved
Hide resolved
packages/smart-wallet/src/proposals/upgrade-walletFactory-proposal.js
Outdated
Show resolved
Hide resolved
| const makeBoardAuxNode = async (chainStorage, boardId) => { | ||
| const boardAux = E(chainStorage).makeChildNode(BOARD_AUX); | ||
| return E(boardAux).makeChildNode(boardId); | ||
| }; |
There was a problem hiding this comment.
Is anything awaiting results from makeBoardAuxNode via publishBrandInfo? Even if not, it would probably make sense to call E(chainStorage).makeChildNode(BOARD_AUX) only once rather than once for every brand.
There was a problem hiding this comment.
I thought I fixed that. Weird.
| const node = makeBoardAuxNode(chainStorage, id); | ||
| const aux = marshalData.toCapData(harden({ displayInfo })); | ||
| await E(node).setValue(JSON.stringify(aux)); |
There was a problem hiding this comment.
This is creating a parallel hierarchy for board-published brands, right? And with a path that is keyed by boardID but lacks an obvious connection to "agoricNames"? Have the right people considered this design and possible alternatives?
There was a problem hiding this comment.
Yes, that's what it's doing. Whether it's sufficiently considered is a good question.
Design considerations included:
- ideally, it would be a platform thing, but add (immutable) auxiliary data to Presences? #2069 is an open research problem
- should the mechanism be for things in agoricNames only or for any brand (or any other remotable, for that matter)?
- we chose: any remotable on the board
- should all the aux data for well-known brands be at one key like
agoricNames.vbankAssetor should each brand have a separate key?- we chose a separate key
Turadg and I discussed those alternatives and agreed to this one. Sam found it inconvenient to make more than one query but eventually agreed it was better than doing O(n) work for each write.
Figuring out who are the right people is an ongoing challenge, for me. I think bringing in your perspective is useful.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
@dckc went over the possible aux data problem with me during recorded office hours today. Since this PR was already otherwise approved and was waiting only on my evaluating of this possible problem, my additional approval here is only approving that there is no such problem here. I have not evaluated the rest of the PR.
Concretely, this is about the canonical concrete example of the problem: That we'd like a presence of a brand to carry the displayInfo. Mapping to the canonical three-party handoff, Carol in VatC is the brand. Alice in VatA sends her presence for Carol to Bob in VatB. VatB, on unmarshalling the message from VatA must form the presence for Carol and deliver it to Bob without waiting for a round trip to VatC. If the brand presence carries the displayInfo at that moment, then VatB would be believing VatA about the displayInfo associated with the brand / Carol, where only VatC should be authoritative about the state of Carol. If VatA lies, then Bob would see the wrong displayInfo.
The entire problem goes away at the cost of a round trip: If the presence itself does not carry the displayInfo, but instead, Bob asks Carol for her displayInfo, then only Carol (and therefore VatC) is taken by Bob as authoritative over Carol's displayInfo. Alice cannot both immediately communicate Carol's identity (by passing a presence) and mislead Bob to believing that identity is associated with a different displayInfo.
The scenario at stake in this PR has none of these hazards. There is no third party less trusted than VatC that can influence the client's notion of what displayInfo is associated with the brand. And the client only obtains the displayInfo after a round trip to querying info that is known only to be updated by those who are legitimately authoritative over that info.
So, if all that makes sense and correctly maps to the question at stake, LGTM. If not, please let me know and we should continue the discussion.
dckc
force-pushed
the
7226-sw-nft-issuer
branch
from
e86d606 to
f2a2b8d
Compare
| * @file Proposal Builder: Upgrade walletFactory | ||
| * | ||
| * Usage: | ||
| * agoric run build-game1-start.js |
| @@ -0,0 +1,2 @@ | |||
| upgrade-walletFactory-permit.json | |||
There was a problem hiding this comment.
question: should we have a global gitignore on -permit.json? if so, should we name them .permit.json to hint that it's a kind of thing?
There was a problem hiding this comment.
It is a kind of thing. (There's a static type and could be a pattern). So I'd support renaming all of them together in a separate PR.
There was a problem hiding this comment.
#8249, draft until this one lands
| @@ -1,13 +1,11 @@ | |||
| #!/bin/bash | |||
|
|
|||
| . ./upgrade-test-scripts/env_setup.sh | |||
| SDK=${SDK:-/usr/src/agoric-sdk} | |||
There was a problem hiding this comment.
please comment what environment provides that source path. Docker? and what wants to override it
| # zoe vat is at incarnation 0 | ||
| test_val "$(yarn --silent node $upgrade11/vat-status.mjs zoe)" "0" "zoe vat incarnation" | ||
| test_val "$(yarn --silent node $upgrade11/tools/vat-status.mjs zoe)" "0" "zoe vat incarnation" |
There was a problem hiding this comment.
remark: probably useful outside upgrade11, but we will refactor in the JS port
There was a problem hiding this comment.
remark: will be totally refactored after #8237
| wfInstance: wfInstanceP, | ||
| ppInstance: ppInstanceP, | ||
| walletBridgeManager: walletBridgeManagerP, | ||
| // @ts-expect-error chainStorage is only falsy in testing |
There was a problem hiding this comment.
nit: shouldn't be necessary.
| // @ts-expect-error chainStorage is only falsy in testing | |
| // @ts-expect-error UNTIL https://github.com/Agoric/agoric-sdk/issues/8247 |
| const { agoricNames, board, assetPublisher } = zcf.getTerms(); | ||
| const upgrading = baggage.has('walletsByAddress'); |
There was a problem hiding this comment.
consider: inlining the has to the conditional below. This is a valid signal of "upgrading" but we don't need that general signal.
There was a problem hiding this comment.
I moved the declaration next to the use.
Fully inlined, it seemed to need a comment. Might as well let the code speak for itself.
| @@ -2,11 +2,10 @@ | |||
| /* eslint @typescript-eslint/no-floating-promises: "warn" */ | |||
|
|
|||
| // deep import to avoid dependency on all of ERTP, vat-data | |||
| ]), | ||
| ); | ||
| // We use the deprecated stage/reallocate API | ||
| // so that we can test this with the version of zoe on mainnet1B. |
|
|
||
| const marshalData = makeMarshal(_val => Fail`data only`); | ||
|
|
||
| // avoid importing all of ERTP |
There was a problem hiding this comment.
above it says a deep import would accomplish this (importing without all of it). yes or no?
There was a problem hiding this comment.
yes.
I think this dates from before I used writeCoreProposal, i.e. when import wouldn't work.
dckc
force-pushed
the
7226-sw-nft-issuer
branch
from
73a6902 to
ff26fa1
Compare
- 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
dckc
force-pushed
the
7226-sw-nft-issuer
branch
from
ff26fa1 to
a0c4ecf
Compare
closes: #8156
refs: #8071
Description / Upgrade Considerations
Security / Scaling Considerations
none that I can think of
Documentation / Testing Considerations
Once we migrate it from mostly-bash to mostly-js, this should make a good example for...