fix Payment type #10828
fix Payment type #10828
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
bc25555
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4eb1f425.agoric-sdk.pages.dev |
| Branch Preview URL: | https://236-type-fixes.agoric-sdk.pages.dev |
turadg
force-pushed
the
236-type-fixes
branch
2 times, most recently
from
c7d1762 to
38fd3af
Compare
| * @param {ERef<Purse>} recoveryPurse | ||
| * @param {ERef<P>} srcPaymentP | ||
| * @template {AssetKind} K | ||
| * @template {Key} M |
There was a problem hiding this comment.
Please rename all these to something other than M.
packages/ERTP/src/issuerKit.js
Outdated
| @@ -322,7 +322,7 @@ harden(prepareIssuerKit); | |||
| * anything else is corrupted by that corrupted state. See | |||
| * https://github.com/Agoric/agoric-sdk/issues/3434 | |||
| * @param {IssuerOptionsRecord} [options] | |||
| * @returns {IssuerKit<K, any>} | |||
| * @returns {IssuerKit<K>} | |||
There was a problem hiding this comment.
Why not also parameterize the type of makeIssuerKit with the thing you're currently calling M? Is this where the inference on the elementShape would go?
There was a problem hiding this comment.
just a matter of prioritization. done now
packages/ERTP/src/types.ts
Outdated
| getAllegedBrand: () => Brand<K>; | ||
| }; | ||
| > = RemotableObject & | ||
| Tagged< |
There was a problem hiding this comment.
I understand that the unfortunate name collision on Tagged comes from elsewhere, so we cannot easily fix it. But especially when juxtaposed with Remotable or other passable type names, it is terribly confusing. Please at least comment all such usage sites.
I see you're passing this through '@agoric/internal/src/tagged.js' anyway. Perhaps we could have the reexport a rename, and then avoid this unfortunate name collision everywhere else? This would be better than commenting each confusing usage site.
There was a problem hiding this comment.
Fair. I've added an alternate export. I kept the rest of the source to make it easier to maintain with changes upstream
| * @type {IssuerKit<'set', { seat: number; show: string; start: string }>} | ||
| */ (makeIssuerKit('Agoric Ballet Opera tickets', AssetKind.SET)); |
There was a problem hiding this comment.
Does it make sense to also pass a corresponding elementShape to this call to makeIssuerKit?
As a test, maybe not worth bothering, so I ask hypothetically: would it have made sense, if code like this appears in production?
There was a problem hiding this comment.
Several sites now use elementShape. I think the illustration is best when there's a TypedPattern shape available, like with InvitationElementShape which I was happy to find. This instance I think serves as a useful example of typing when you don't have an elementShape defined.
| const { mint, issuer, brand } = makeIssuerKit('invitations', AssetKind.SET); | ||
| const { mint, issuer, brand } = | ||
| /** @type {IssuerKit<'set', InvitationDetails>} */ ( | ||
| makeIssuerKit('invitations', AssetKind.SET) |
| * @returns {O['elementShape'] extends TypedPattern<infer T> | ||
| * ? IssuerKit<K, T> | ||
| * : IssuerKit<K>} |
turadg
force-pushed
the
236-type-fixes
branch
from
1ec2322 to
bc25555
Compare
|
Stuck on this error: I'll come back to it when I have time |
refs: https://github.com/Agoric/agoric-private/issues/236
Description
The
Paymenttype wasn't using theM extends Keyparameter, causing an error in type checking whennoUnusedLocalsis enabled.That was placed there in anticipation of using it to convey the type of keys in a set-like amount. This implements that.
It also adds a
makePriceQuoteIssuerto reduce the boilerplate of casting themakeIssuerKit('quote', 'set')to carry thePriceDescriptionkey.Security Considerations
none
Scaling Considerations
none
Documentation Considerations
nothing new
Testing Considerations
existing tests
Upgrade Considerations
none