chore: improve use of AssetKind type #4736
Conversation
There was a problem hiding this comment.
Please keep thie IssuerKit type spelled out as discussed below.
And unless we can write 'nat' as NAT, I can't speak for @erights and @Chris-Hibbert on this.
| * V extends CopySetValue ? 'copySet' : | ||
| * V extends CopyBagValue ? 'copyBag' : | ||
| * never} | ||
| * @type {(value: V) => AssetKindForValue<V>} |
There was a problem hiding this comment.
In general, an asset kind determines a value type, but not the other way around. We could have another assetkind that uses bigint values but uses mod p math or some such. The mapping is currently invertible, so I guess this is OK, but it gives me pause.
packages/ERTP/src/amountMath.js
Outdated
| * @param {Amount<V>} rightAmount | ||
| * @template {AssetKind} [K=AssetKind] | ||
| * @param {Amount<K>} leftAmount | ||
| * @param {Amount<K>} rightAmount | ||
| * @param {Brand | undefined} brand |
There was a problem hiding this comment.
| * @param {Brand | undefined} brand | |
| * @param {Brand<K> | undefined} brand |
Pinning down things like the kind is the reason to supply the optional brand.
packages/ERTP/src/amountMath.js
Outdated
| * @param {Amount<V>} rightAmount | ||
| * @template {AssetKind} [K=AssetKind] | ||
| * @param {Amount<K>} leftAmount | ||
| * @param {Amount<K>} rightAmount | ||
| * @param {Brand=} brand |
There was a problem hiding this comment.
| * @param {Brand=} brand | |
| * @param {Brand<K>=} brand |
packages/ERTP/src/amountMath.js
Outdated
| * @param {Amount<V>} rightAmount | ||
| * @template {AssetKind} [K=AssetKind] | ||
| * @param {Amount<K>} leftAmount | ||
| * @param {Amount<K>} rightAmount | ||
| * @param {Brand=} brand |
There was a problem hiding this comment.
| * @param {Brand=} brand | |
| * @param {Brand<K>=} brand |
packages/ERTP/src/amountMath.js
Outdated
| * @param {Amount<V>} rightAmount | ||
| * @template {AssetKind} [K=AssetKind] | ||
| * @param {Amount<K>} leftAmount | ||
| * @param {Amount<K>} rightAmount | ||
| * @param {Brand=} brand |
There was a problem hiding this comment.
| * @param {Brand=} brand | |
| * @param {Brand<K>=} brand |
| * @property {Brand} brand | ||
| * @property {V} value | ||
| * @property {Brand<K>} brand | ||
| * @property {AssetValueForKind<K>} value |
packages/ERTP/src/types.js
Outdated
| @@ -58,6 +58,21 @@ | |||
| */ | |||
|
|
|||
| /** | |||
| * @template {AssetKind} K | |||
| * @typedef {K extends 'nat' ? NatValue: K extends 'set' ? SetValue: K extends 'copySet' ? CopySetValue: K extends 'copyBag' ? CopyBagValue : never} AssetValueForKind | |||
| * | ||
| * `displayInfo` gives information to UI on how to display the amount. | ||
| * | ||
| * @typedef {Object} IssuerKit |
There was a problem hiding this comment.
As noted above, please leave this IssuerKit type spelled out for now.
I'm all for moving the MakeIssuerKit stuff by the function code.
There was a problem hiding this comment.
We discussed offline.
The decision rests on whether IssuerKit is can be made by multiple functions. If so it's an interface and needs its own type declaration. If not, it's like a class constructor and should use ReturnType.
So far nothing else creates objects like this.
agoric-sdk/packages/vats/src/virtual-purse.js
Lines 30 to 35 in 16366f0
We defer to @erights on whether IssuerKit will be instantiated by other functions and I'm removing that change from this PR so that it doesn't need his review to land.
There was a problem hiding this comment.
My concern here is mis-placed. Issuer is the thing that is interface-like that's important to keep spelled out.
| * @param {Amount<'nat'>} debtAmount | ||
| * @param {Amount<'nat'>} collateralAmount |
There was a problem hiding this comment.
| * @param {Amount<'nat'>} debtAmount | |
| * @param {Amount<'nat'>} collateralAmount | |
| * @param {Amount<NAT>} debtAmount | |
| * @param {Amount<NAT>} collateralAmount |
If there is not a straightforward way to arrange this, then I can't speak for @Chris-Hibbert and @erights in approving tihs.
There was a problem hiding this comment.
This can totally be done, by defining const NAT = 'nat' but since NAT is 'nat' then I think what the user sees in an IDE or type error will still be 'nat'.
| * @property {AssetKind} assetKind | ||
| * @property {Brand<K>} brand | ||
| * @property {Issuer<K>} issuer | ||
| * @property {K} assetKind |
There was a problem hiding this comment.
I think this answers one of my earlier questions about constraining assetKind to match brand.
turadg
force-pushed
the
ta/type-brand-assetKind
branch
from
d9f6a56 to
23dc662
Compare
turadg
force-pushed
the
ta/type-brand-assetKind
branch
from
23dc662 to
0857866
Compare
* chore: fix types for Amount kind * fixup! chore: fix types for Amount kind * fixup! chore: fix types for Amount kind
| /** | ||
| * @callback MintPayment | ||
| * | ||
| * Creates a new Payment containing newly minted amount. | ||
| * | ||
| * @param {Amount} newAmount | ||
| * @returns {Payment} |
There was a problem hiding this comment.
I think this type disappeared in the refactor?
| * @property {() => Issuer} getIssuer Gets the Issuer for this mint. | ||
| * @property {MintPayment} mintPayment | ||
| * @property {() => Issuer<K>} getIssuer Gets the Issuer for this mint. | ||
| * @property {MintPayment<K>} mintPayment |
There was a problem hiding this comment.
MintPayment used here has no declaration.
no-ticket, pulled out of #4732 in support of #3788
Description
Our current types for AssetKind have these deficiencies:
AmountMath.makeEmpty(runBrand, 'nat')Amount<NatValue>, more verbose than necessary.This relies on parameterizing assetKind instead of assetValue. That type parameter applies to many other types and doesn't require importing a type in order to use because it's a string literal. E.g.
natinstead of NatValue type. There's no risk of typos because the type parameter K must be a subtype of AssetKind which is the union of 'nat' | 'set' | 'copySet' | 'copyBag', so if someone were to type any string other than one of those four it would be a type failure. ( There may be an aesthetic argument to use AssetKind.NAT (const) but it would be more verbose without any safety benefit: Amount<AssetKind.NAT>.) A suggestion to useAmount<NAT>would be no more verbose and a little cleaner but then you'd have to define or import NAT in the file you're typing that.Security Considerations
--
Documentation Considerations
--
Testing Considerations
tsc