-
Notifications
You must be signed in to change notification settings - Fork 208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(vats): upgradeable mintHolder contract #6894
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
packages/vats/src/mintHolder.js
Outdated
); | ||
export const prepare = (zcf, _privateArgs, instanceBaggage) => { | ||
const provide = () => { | ||
if (!instanceBaggage.has('name')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this name
value looks a little magic. What code defined this string? Can we have a shared constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way I would do this is to add a method to issuerKit:
const hasIssuer = baggage => {
return issuerBaggage.has('name') &&
issuerBaggage.has('assetKind') &&
issuerBaggage.has('displayInfo') &&
issuerBaggage.has('elementShape');
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what that's doing is much clearer to me. no need for constants in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defined it that way, but haven't moved it to @agoric/ertp
yet. Would you deep import it from packages/ERTP/src/issuerKit.js
, @Chris-Hibbert ? Or really make it part of the public ERTP API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a deep import is fine in this case. Whoever is calling it is sharing implementation details with ERTP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
turns out there's an export * from './issuerKit.js';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:niice:
``` BOOT starting buildV1 error during vat dispatch() of startVat,[object Object] (Error#1) Error#1: prepare must be defined to upgrade a contract Error: prepare must be defined to upgrade a contract at construct () at Error (/bundled-source/.../node_modules/ses/src/error/tame-error-constructor.js:56) at makeError (/bundled-source/.../node_modules/ses/src/error/assert.js:242) at fail (/bundled-source/.../node_modules/ses/src/error/assert.js:370) at restartContract (.../zoe/src/contractFacet/zcfZygote.js:398) ```
be8828b
to
0f1cb65
Compare
refs: #6553
Description
Enhance
mintHolder
to not onlymakeDurableIssuerKit
but also re-connect it toinstanceBaggage
on upgrade.Documentation Considerations
makes for sort of a "hello world" upgrade example, maybe?
Testing Considerations
SwingSet test of null upgrade based on test-coveredCall-service-upgrade.js; perhaps not the latest-and-greatest upgrade test harness, but pretty straightforward in this case.