From 153caff2398825bf541f2cbf2777789a454f2603 Mon Sep 17 00:00:00 2001 From: Kate Sills Date: Tue, 3 Nov 2020 11:30:35 -0800 Subject: [PATCH] chore: address PR comments --- packages/ERTP/src/displayInfo.js | 38 +++++++++++++++++++ packages/ERTP/src/issuer.js | 13 ++++--- packages/ERTP/src/types.js | 27 +++++++------ .../ERTP/test/unitTests/test-issuerObj.js | 23 +++++++---- 4 files changed, 78 insertions(+), 23 deletions(-) create mode 100644 packages/ERTP/src/displayInfo.js diff --git a/packages/ERTP/src/displayInfo.js b/packages/ERTP/src/displayInfo.js new file mode 100644 index 000000000000..b008bcb1a083 --- /dev/null +++ b/packages/ERTP/src/displayInfo.js @@ -0,0 +1,38 @@ +import { assert, details, q } from '@agoric/assert'; + +/** + * Assert all values from `part` appear in `whole`. + * + * @param {string[]} whole + * @param {string[]} part + */ +export const assertSubset = (whole, part) => { + part.forEach(key => { + assert.typeof(key, 'string'); + assert( + whole.includes(key), + details`key ${q(key)} was not one of the expected keys ${q(whole)}`, + ); + }); +}; + +// Assert that the keys of `record` are all in `allowedKeys`. If a key +// of `record` is not in `allowedKeys`, throw an error. If a key in +// `allowedKeys` is not a key of record, we do not throw an error. +export const assertKeysAllowed = (allowedKeys, record) => { + const keys = Object.getOwnPropertyNames(record); + assertSubset(allowedKeys, keys); + // assert that there are no symbol properties. + assert( + Object.getOwnPropertySymbols(record).length === 0, + details`no symbol properties allowed`, + ); +}; + +export const assertDisplayInfo = allegedDisplayInfo => { + if (allegedDisplayInfo === undefined) { + return; + } + const displayInfoKeys = harden(['decimalPlaces']); + assertKeysAllowed(displayInfoKeys, allegedDisplayInfo); +}; diff --git a/packages/ERTP/src/issuer.js b/packages/ERTP/src/issuer.js index d58ef51c4279..c785cd563a5f 100644 --- a/packages/ERTP/src/issuer.js +++ b/packages/ERTP/src/issuer.js @@ -10,14 +10,20 @@ import { isPromise } from '@agoric/promise-kit'; import { makeAmountMath, MathKind } from './amountMath'; import { makeInterface, ERTPKind } from './interfaces'; +import { assertDisplayInfo } from './displayInfo'; import './types'; /** * @type {MakeIssuerKit} */ -function makeIssuerKit(allegedName, amountMathKind = MathKind.NAT, decimals) { +function makeIssuerKit( + allegedName, + amountMathKind = MathKind.NAT, + displayInfo = undefined, +) { assert.typeof(allegedName, 'string'); + assertDisplayInfo(displayInfo); const brand = Remotable( makeInterface(allegedName, ERTPKind.BRAND), @@ -32,10 +38,7 @@ function makeIssuerKit(allegedName, amountMathKind = MathKind.NAT, decimals) { getAllegedName: () => allegedName, // Give information to UI on how to display the amount. - // Fungible digital assets should be represented in integers, in - // the smallest unit (i.e. USD might be represented in mill, - // a thousandth of a dollar. In that case, `decimals` would be 3). - decimals: () => decimals, + getDisplayInfo: () => displayInfo, }, ); diff --git a/packages/ERTP/src/types.js b/packages/ERTP/src/types.js index 6aea2a024c37..0461c35681a3 100644 --- a/packages/ERTP/src/types.js +++ b/packages/ERTP/src/types.js @@ -97,6 +97,19 @@ * to set subtraction. */ +/** + * @typedef {Object} DisplayInfo + * @property {number} decimalPlaces + * Tells the display software how many decimal places to move the + * decimal over to the left, or in other words, which position corresponds to whole + * numbers. We require fungible digital assets to be represented in + * integers, in the smallest unit (i.e. USD might be represented in mill, + * a thousandth of a dollar. In that case, `decimalPlaces` would be 3.) + * For non-fungible digital assets, this should be left as undefined. + * The decimalPlaces property should be used for *display purposes only*. Any + * other use is an anti-pattern. + */ + /** * @typedef {Object} Brand * The brand identifies the kind of issuer, and has a function to get the @@ -112,12 +125,8 @@ * @property {(allegedIssuer: ERef) => Promise} isMyIssuer Should be used with * `issuer.getBrand` to ensure an issuer and brand match. * @property {() => string} getAllegedName - * @property {() => number} decimals + * @property {() => DisplayInfo} getDisplayInfo * Give information to UI on how to display the amount. - * Fungible digital assets should be represented in integers, in - * the smallest unit (i.e. USD might be represented in mill, - * a thousandth of a dollar. In that case, `decimals` would be 3). - * Should not be used for nonfungible digital assets. */ /** @@ -195,7 +204,7 @@ * @callback MakeIssuerKit * @param {string} allegedName * @param {AmountMathKind} [amountMathKind=MathKind.NAT] - * @param {number} [decimals] + * @param {DisplayInfo=} [displayInfo=undefined] * @returns {IssuerKit} * * The allegedName becomes part of the brand in asset descriptions. The @@ -207,11 +216,7 @@ * from the mathHelpers library. For example, natMathHelpers, the * default, is used for basic fungible tokens. * - * `decimals` gives information to UI on how to display the amount. - * Fungible digital assets should be represented in integers, in - * the smallest unit (i.e. USD might be represented in mill, - * a thousandth of a dollar. In that case, `decimals` would be 3). - * `decimals` should not be used for nonfungible digital assets. + * `displayInfo` gives information to UI on how to display the amount. * * @typedef {Object} IssuerKit * The return value of makeIssuerKit diff --git a/packages/ERTP/test/unitTests/test-issuerObj.js b/packages/ERTP/test/unitTests/test-issuerObj.js index 39e67a905eab..2bc7792cd133 100644 --- a/packages/ERTP/test/unitTests/test-issuerObj.js +++ b/packages/ERTP/test/unitTests/test-issuerObj.js @@ -14,26 +14,35 @@ test('issuer.getBrand, brand.isMyIssuer', t => { ); t.is(issuer.getAllegedName(), myBrand.getAllegedName()); t.is(issuer.getAllegedName(), 'fungible'); - t.is(brand.decimals(), undefined); + t.is(brand.getDisplayInfo(), undefined); }); -test('brand decimals', t => { - const { brand } = makeIssuerKit('fungible', MathKind.NAT, 3); - t.is(brand.decimals(), 3); +test('brand.getDisplayInfo()', t => { + const displayInfo = harden({ decimalPlaces: 3 }); + const { brand } = makeIssuerKit('fungible', MathKind.NAT, displayInfo); + t.is(brand.getDisplayInfo(), displayInfo); const display = amount => { const { brand: myBrand, value } = amount; - const decimals = myBrand.decimals(); + const { decimalPlaces } = myBrand.getDisplayInfo(); const valueDisplay = value.toString(); const length = valueDisplay.length; return [ - valueDisplay.slice(0, length - decimals), + valueDisplay.slice(0, length - decimalPlaces), '.', - valueDisplay.slice(length - decimals), + valueDisplay.slice(length - decimalPlaces), ].join(''); }; t.is(display({ brand, value: 3000 }), '3.000'); }); +test('bad display info', t => { + const displayInfo = harden({ somethingUnexpected: 3 }); + t.throws(() => makeIssuerKit('fungible', MathKind.NAT, displayInfo), { + message: + 'key "somethingUnexpected" was not one of the expected keys ["decimalPlaces"]', + }); +}); + test('amountMath from makeIssuerKit', async t => { const { issuer, amountMath, brand } = makeIssuerKit('fungible'); const ibrand = await E(issuer).getBrand();