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.

Sign up for GitHub

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

chore: improve use of AssetKind type #4736

Merged
merged 4 commits into from
Mar 10, 2022
Merged

chore: improve use of AssetKind type #4736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0fa3bd4
chore: fix types for Amount kind
turadg Mar 3, 2022
d753ead
fixup! chore: fix types for Amount kind
turadg Mar 8, 2022
17f4c60
Merge branch 'master' into ta/type-brand-assetKind
turadg Mar 10, 2022
0857866
fixup! chore: fix types for Amount kind
turadg Mar 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 55 additions & 60 deletions packages/ERTP/src/amountMath.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,7 @@ const helpers = {

/**
* @template {AmountValue} V
* @type {(value: V) =>
* V extends NatValue ? 'nat' :
* V extends SetValue ? 'set' :
* V extends CopySetValue ? 'copySet' :
* V extends CopyBagValue ? 'copyBag' :
* never}
* @type {(value: V) => AssetKindForValue<V>}
Copy link
Member

Choose a reason for hiding this comment

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

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.

*/
const assertValueGetAssetKind = value => {
const passStyle = passStyleOf(value);
Expand Down Expand Up @@ -148,10 +143,10 @@ const optionalBrandCheck = (allegedBrand, brand) => {
};

/**
* @template {AmountValue} [V=AmountValue]
* @param {Amount<V>} leftAmount
* @param {Amount<V>} rightAmount
* @param {Brand | undefined} brand
* @template {AssetKind} [K=AssetKind]
* @param {Amount<K>} leftAmount
* @param {Amount<K>} rightAmount
* @param {Brand<K> | undefined} brand
* @returns {MathHelpers<*>}
*/
const checkLRAndGetHelpers = (leftAmount, rightAmount, brand = undefined) => {
Expand Down Expand Up @@ -179,11 +174,11 @@ const checkLRAndGetHelpers = (leftAmount, rightAmount, brand = undefined) => {
};

/**
* @template {AmountValue} V
* @param {MathHelpers<AmountValue>} h
* @param {Amount<V>} leftAmount
* @param {Amount<V>} rightAmount
* @returns {[V, V]}
* @template {AssetKind} K
* @param {MathHelpers<AssetKind>} h
* @param {Amount<K>} leftAmount
* @param {Amount<K>} rightAmount
* @returns {[K, K]}
*/
const coerceLR = (h, leftAmount, rightAmount) => {
// @ts-ignore cast (ignore b/c erroring in CI but not my IDE)
Expand All @@ -203,10 +198,10 @@ const AmountMath = {
/**
* Make an amount from a value by adding the brand.
*
* @template {AmountValue} [V=AmountValue]
* @param {Brand} brand
* @param {V extends NatValue ? NatValue : V extends SetValue ? SetValue : V extends CopySetValue ? CopySetValue : V extends CopyBagValue ? CopyBagValue : never} allegedValue
* @returns {Amount<V>}
* @template {AssetKind} [K=AssetKind]
* @param {Brand<K>} brand
* @param {AssetValueForKind<K>} allegedValue
* @returns {Amount<K>}
*/
// allegedValue has a conditional expression for type widening, to prevent V being bound to a a literal like 1n
make: (brand, allegedValue) => {
Expand All @@ -220,10 +215,10 @@ const AmountMath = {
* Make sure this amount is valid enough, and return a corresponding
* valid amount if so.
*
* @template {AmountValue} [V=AmountValue]
* @template {AssetKind} [K=AssetKind]
* @param {Brand} brand
* @param {Amount<V>} allegedAmount
* @returns {Amount<V>}
* @param {Amount<K>} allegedAmount
* @returns {Amount<K>}
*/
coerce: (brand, allegedAmount) => {
assertRemotable(brand, 'brand');
Expand All @@ -240,20 +235,20 @@ const AmountMath = {
/**
* Extract and return the value.
*
* @template {AmountValue} [V=AmountValue]
* @param {Brand} brand
* @param {Amount<V>} amount
* @returns {V}
* @template {AssetKind} [K=AssetKind]
* @param {Brand<K>} brand
* @param {Amount<K>} amount
* @returns {AssetValueForKind<K>}
*/
getValue: (brand, amount) => AmountMath.coerce(brand, amount).value,
/**
* Return the amount representing an empty amount. This is the
* identity element for MathHelpers.add and MatHelpers.subtract.
*
* @template {AssetKind} K
* @param {Brand} brand
* @param {K=} [assetKind='nat']
* @returns {Amount<K extends 'nat' ? NatValue: K extends 'set' ? SetValue: K extends 'copySet' ? CopySetValue: K extends 'copyBag' ? CopyBagValue : never>}
* @param {Brand<K>} brand
* @param {K} [assetKind]
* @returns {Amount<K>}
*/
// @ts-expect-error TS/jsdoc things 'nat' can't be assigned to K subclassing AssetKind
// If we were using TypeScript we'd simply overload the function definition for each case.
Expand All @@ -268,9 +263,9 @@ const AmountMath = {
* Return the amount representing an empty amount, using another
* amount as the template for the brand and assetKind.
*
* @template {AmountValue} V
* @param {Amount<V>} amount
* @returns {Amount<V>}
* @template {AssetKind} K
* @param {Amount<K>} amount
* @returns {Amount<K>}
*/
makeEmptyFromAmount: amount => {
assertRecord(amount, 'amount');
Expand Down Expand Up @@ -302,10 +297,10 @@ const AmountMath = {
* whether rectangle A is greater than rectangle B depends on whether rectangle
* A includes rectangle B as defined by the logic in MathHelpers.
*
* @template {AmountValue} [V=AmountValue]
* @param {Amount<V>} leftAmount
* @param {Amount<V>} rightAmount
* @param {Brand=} brand
* @template {AssetKind} [K=AssetKind]
* @param {Amount<K>} leftAmount
* @param {Amount<K>} rightAmount
* @param {Brand<K>=} brand
* @returns {boolean}
*/
isGTE: (leftAmount, rightAmount, brand = undefined) => {
Expand All @@ -316,10 +311,10 @@ const AmountMath = {
* Returns true if the leftAmount equals the rightAmount. We assume
* that if isGTE is true in both directions, isEqual is also true
*
* @template {AmountValue} [V=AmountValue]
* @param {Amount<V>} leftAmount
* @param {Amount<V>} rightAmount
* @param {Brand=} brand
* @template {AssetKind} [K=AssetKind]
* @param {Amount<K>} leftAmount
* @param {Amount<K>} rightAmount
* @param {Brand<K>=} brand
* @returns {boolean}
*/
isEqual: (leftAmount, rightAmount, brand = undefined) => {
Expand All @@ -333,11 +328,11 @@ const AmountMath = {
* amount, it usually means including all of the elements from both
* left and right.
*
* @template {AmountValue} [V=AmountValue]
* @param {Amount<V>} leftAmount
* @param {Amount<V>} rightAmount
* @param {Brand=} brand
* @returns {Amount<V>}
* @template {AssetKind} [K=AssetKind]
* @param {Amount<K>} leftAmount
* @param {Amount<K>} rightAmount
* @param {Brand<K>=} brand
* @returns {Amount<K>}
*/
add: (leftAmount, rightAmount, brand = undefined) => {
const h = checkLRAndGetHelpers(leftAmount, rightAmount, brand);
Expand All @@ -352,11 +347,11 @@ const AmountMath = {
* left amount must include the right amount, this is NOT equivalent
* to set subtraction.
*
* @template {AmountValue} [V=AmountValue]
* @param {Amount<V>} leftAmount
* @param {Amount<V>} rightAmount
* @param {Brand=} brand
* @returns {Amount<V>}
* @template {AssetKind} [K=AssetKind]
* @param {Amount<K>} leftAmount
* @param {Amount<K>} rightAmount
* @param {Brand<K>=} brand
* @returns {Amount<K>}
*/
subtract: (leftAmount, rightAmount, brand = undefined) => {
const h = checkLRAndGetHelpers(leftAmount, rightAmount, brand);
Expand All @@ -366,21 +361,21 @@ const AmountMath = {
/**
* Returns the min value between x and y using isGTE
*
* @template {AmountValue} [V=AmountValue]
* @param {Amount<V>} x
* @param {Amount<V>} y
* @param {Brand=} brand
* @returns {Amount<V>}
* @template {AssetKind} [K=AssetKind]
* @param {Amount<K>} x
* @param {Amount<K>} y
* @param {Brand<K>=} brand
* @returns {Amount<K>}
*/
min: (x, y, brand = undefined) => (AmountMath.isGTE(x, y, brand) ? y : x),
/**
* Returns the max value between x and y using isGTE
*
* @template {AmountValue} [V=AmountValue]
* @param {Amount<V>} x
* @param {Amount<V>} y
* @param {Brand=} brand
* @returns {Amount<V>}
* @template {AssetKind} [K=AssetKind]
* @param {Amount<K>} x
* @param {Amount<K>} y
* @param {Brand<K>=} brand
* @returns {Amount<K>}
*/
max: (x, y, brand = undefined) => (AmountMath.isGTE(x, y, brand) ? x : y),
};
Expand Down
32 changes: 31 additions & 1 deletion packages/ERTP/src/issuerKit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,38 @@ import { makePaymentLedger } from './paymentLedger.js';
import './types.js';

/**
* @type {MakeIssuerKit}
* @template {AssetKind} K
* The allegedName becomes part of the brand in asset descriptions. The
* allegedName doesn't have to be a string, but it will only be used for
* its value. The allegedName is useful for debugging and double-checking
* assumptions, but should not be trusted.
*
* The assetKind will be used to import a specific mathHelpers
* from the mathHelpers library. For example, natMathHelpers, the
* default, is used for basic fungible tokens.
*
* `displayInfo` gives information to the UI on how to display the amount.
*
* @param {string} allegedName
* @param {K} [assetKind=AssetKind.NAT]
* @param {AdditionalDisplayInfo} [displayInfo={}]
* @param {ShutdownWithFailure=} optShutdownWithFailure If this issuer fails
* in the middle of an atomic action (which btw should never happen), it
* potentially leaves its ledger in a corrupted state. If this function was
* provided, then the failed atomic action will call it, so that some
* larger unit of computation, like the enclosing vat, can be shutdown
* before anything else is corrupted by that corrupted state.
* See https://github.com/Agoric/agoric-sdk/issues/3434
* @returns {{
* mint: Mint<K>,
* issuer: Issuer<K>,
Comment on lines +37 to +38
Copy link
Member

Choose a reason for hiding this comment

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

and lo, there was much rejoicing in the land!

* brand: Brand<K>,
* displayInfo: DisplayInfo,
* }}
*/
const makeIssuerKit = (
allegedName,
// @ts-expect-error K could be instantiated with a different subtype of AssetKind
assetKind = AssetKind.NAT,
displayInfo = harden({}),
optShutdownWithFailure = undefined,
Expand Down Expand Up @@ -61,3 +89,5 @@ const makeIssuerKit = (
harden(makeIssuerKit);

export { makeIssuerKit };

/** @typedef {ReturnType<typeof makeIssuerKit>} IssuerKit */
Copy link
Member

Choose a reason for hiding this comment

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

I'm fond of this idiom for internal use, but for an external API, it seems more important to spell out the type. Callers should not need to look at the implementation of makeIssuerKit to find it out this type. Yes, an IDE can figure it out for you, but we don't always look at code via an IDE.

somewhat related: generating documentation: #4291 . If we can use tsc to generate the spelled out type for documentation purposes, maybe this would be OK. But let's not presume that in this PR.

14 changes: 10 additions & 4 deletions packages/ERTP/src/paymentLedger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ import '@agoric/store/exported.js';
* Make the paymentLedger, the source of truth for the balances of
* payments. All minting and transfer authority originates here.
*
* @template {AssetKind} [K=AssetKind]
* @param {string} allegedName
* @param {Brand} brand
* @param {AssetKind} assetKind
Comment on lines 21 to 22
Copy link
Member

Choose a reason for hiding this comment

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

Again, brand is how the caller picks K, right?

Suggested change
* @param {Brand} brand
* @param {AssetKind} assetKind
* @param {Brand<K>} brand
* @param {AssetKind} assetKind

The assetKind should match K too, but I don't know whether there's a straightforward way to express that.

* @param {DisplayInfo} displayInfo
* @param {ShutdownWithFailure=} optShutdownWithFailure
* @returns {{ issuer: Issuer, mint: Mint }}
* @returns {{ issuer: Issuer<K>, mint: Mint<K> }}
*/
export const makePaymentLedger = (
allegedName,
Expand Down Expand Up @@ -248,7 +249,12 @@ export const makePaymentLedger = (
});
};

/** @type {MintPayment} */
/**
* Creates a new Payment containing newly minted amount.
*
* @param {Amount<K>} newAmount
* @returns {Payment}
Copy link
Member

Choose a reason for hiding this comment

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

Oh... we don't get Payment<K> yet? nor Purse<K>? Hm... going there does seem a little complex, so I'm fine with that.

*/
const mintPayment = newAmount => {
newAmount = coerce(newAmount);
const payment = makePayment(allegedName, brand);
Expand Down Expand Up @@ -331,7 +337,7 @@ export const makePaymentLedger = (
withdraw,
};

/** @type {Issuer} */
/** @type {Issuer<K>} */
const issuer = Far(`${allegedName} issuer`, {
isLive,
getAmountOf,
Expand All @@ -348,7 +354,7 @@ export const makePaymentLedger = (
makePurse(allegedName, assetKind, brand, purseMethods),
});

/** @type {Mint} */
/** @type {Mint<K>} */
const mint = Far(`${allegedName} mint`, {
getIssuer: () => issuer,
mintPayment,
Expand Down
Loading