Skip to content
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

fix(zcf): mint kind during reincarnation #7508

Merged
merged 1 commit into from
Apr 26, 2023
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Apr 25, 2023

closes: #7502

Description

ZCF mint types were failing to restore during reincarnation. @warner traced this down to a faulty argument to zcfMintBaggage.add(). That should have been caught by TypeScript but wasn't due to #7507

There was an additional bug in awaiting a remote call (zoe vat from zcf vat) during reincarnation. This precludes the call during reincarnation by keeping the result in baggage. This changed allowed for less async handling so this also cleans up some related paths.

Security Considerations

issuer record in baggage

Scaling Considerations

--

Documentation Considerations

--

Testing Considerations

Existing tests pass. No unit tests for regression because it will be stressed by contract upgrade tests such as for #7466 in which this problem was discovered.

@turadg turadg requested review from warner and erights April 25, 2023 21:10
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Question about formatting, but don't let it slow things down.

Comment on lines +22 to +26
/** @type {import('@agoric/vat-data').Baggage} */ zcfBaggage,
/** @type {{ (keyword: string, issuerRecord: IssuerRecord): void }} */ recordIssuer,
/** @type {GetAssetKindByBrand} */ getAssetKindByBrand,
/** @type { (exit?: undefined) => { zcfSeat: any; userSeat: Promise<UserSeat> }} */ makeEmptySeatKit,
/** @type {ZcfMintReallocator} */ reallocator,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a radical shift in style from @param. Why? At least at first, I do not find it more readable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the style that "infer params from usage" automates. I think it's worth allowing because it removes the need to repeat the parameter name.

Comment on lines +94 to +95
/** @type {string} */ keyword,
/** @type {IssuerRecord} */ issuerRecord,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I share MarkM's questions, but LGTM, even without the proposed simplification.

) => {
/** @type {SetStore<ZCFMint>} The set of baggages for zcfMints */
/** @type {SetStore<import('@agoric/vat-data').Baggage>} The set of baggages for zcfMints */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to make this type more specific, to declare that it holds a set of zcfMintBaggage (which has a particular set of keys), rather than a set of generic Baggage (which itself is just a MapStore)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be pretty awkward since a MapStore allows any keys. We could have a subtype that maintains a list of keyname to type mappings and conditions get/init/set accordingly but I'm not sure it's a good idea and it's definitely out of scope :)

zcfMintBaggageSet.add(zcfMint);
return zcfMint;

zcfMintBaggage.init('issuerRecord', issuerRecord);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but it now looks awfully like per-zcfMint state, and I wouldn't mind a cleanup pass that replaced the prepareSingleton (one Kind per mint) with a prepare something else that shared a single Kind among all mints, with state.keyword, state.zoeMint, and state.issuerRecord. That would make startup faster, remove the extra Kinds, reduce memory use a little bit, and be generally simpler.

But, probably out of scope for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I'll follow up with that.

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Apr 25, 2023
@mergify mergify bot merged commit e380327 into master Apr 26, 2023
@mergify mergify bot deleted the 7502-reincarnate-feemint branch April 26, 2023 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZCF registerFeeMint fails reincarnation
3 participants